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

Radial Gauge accessibility: high contrast and narrator #2522

Merged
merged 23 commits into from Oct 22, 2018

Conversation

Projects
None yet
5 participants
@XamlBrewer
Copy link
Collaborator

commented Oct 2, 2018

Issue: #2276

PR Type

What kind of change does this PR introduce?

  • Other: Control Accessibility

What is the current behavior?

Radial Gauge color scheme is not appropriate in high contrast mode (e.g. needle and background have same color).
Radial Gauge control is not detected by Narrator (only the text is).

What is the new behavior?

Radial Gauge components are distinguishable in all high contrast modes.
Radial Gauge control is detected and described by Narrator as a custom control.

PR Checklist

Please check if your PR fulfills the following requirements:

  • [*] Tested code with current supported SDKs
  • Docs have been added/updated which fit documentation template. (for bug fixes / features)
  • Sample in sample app has been added / updated (for bug fixes / features)
  • 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

XamlBrewer and others added some commits Oct 2, 2018

@nmetulev

This comment has been minimized.

Copy link
Member

commented Oct 3, 2018

@kbrons, could you test this against your test app?

@lucaasrojas

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2018

I tested it in a small app and it worked. But, the control's colors only changed based on the high contrast theme. I think that you should be able to customize the colors and, when high contrast is enabled, change the colors based on the theme. (See the last picture).
In addition, the Narrator didn't recognize the control. It only says the window's name.

Without changes
image

With changes
image

Color issue
image

@XamlBrewer

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 5, 2018

About the color issue: do you mean that the control should ignore all overridden colors (like the NeedleBrush and the TickBrush: Blue and Red) when in high contrast mode, but instead use the default theme resource brushes (in this case respectively SystemControlHighlightChromeHighBrush and SystemControlForegroundBaseHighBrush) ?

About the narrator: it should work - when the control gets the focus. Maybe your sample app is preventing that.

@XamlBrewer XamlBrewer closed this Oct 5, 2018

@XamlBrewer XamlBrewer reopened this Oct 5, 2018

@lucaasrojas

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2018

Exactly, I should be able to change the colors without the high contrast mode but when it is enabled, the colors should be ignored and overwritten with the theme's ones. About the Narrator, could be a problem of my app, let's see what other member says about it.

@XamlBrewer

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 5, 2018

Thanks for the clarification.

XamlBrewer and others added some commits Oct 8, 2018

@nmetulev

This comment has been minimized.

Copy link
Member

commented Oct 8, 2018

Looks like the build is failing because of:

RadialGauge\RadialGauge.cs(603,29): error SA1204: Static members must appear before non-static members [D:\a\1\s\Microsoft.Toolkit.Uwp.UI.Controls\Microsoft.Toolkit.Uwp.UI.Controls.csproj]
RadialGauge\RadialGauge.cs(174,30): error SA1203: Constant fields must appear before non-constant fields [D:\a\1\s\Microsoft.Toolkit.Uwp.UI.Controls\Microsoft.Toolkit.Uwp.UI.Controls.csproj]

XamlBrewer and others added some commits Oct 9, 2018

@nmetulev
Copy link
Member

left a comment

I tested narrator and was not able to get it to work as well. The control was getting focus but the narrator was not describing it. All other focused elements around the control worked fine with the narrator.

@nmetulev

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

@kbrons, please review

@@ -191,9 +200,17 @@ public RadialGauge()
{
DefaultStyleKey = typeof(RadialGauge);

ThemeListener.ThemeChanged -= ThemeListener_ThemeChanged;
ThemeListener.ThemeChanged += ThemeListener_ThemeChanged;

This comment has been minimized.

Copy link
@azchohfi

azchohfi Oct 16, 2018

Contributor

Can't this leak? Shouldn't we do that on the OnApplyTemplate method? Btw, the PointerReleased over there is not being unassigned.

This comment has been minimized.

Copy link
@XamlBrewer

XamlBrewer Oct 17, 2018

Author Collaborator

Agree on moving the registration to OnApplyTemplate (since the handler forces a redraw). Will also add an UnLoaded handler to unregister all handlers.

XamlBrewer and others added some commits Oct 18, 2018

@nmetulev nmetulev merged commit 69dae5a into windows-toolkit:master Oct 22, 2018

3 checks passed

Toolkit-CI Build #20181022.14 succeeded
Details
WIP ready for review
Details
license/cla All CLA requirements met.
Details
@JerryNixon

This comment has been minimized.

Copy link

commented Oct 23, 2018

Worth the effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.