Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Polish Settings UI: Keyboard navigation (w/ tab key) #8768

Closed
2 of 6 tasks
carlos-zamora opened this issue Jan 12, 2021 · 5 comments · Fixed by #9196
Closed
2 of 6 tasks

Polish Settings UI: Keyboard navigation (w/ tab key) #8768

carlos-zamora opened this issue Jan 12, 2021 · 5 comments · Fixed by #9196
Labels
Area-SettingsUI Anything specific to the SUI Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal v1.7

Comments

@carlos-zamora
Copy link
Member

carlos-zamora commented Jan 12, 2021 •
edited
Loading

This is a list of issues we've found when trying to navigate the Settings UI using only the keyboard:

  • On Color Schemes edit page (with a custom scheme open so it's editable) use the tab key to navigate between controls. Press Tab from +AddNew and it will go select nothingness. One more tab will select the Black box. It should probably never select nothingness.
    • It also selects nothingness between Bright White and Foreground. Bright White (Tab) Nothingness (Tab) Foreground.
  • On all pages, it appears that once you tab all the way to Apply in the bottom right corner… you get stuck. I think tab navigation is supposed to be fully circular (going back to the first thing or the most Shift+Tabby thing) and around again.
  • Open SUI. Switch to a TerminalTab. Invoke "open Settings UI" kbd again. Hamburger menu focused.
    • Expected: highlight last highlighted setting

Potentially upstream issues:
@carlos-zamora carlos-zamora added Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Priority-2 A description (P2) Area-SettingsUI Anything specific to the SUI labels Jan 12, 2021
@carlos-zamora carlos-zamora added this to the Terminal v1.6 milestone Jan 12, 2021
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jan 12, 2021
@carlos-zamora carlos-zamora removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jan 12, 2021
@carlos-zamora carlos-zamora mentioned this issue Feb 5, 2021
Merged
1 task
ghost pushed a commit that referenced this issue Feb 8, 2021
@carlos-zamora
Represent inheritance in Settings UI (#8919)
3b7b200 
## Summary of the Pull Request
Introduces the `SettingContainer`. `SettingContainer` is used to wrap a setting in the settings UI and provide the following functionality:
- a reset button next to the header
- tooltips and automation properties for the setting being wrapped
- a comment stating if you are currently overriding a setting

## References
[Spec - Inheritance in Settings UI](https://github.com/microsoft/terminal/blob/main/doc/specs/%231564%20-%20Settings%20UI/cascading-settings.md)
#8804 - removes the ambiguity of leaving a setting blank
#6800 - Settings UI Epic
#8899 - Automation properties for Settings UI
#8768 - Keyboard Navigation

## PR Checklist
* [X] Closes #8804

## Detailed Description of the Pull Request / Additional comments
A few highlights in this PR:
- CommonResources.xaml:
  - we need to merge the SettingContainerStyle.xaml in there. Otherwise, XAML doesn't merge these files properly and can't apply the template.
- Profiles.cpp:
  - view model checks if the starting directory and background image were reset, to determine which value to show when unchecking the special value
  - `Profiles::OnNavigatedTo()` needs a property changed handler to update its own "Current<Setting>" and update the UI properly
- Profiles.xaml:
  - basically wrapped all of the settings we want to be inheritable in there
  - `Binding` is used instead of `x:Bind` in some places because `x:Bind` can't find the parent `SettingContainer` and gives you a compiler error.
- Resources.resw:
  - had to set the "HeaderText" and "HelpText" on each setting container. Does a decent localization burden, unfortunately.
- `SettingContainer` files
  - This operates by creating a template and applying that template over other settings. This allows you to inject the existing controls inside of this. This means that we need to provide our UIElements names and access/modify them via `OnApplyTemplate`
  - We had to remove the header from each individual control, and have `SettingContainer` be in charge of it. This allows us to add the reset button in there.
  - Due to the problem mentioned earlier about CommonResources.xaml, we can't reference anything from CommonResources.xaml.
  - Using `DependencyProperty` to let us set a few properties in the XML files. Particularly, `Has<Setting>` and `Clear<Setting>` are what do all the heavy lifting of interacting with the inheritance model.

## Demo
![Inheritance Demo](https://user-images.githubusercontent.com/11050425/106192086-92a56680-6160-11eb-838c-4ec0beb54965.gif)

## Validation Steps Performed
- Verified correct binding behavior with the following generic setting controls:
  - radio buttons
  - toggle switch
  - text block
  - slider
  - settings with browse buttons
  - the background image alignment control
  - controls with special check boxes (starting directory and background image)

## Next Steps
- The automation properties have been verified using NVDA. This is a part of resolving #8899.
- The override text is currently "Overrides a setting". According to #8269, we actually want to add a hyperlink in there that navigates to the parent profile object. This will be a follow-up task as it requires settings model changes.
@carlos-zamora carlos-zamora mentioned this issue Feb 11, 2021
Merged
@carlos-zamora
Copy link
Member Author

Ok. I've done a bit of research on this issue. Here's some notes:

  • color schemes page
    • will be fixed as a part of the color schemes redesign
  • focus doesn't cycle on save button
  • highlight last highlighted setting
    • Tried using the FocusManager to store the last focused item (in SettingsTab and MainPage). Neither worked. This will require a more complex solution.
    • I'll be pulling this out into its own issue and labelling it as help wanted. I feel that it doesn't have that high of a priority (at least compared to other things).
  • tab navigation in left bar
    • this one now feels like it's by design. Closing.
  • radio buttons
    • already tagged
  • combobox
    • This one's tough. ComboBox has an IsTextSearchEnabled property that enabled this functionality. However, it only works if the list of items are "strings". Since we're defining an item template to add an icon or directly bind to the settings model, we can't explicitly use this feature and have icons at the same time.
    • I'm going to file a feature request on XAML for this one.

ghost pushed a commit that referenced this issue Feb 17, 2021
@carlos-zamora
Perform polish of miscellaneous Settings UI bugs (#9126)
557edc6 
- 0b0dbdf Makes the browse buttons center vertically aligned
  - This is now made possible by #8919. The "center" used to include the height of the header. Now that it's separated, the center is solely calculated to be the text box.
  - Closes #8764 
- 0288f06 Fix keyboard navigation focus for color schemes rename button
  - Enter/Esc when in the scheme renamer now focuses the combo box
  - Keyboard-invoking accept/cancel button focuses the rename button
  - References #8765 and #8768
- d5ef552 Cyclical tab navigation
  - now, if you try to tab past the save button, you cycle back to the beginning of the navigation view
  - this is consistent with the xaml controls gallery
  - References #8768 
- a613b08 AutomationProperties for Save, Reset, and open json buttons
  - References #8899
@carlos-zamora
Copy link
Member Author

The ComboBox feature request is being tracked in microsoft/microsoft-ui-xaml#4182

@DHowett
Copy link
Member

DHowett commented Feb 18, 2021

Did you try StMoy's suggestion?

@carlos-zamora
Copy link
Member Author

Did you try StMoy's suggestion?

Not yet. I've been working on the color schemes page redesign.

@carlos-zamora carlos-zamora mentioned this issue Feb 18, 2021
Merged
@ghost ghost added the In-PR This issue has a related PR label Feb 18, 2021
@carlos-zamora carlos-zamora mentioned this issue Feb 18, 2021
Merged
ghost pushed a commit that referenced this issue Feb 19, 2021
@carlos-zamora
Enable text search on combo boxes (#9206)
2c22b68 
`ComboBox` has a text search function that allows users to type letters, and the `ComboBoxItem` starting with those letters is shown. In order to enable this functionality, the underlying items must be `IStringable`. This exposes a `ToString()` function and fixes all of our issues.

This PR adds the `IStringable` interface to `ColorScheme`, `Profile`, and `EnumEntry`.

## References
#6800 - Settings UI Epic
#8768 - Keyboard Navigation
microsoft/microsoft-ui-xaml#4182 - discussion with WinUI about how to overcome this issue

## Validation Steps Performed
Tested...
- Launch > Default Profile
- Color Schemes > Name
- Profile > Appearance > Color scheme
- Profile > Appearance > Font weight

Also tested radio buttons, but those still don't work, unfortunately. Looks like they don't have the same underlying mechanism.
@ghost ghost closed this as completed in #9196 Feb 19, 2021
ghost pushed a commit that referenced this issue Feb 19, 2021
@carlos-zamora
Redesign color schemes page (#9196)
bb0e1d3 
This PR performs a large overall polish of the color schemes page:
- Ensures keyboard navigation is holistically improved (i.e. fully
  accessible, no lost focus, etc...)
- Adds tooltips and automation properties to all controls
- Redesigns the page according to @mdtauk's approved design
  ([link](#8997 (comment))).
  Note, there are some minor modifications to the design that were
  approved by @cinnamon-msft.
- Automatically reflow's the color buttons when they do not fit in
  horizontal mode

## Detailed Description of the Pull Request / Additional comments
- Redesign
  - a data template was introduced to make color representation
    consistent and straightforward
  - `ContentControl` is used to hold a reference to the
    `ColorTableEntry` and represent it properly using the aforementioned
    data template.
  - The design is mainly a StackPanel holding two grids: color table &
    functional colors.
  - The color table is populated via code. After much thought, this
    seems to be the easiest way to correctly bind 16 controls that are
    very similar.
  - The functional colors are populated via XAML manually.
  - We need a grid to separate the text and the buttons. This allows for
    scenarios like "selection background is the longest string" to force
    the buttons and text to be aligned.
- Reflow
  - A `VisualStateManager` uses an `AdaptiveTrigger` to change the
    orientation of the color tables' stack panel. The adaptive trigger
    was carefully calculated to align with the navigation view's
    breakpoint.
- Keyboard Navigation
  - (a lesson from `SettingContainer`) `ContentControl` can be focused
    as opposed to the content inside of it. We don't want that, so we
    set `IsTabStop` to false on it. That basically fixes all of our
    keyboard navigation issues in this new design.
- Automation Properties and ToolTips
  - As in my previous PRs, I can't seem to figure out how to bind to a
    control's automation property to its own tooltip. So I had to do
    this in the code and add a few `x:Name` around.

## Validation Steps Performed
- Manually tested...
  - tab navigation
  - accessibility insights
  - NVDA
  - changing color schemes updates color table
- specific scenario:
  - change a color table color and a functional color
  - navigate to a different color scheme
  - navigate back to the first color scheme
  - if the colors persist, the changes were propagated to the settings model

References #8997 - Based on the work from @Chips1234
References #6800 - Settings UI Epic
Closes #8765 - Polish Color Schemes page
Closes #8899 - Automation Properties
Closes #8768 - Keyboard Navigation
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Feb 19, 2021
@ghost
Copy link

ghost commented Mar 1, 2021

🎉This issue was addressed in #9196, which has now been successfully released as Windows Terminal Preview v1.7.572.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-SettingsUI Anything specific to the SUI Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal v1.7
Development

Successfully merging a pull request may close this issue.

Redesign color schemes page microsoft/terminal
3 participants
@DHowett @carlos-zamora @cinnamon-msft