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

A variety of accessibility-related fixes for the contents of the main pane on the Config page #8682

Merged
merged 1 commit into from Feb 5, 2021

Conversation

guybark
Copy link
Contributor

@guybark guybark commented Dec 16, 2020

  1. Update app.config to pick up some .NET Framework accessibility fixes up to 4.8, depending on what version of .NET Framework is available on the device at runtime.

  2. Add unique accessible names for the Browse and Suggest buttons to avoid duplicate names being exposed programmatically on sibling buttons. (If the UI is ever localized, the accessible names must be localized too.)

  3. Change the order in which controls are added to their containers, in order to have the programmatic order (as exposed through the UI Automation API) of the controls match their logical order.

  4. Remove the TabIndex property being set on the controls, because that interferes with WinForms setting accessible names on TextBox and ComboBox controls based on the text from the Labels that programmatically precedes them. The default tab order is now based on the order in which the controls are added to their containers.

Before

The Accessibility Insights for Windows tool reports 31 failures against the UI, as shown in the following image

AIWinBefore

After

The Accessibility Insights for Windows tool reports 7 failures against the UI, as shown in the following image. Note that today the remaining failures cannot be avoided while building for .NET Framework.

AIWinAfter

@ghost ghost assigned guybark Dec 16, 2020
@codecov
Copy link

codecov bot commented Dec 16, 2020

Codecov Report

Merging #8682 (0718d74) into master (ad44c43) will increase coverage by 0.11%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #8682      +/-   ##
==========================================
+ Coverage   55.99%   56.11%   +0.11%     
==========================================
  Files         905      919      +14     
  Lines       65301    65518     +217     
  Branches    11882    11997     +115     
==========================================
+ Hits        36568    36764     +196     
+ Misses      25794    25760      -34     
- Partials     2939     2994      +55     
Flag Coverage Δ
production 43.26% <41.06%> (+0.11%) ⬆️
tests 94.45% <81.37%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@gerhardol gerhardol added the 📜 status: needs cla The author needs to update contributors.txt before reviewing or merging can proceed label Dec 16, 2020
@gerhardol
Copy link
Member

can you please sign off in contributors.txt?

This is just one form - is it possible to make an estimate of what it would take to apply to all?

@vbjay
Copy link
Contributor

vbjay commented Dec 16, 2020 via email

Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

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

LGTM, have not run it.

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

This is interesting, so we don't need tabindex to be accessible? Seems counter-intuitive...

@vbjay
Copy link
Contributor

vbjay commented Dec 16, 2020 via email

Copy link
Member

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

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

Tab order the same as before

//
// btnCommitTemplateBrowse
//
this.btnCommitTemplateBrowse.AccessibleName = "Browse Path to commit template";
Copy link
Member

Choose a reason for hiding this comment

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

These strings should probably be translated.
The translation app is not picking this up right now (this is the first use of AccessibleName in this project).
Short term, translation strings could be created separately (but how hard can it be to fix the translationapp, to make sure the build fails if not translated?)

Copy link
Member

Choose a reason for hiding this comment

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

AccessibleName (and AccessibleDescription) added to the translation app in #8713.
@guybark any comments on the items to translate?
#8713 should be merged soon, then this PR should be rebased on master and translation run once more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point, thanks for raising it. The AccessibleName properties needs to be as translated as visual text labels are. (So by default, translated, except for things like branding.) The AccessibleName is what gets announced by screen reader for customers who are blind or have low vision, so needs to be in a language that the customer wants. I hard-coded the strings because in the same file there seemed to be hard-coded Text properties, and I assumed the app wasn't localized. It sounds like I made the wrong assumption. If the AccessibleName properties won't be translated, the change should be canceled until it's possible to have the strings translated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding the removal of the use of TabIndex, WinForms apps are vulnerable to both the tab order and the programmatic order of the UI not being a good match for the use of the UI. The tab order is relatively straightforward to fix in VS through use of its "Tab Order" feature (in the View menu) but that doesn't fix the programmatic order. The only way to fix the programmatic order is to ensure that the calls to Add() in the designer.cs file match the logical order of the UI. The existing TabIndex use overrides that, so if the TabIndex use is removed the Add() order will dictate the tab order and programmatic order. If the form is later opened up in the VS designer, the TabIndex use will be added back but in an order that matches the Add() order, so the tab order and programmatic order remain good. A point was raised above that things may break later when controls are added or removed, and that's right. All WinForms apps are vulnerable, and the only way for things to work well for customers is for app builders to include the maintenance of the tab order and programmatic order in the list of things they do as their app evolves over time.

Copy link
Member

Choose a reason for hiding this comment

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

AccessibleName will be translated when you rebase, the TranslationApp will pick up this field and add it to the source English.xlf, translated on Transifex.
https://github.com/gitextensions/gitextensions/wiki/How-To%3A-build-instructions#update-english-translations

So for order: The easiest way around seem to be to just make sure that there is no TabIndex

@RussKie RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Jan 2, 2021
@gerhardol
Copy link
Member

@ghost ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Jan 12, 2021
@mstv mstv added 🖊️ status: cla signed and removed 📜 status: needs cla The author needs to update contributors.txt before reviewing or merging can proceed labels Jan 25, 2021
@mstv
Copy link
Member

mstv commented Jan 25, 2021

Could you give your signoff to the contributors list in a dedicated PR, please, in order to avoid merge conflicts by merging it quickly?
We want to avoid merges into feature branches. So rebase or cherry-pick the main commit and force-push this branch, please.

@RussKie
Copy link
Member

RussKie commented Jan 26, 2021

Thank you

1. Update app.config to pick up some .NET Framework accessibility fixes up to 4.8,
depending on what version of .NET Framework is available on the device at runtime.

2. Add unique accessible names for the Browse and Suggest buttons to avoid duplicate
names being exposed programmatically on sibling buttons. (If the UI is ever localized,
the accessible names must be localized too.)

3. Change the order in which controls are added to their containers, in order to have
the programmatic order (as exposed through the UI Automation API) of the controls match
their logical order.

4. Remove the TabIndex property being set on the controls, because that interferes with
WinForms setting accessible names on TextBox and ComboBox controls based on the text from
the Labels that programmatically precedes them. The default tab order is now based on the
order in which the controls are added to their containers.
@guybark
Copy link
Contributor Author

guybark commented Feb 5, 2021

I've run the built artifacts and verified that the changes are good. The image below shows the Accessibility Insights for Windows tool reporting that all the controls in the main area of this particular config UI have appropriate UIA Name properties, and they appear in the logical order in the UIA hierarchy. I manually verified that when tabbing through the controls, keyboard focus moves in the expected order.

GitExtensions_Config_UIAHierarchy

@RussKie RussKie merged commit f0e6f1b into gitextensions:master Feb 5, 2021
@ghost ghost added this to the 3.6 milestone Feb 5, 2021
@RussKie
Copy link
Member

RussKie commented Feb 5, 2021

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants