Skip to content

Conversation

@nicolaihenriksen
Copy link
Contributor

@nicolaihenriksen nicolaihenriksen commented Feb 25, 2023

Fixes #3094

As per the discussion thread below, this PR adds DialogHost.RestoreFocusElement and DialogHost.IsRestoreFocusDisabled attached/dependency properties.

The former is currently used for the TabItem style (also the one for the "rail item") to ensure focus is returned to the parent TabControl rather than the TabItem itself.

The latter is used to completely disable the restore focus feature in case that is relevant.

Added 4 UI tests to verify the behavior.

@Keboo
Copy link
Member

Keboo commented Feb 26, 2023

After giving it some thought here would be my proposal.

  1. Create a new DialogHost.RestoreFocusElement (perhaps with a better name)
  2. When setting DialogHost._restoreFocusDialogClose first check that element to see if the above attached property has a value, if it does use it for the _restoreFocusDialogClose value.
  3. In the style for the TabItem (MaterialDesignTabItem) we do a relative binding looking for a parent tab control (something like: {Binding RelativeSource={RelativeSource FindAncestor, AncestorType=TabControl}}). We may need to look at other styles as well, such as MaterialDesignNavigationRailTabItem.
  4. Optionally, we could consider a mechanism for turning off the restore focus behavior (I say optional because rarely has anyone ever requested this). But we could create a static property on DialogHost (something like DialogHost.DisableRestoreFocus or a better name). Then in step 2 above if that is the value of the attached property, we would set DialogHost._restoreFocusDialogClose to null to effectively disable the behavior.

Thoughts?

@nicolaihenriksen
Copy link
Contributor Author

nicolaihenriksen commented Feb 27, 2023

After giving it some thought here would be my proposal.
...
Thoughts?

I think this sound like a very good approach. I very much like the idea that the responsibility for picking the correct element to focus is removed from the DialogHost and put into the respective controls themselves. This aligns nicely with the Tell Don't Ask principle. Also, it is probably a rather limited number of controls which face this type of (possible) issue.

Having the additional option to completely disable the restore behavior also seems like a good idea to me. We'll just default the property to false such that it is something a caller would have to explicitly request for the DialogHost.

I have added the properties discussed and UI tests to test the scenarios. What remains is agreeing on the names of the properties, and applying the attached property on other controls suffering from the same issue (e.g. MaterialDesignNavigationRailTabItem)

@Keboo
Copy link
Member

Keboo commented Feb 28, 2023

This is great. I think there is a bit of a disjoint on the names of the properties. (FocusRestore vs RestoreFocus). I think IsFocusRestoreDisabled should probably be the one changed to IsRestoreFocusDisabled. Thoughts?

@nicolaihenriksen
Copy link
Contributor Author

This is great. I think there is a bit of a disjoint on the names of the properties. (FocusRestore vs RestoreFocus). I think IsFocusRestoreDisabled should probably be the one changed to IsRestoreFocusDisabled. Thoughts?

I agree, I'll change it.

@nicolaihenriksen nicolaihenriksen changed the title WIP: TabControl.SelectedIndex via DialogHost Properly restore focus when TabControl.SelectedIndex is set via DialogHost Feb 28, 2023
@nicolaihenriksen nicolaihenriksen marked this pull request as ready for review February 28, 2023 07:58
@VR462002
Copy link

Hi Nicolai and Kevin,

thank you for working on this, you guys are awesome!

@nicolaihenriksen Thanks for testing and explaining the focus problem!
@Keboo Always appreciate your input and enjoy your live videos/recordings

@Keboo Keboo merged commit b53fbd8 into MaterialDesignInXAML:master Feb 28, 2023
@Keboo Keboo added this to the 4.8.0 milestone Feb 28, 2023
@Keboo Keboo added the release notes Items are likely to be highlighted in the release notes. label Feb 28, 2023
@nicolaihenriksen nicolaihenriksen deleted the fix3094 branch March 1, 2023 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release notes Items are likely to be highlighted in the release notes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TabControl.SelectedIndex via DialogHost (repo)

3 participants