Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

fix(select): overhaul screen reader support #11761

Merged
merged 1 commit into from
Mar 10, 2020
Merged

Conversation

Splaktar
Copy link
Member

@Splaktar Splaktar commented Jul 15, 2019

PR Checklist

Please check that your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added or this is not a bug fix / enhancement
  • Docs have been added, updated, or were not required

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Enhancement
[ ] Documentation content changes
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

  • md-select does not work well with VoiceOver on iOS or macOS
  • When using md-select with the multiple attribute, options are always "checked" because of ngAria.

Issue Number:
Fixes #10748. Fixes #10967.

What is the new behavior?

  • move to WAI-ARIA's Collapsible Dropdown Listbox practice
    • don't apply aria-required to md-select
    • as it isn't compatible with the button role
    • the md-content element is now the listbox
      • has the appropriate attributes and a unique id
      • and receives focus when the pop-up panel opens
      • aria-owns now points to this listbox so that indexes work
    • option focus is handled via aria-activedescendant
    • remove aria-expanded when collapsed
    • remove aria-disabled attribute when not disabled
  • manually remove aria-checked set by ngAria due to ngValue usage
  • apply md-focused class to the option with focus
  • improve ng-multiple implementation
    • account for multiple attribute on md-select-menu
  • remove unused deregisterCollectionWatch()
  • fix overloaded variable names
  • don't set aria-selected="false" on options in single selection mode
  • stop labels and values from being announced multiple times
  • add JSDoc/Closure Compiler details and types
  • refinements for VoiceOver users
  • clean up watchers, observers, and event handlers on $destroy
  • fix a case where the initial selection model could contain two values
    • for the empty option, i.e. "" and "None"
    • deselection was only clearing the first one in single selection mode
  • reduce duplicated code for focusing option nodes
  • improve keyboard option scrolling behavior
  • eliminate duplicate call to autoFocus()

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Completed Testing and Issues Identified

  • Finalize testing on VoiceOver in Chrome
  • Finalize testing on VoiceOver in Safari
  • Finalize testing on NVDA
  • Escape doesn't set the model back to the previous value
  • User defined aria-labelledby gets overwritten
  • aria-hidden on md-select-value is cleared when pop-up is closed, resulting in label being read twice when the md-select has no selection
  • Test in VoiceOver for iOS on Safari
    • Test with VoiceOver rotor
      • This seems to be working well as using the swipe right/left now cycles through the options as intended.
      • It should be noted that md-select elements show up under the Buttons Rotor instead of Form Controls. This seems to be working as intended.
    • Test exiting the pop-up panel in multiple selection mode
      • This issue with being difficult to exit a multiple selection select exists with Angular Material as well. The "Two finger scrub" to dismiss alerts/dialogs or go back a page seems to always go back a page when used with AngularJS/Angular Material Autocomplete and Select. I tried changing the md-select-menu-container to role="dialog" and adding "aria-haspopup="dialog" to the md-select, but that didn't help and the Z (two finger scrub) always went back to the previous page. This is being tracked in select(multiple): bad UX for exiting pop-up panel on iOS #11791.
  • Test on ChromeVoX
    • ChromeVox doesn't handle aria-lablelledby well and reads the md-select-value first and then the aria-label followed by the md-select-value again. Ideally, the label should be read and then the value. This will need to be handled in separate issue after consulting with the a11y team and possibly opening a bug against ChromeVox.
  • Autocomplete options' hover styling doesn't extend fully to the left edge
  • Test with TalkBack on Chrome for Android. I tested and saw similar behavior on Firefox for Android as well.
    • Single selection appears to be working well. "Checked" is no longer announced in single selection mode.
    • Multiple selection can be dismissed by a separated two finger tap.
    • Options in multiple selection cannot be selected (checked). This works for Angular Material, but not here. This isn't a regression as this has always been a problem with md-select on Android (back at least to 1.1.5). This is being tracked in select: options in multiple selection cannot be selected when using TalkBack #11770. This is likely a bug with TalkBack. Need to try to reproduce with a WAI-ARIA example and then find out where to submit the defect against TalkBack or Chrome for Android.

