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.

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

Add automation properties to Settings UI #8899

Closed
carlos-zamora opened this issue Jan 26, 2021 · 1 comment · Fixed by #9196
Closed

Add automation properties to Settings UI #8899

carlos-zamora opened this issue Jan 26, 2021 · 1 comment · Fixed by #9196
Assignees
Labels
Area-Settings UI Anything specific to the SUI Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@carlos-zamora
Copy link
Member

Description of the new feature/enhancement

We need to add automation properties throughout the settings UI. Here are a few examples that really need to be addressed:

  • Profile > Appearance > background image alignment control
  • color schemes > buttons
  • Startup > default profile > combo box items
  • (unverified) Profile/ColorSchemes > Delete button (and any other buttons with icons)

Proposed technical implementation details (optional)

The general approach to adding automation properties is to add something like this to the resw file:

<data name="Profile_BackgroundImageAlignmentBottom.[using:Windows.UI.Xaml.Automation]AutomationProperties.Name" xml:space="preserve">
	<value>Align to Bottom</value>
</data>

However, this causes some localization burden, and since the help text will most likely match the tooltip, it would be more ideal if we could reuse the same resw entry. This would ensure that they're always in sync.

I think CommandPalette had to do something similar to this.

@carlos-zamora carlos-zamora added Product-Terminal The new Windows Terminal. Issue-Task It's a feature request, but it doesn't really need a major design. Area-Settings UI Anything specific to the SUI labels Jan 26, 2021
@carlos-zamora carlos-zamora added this to the Terminal v1.7 milestone Jan 26, 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 26, 2021
@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jan 26, 2021
ghost pushed a commit that referenced this issue Feb 8, 2021
## 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 self-assigned this Feb 11, 2021
ghost pushed a commit that referenced this issue Feb 17, 2021
- 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
@ghost ghost added the In-PR This issue has a related PR label Feb 18, 2021
@ghost ghost closed this as completed in #9196 Feb 19, 2021
ghost pushed a commit that referenced this issue Feb 19, 2021
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
Labels
Area-Settings UI Anything specific to the SUI Issue-Task It's a feature request, but it doesn't really need a major design. 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
Development

Successfully merging a pull request may close this issue.

2 participants