Skip to content

fix: Prevent focus reset for DataGridTemplateColumn #4148

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

fix: Prevent focus reset for DataGridTemplateColumn #4148

wants to merge 1 commit into from

Conversation

lukeblevins
Copy link
Contributor

@lukeblevins lukeblevins commented Jul 30, 2021

Fixes #2493

This prevents DataGrid from assuming focus should be reset back to itself when a control inside DataGridTemplateColumn gains focus. This is my first WCT PR, so please go easy on me 😆

PR Type

What kind of change does this PR introduce?

Bugfix

What is the current behavior?

Trying to open a control like CalendarDatePicker from within a DataGridTemplateColumn doesn't work, as there is specific behavior to detect when DataGrid loses focus and return it. This doesn't make much sense for controls which require focus to function properly.

What is the new behavior?

When the DataGrid loses focus, we now specifically detect if the editing column is a DataGridTemplateColumn and alter the behavior to accommodate this.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

Other information

I'm unsure if this is the best way to do this, but it makes the most sense given how DataGrid already has the capability to detect the editing element and column type.

Related: unoplatform/uno#6543

@ghost
Copy link

ghost commented Jul 30, 2021

Thanks duke7553 for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost requested review from michael-hawker, azchohfi and Rosuavio July 30, 2021 18:36
@ghost ghost added bug 🐛 An unexpected issue that highlights incorrect behavior DataGrid 🔠 Issues on DataGrid control labels Jul 30, 2021
@michael-hawker michael-hawker added this to the 7.1 milestone Jul 30, 2021
@michael-hawker
Copy link
Member

xmlns:controls="using:Microsoft.Toolkit.Uwp.UI.Controls" 
xmlns:converters="using:Microsoft.Toolkit.Uwp.UI.Converters">
<Page.Resources>
      <converters:StringFormatConverter x:Key="StringFormatConverter"/>
        <DataTemplate x:Key="CalendarCell">
            <CalendarDatePicker Margin="3,4,3,3"/>
         </DataTemplate>
    </Page.Resources>

    <Grid>
      <StackPanel>
         <StackPanel Orientation="Horizontal" Margin="0,10,0,0">
            <TextBlock Text="Sample Standard CalendarDatePicker on a page:" Margin="5,3,0,0"/>
            <CalendarDatePicker Name="cdpMyDate"  Margin="8,0,0,0" />
         </StackPanel>
   
      <controls:DataGrid Name="dgDates" AutoGenerateColumns="false" Margin="8,8,0,0"  BorderBrush="LightGray" BorderThickness="1" GridLinesVisibility="Horizontal" Width="250" Height="200" HorizontalAlignment="Left">
         <controls:DataGrid .Columns>
               <controls:DataGridTemplateColumn CellTemplate="{StaticResource CalendarCell}" IsReadOnly="True" Header="Date"/>
               <controls:DataGridTextColumn Binding="{Binding Text}" Header="Description"/>
            </controls:DataGrid .Columns>
      </controls:DataGrid >
      </StackPanel>
   </Grid>

Think this modified example can help us test in the sample app, though we need to add DataGridTemplateColumn to our App.xaml for not being optimized out.

Copy link
Contributor

@XAML-Knight XAML-Knight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving this PR, as it relates specifically to #2493

Testing was successful, following @michael-hawker's suggestion for the SampleApp. I did have to add an extra column for <controls:DataGridTemplateColumn /> to the sample page DataGridPage.xaml.


if (focusLeftDataGrid && editingColumn is DataGridTemplateColumn)
{
dataGridWillReceiveRoutedEvent = false;
Copy link
Contributor Author

@lukeblevins lukeblevins Aug 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@XAML-Knight @michael-hawker Wouldn't it make more sense to simply return here, rather than hook up to the LostFocus event below? I don't think that behavior works the way I thought it would.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@duke7553, feel free to submit an issue for this. FWIW, the unit tests pass if we simply returned from here, and the behavior of the DataGrid appears to remain the same, as we'd expect.

@XAML-Knight
Copy link
Contributor

@michael-hawker , @RosarioPulella could someone take a look at & review this PR?

@ghost ghost removed the needs attention 👋 label Aug 16, 2021
@Rosuavio
Copy link
Contributor

@duke7553 can you rename your branch? We mention that contributors must use feature branches in our wiki

https://github.com/CommunityToolkit/WindowsCommunityToolkit/wiki/Create-and-Submit-PullRequest

⚠️ We will not merge the PR to the main repo if your changes are not in a feature branch of your forked repository ⚠️

@lukeblevins lukeblevins deleted the branch CommunityToolkit:main August 31, 2021 15:21
@lukeblevins lukeblevins deleted the main branch August 31, 2021 15:21
@lukeblevins lukeblevins restored the main branch August 31, 2021 15:22
@Rosuavio
Copy link
Contributor

I am not sure but you might have to open a new PR

@lukeblevins
Copy link
Contributor Author

@RosarioPulella Good catch! I'll keep that in mind for next time.

Let's pick up at #4206

@jeromelaban
Copy link
Contributor

@RosarioPulella I think it'd be interesting to specify an example of what a feature branch is for this repo (not the main branch, whichever the name and format, if I understand correctly). "Feature" has very different meanings depending on repositories.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior DataGrid 🔠 Issues on DataGrid control
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DataGrid DataGridTemplateColumn CalendarDatePicker and DatePicker Flash Then Desappear!
5 participants