@Splaktar Splaktar added type: bug a11y This issue is related to accessibility g3: reported The issue was reported by an internal or external product team. P1: urgent Urgent issues that should be addressed in the next minor or patch release. g3: sync labels Jul 15, 2019
@Splaktar Splaktar added this to the 1.1.20 milestone Jul 15, 2019
@Splaktar Splaktar self-assigned this Jul 15, 2019
@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Jul 15, 2019
@Splaktar Splaktar force-pushed the select-improveA11y branch from afd9170 to 2d863a7 Compare July 24, 2019 04:56
@Splaktar Splaktar changed the title fix(select): improve screen reader support fix(select): overhaul screen reader support Jul 24, 2019
@Splaktar Splaktar force-pushed the select-improveA11y branch 2 times, most recently from b7f481c to 0e1691e Compare July 29, 2019 03:23
@Splaktar Splaktar added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Jul 29, 2019
@Splaktar Splaktar force-pushed the select-improveA11y branch 4 times, most recently from 521261d to 4694e08 Compare August 5, 2019 05:46
@Splaktar Splaktar requested a review from jelbourn August 5, 2019 05:47
@Splaktar Splaktar marked this pull request as ready for review August 5, 2019 05:47
@Splaktar Splaktar added pr: merge ready This PR is ready for a caretaker to review and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Aug 7, 2019
@Splaktar Splaktar force-pushed the select-improveA11y branch from 4694e08 to 178a040 Compare August 12, 2019 22:31
@mmalerba mmalerba added P2: required Issues that must be fixed. and removed P1: urgent Urgent issues that should be addressed in the next minor or patch release. labels Aug 13, 2019
@mmalerba
Copy link
Contributor

This has ~50 targets failing in g3, will need to look into it more when I get a chance

@Splaktar
Copy link
Member Author

Splaktar commented Sep 7, 2019

Rebased.

- move to [WAI-ARIA's Collapsible Dropdown Listbox practice](https://www.w3.org/TR/2019/NOTE-wai-aria-practices-1.1-20190207/examples/listbox/listbox-collapsible.html)
  - don't apply aria-required to md-select
  - as it isn't compatible with the button role
  - the md-content element is now the listbox
      - has the appropriate attributes and a unique id
      - and receives focus when the pop-up panel opens
      - aria-owns now points to this listbox so that indexes work
  - option focus is handled via `aria-activedescendant`
  - remove `aria-expanded` when collapsed
  - remove `aria-disabled` attribute when not disabled
-  manually remove `aria-checked` set by ngAria due to ngValue usage
-  apply `md-focused` class to the option with focus
- improve `ng-multiple` implementation
  - account for `multiple` attribute on `md-select-menu`
- remove unused `deregisterCollectionWatch()`
- fix overloaded variable names
- don't set aria-selected="false" on options in single selection mode
- stop labels and values from being announced multiple times
- add JSDoc/Closure Compiler details and types
- refinements for VoiceOver users
- clean up watchers, observers, and event handlers on $destroy
- fix a case where the initial selection model could contain two values
  - for the empty option, i.e. "" and "None"
  - deselection was only clearing the first one in single selection mode
- reduce duplicated code for focusing option nodes
- improve keyboard option scrolling behavior
- eliminate duplicate call to `autoFocus()`
- fix docs css to not interfere with autocomplete suggestion styling

Fixes #10748. Fixes #10967.
@Splaktar Splaktar force-pushed the select-improveA11y branch from b10b7fe to bb3f9f5 Compare March 6, 2020 10:15
@Splaktar
Copy link
Member Author

Splaktar commented Mar 6, 2020

Rebased.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a11y This issue is related to accessibility cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ g3: reported The issue was reported by an internal or external product team. P2: required Issues that must be fixed. pr: merge ready This PR is ready for a caretaker to review type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

select: does not work well with VoiceOver select(multiple): aria-checked and aria-selected broken
5 participants