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

fix(multiple): fix VoiceOver confused by Select/Autocomplete's ARIA semantics #26861

Merged
merged 1 commit into from Apr 25, 2023

Conversation

zarend
Copy link
Contributor

@zarend zarend commented Mar 29, 2023

For Select and Autcomplete components, fix issues where VoiceOver was confused
by the ARIA semantics of the combobox. Fix multiple behaviors:

  • Fix VoiceOver focus ring stuck on the combobox while navigating
    options.
  • Fix VoiceOver would sometimes reading option as a TextNode and not
    communicating the selected state and position in set.
  • Fix VoiceOver "flickering" behavior where VoiceOver would display one
    announcement then quickly change to another annoucement.

Fix the same issues for both Select and Autocomplete component.

Implement fix by correcting the combobox element and also invidual
options.

First, move the aria-owns reference to the overlay from the child of the
combobox to the parent modal of the comobobx. Having an aria-owns
reference inside the combobox element seemed to confuse VoiceOver.

Second, apply aria-hidden="true" to the ripple element and pseudo
checkboxes on mat-option. These DOM nodes are only used for visual
purposes, so it is most appropriate to remove them from the
accessibility tree. This seemed to make VoiceOver's behavior more
consistent.

Fix #23202, #19798

@zarend zarend added P2 The issue is important to a large percentage of users, with a workaround Accessibility This issue is related to accessibility (a11y) area: material/autocomplete area: material/select dev-app preview When applied, previews of the dev-app are deployed to Firebase labels Mar 29, 2023
@github-actions
Copy link

github-actions bot commented Mar 29, 2023

Deployed dev-app for b697571 to: https://ng-dev-previews-comp--pr-angular-components-26861-kaqryhf5.web.app

Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt.

@zarend zarend force-pushed the mat-select-aria-owns branch 4 times, most recently from 7033c3c to 86cc57e Compare March 30, 2023 17:33
@zarend zarend changed the title fix(material/select): fix VoiceOver confused by ARIA semantics fix(multiple): fix VoiceOver confused by Select/Autocomplete's ARIA semantics Mar 30, 2023
@zarend zarend marked this pull request as ready for review March 30, 2023 17:43
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.

(same comments for select as autocomplete)

src/cdk/a11y/live-announcer/live-announcer.ts Show resolved Hide resolved
src/material/autocomplete/autocomplete-trigger.ts Outdated Show resolved Hide resolved
src/material/autocomplete/autocomplete-trigger.ts Outdated Show resolved Hide resolved
Comment on lines 883 to 901
const modals = this._document.querySelectorAll(
'body > .cdk-overlay-container [aria-modal="true"]',
);
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't need all modals here, should we? I think we should theoretically only need the closest modal ancestor of the autocomplete trigger

const modal = this.element.nativeElement.closest('[aria-modal="true"]');

src/material/autocomplete/autocomplete-trigger.ts Outdated Show resolved Hide resolved
src/material/autocomplete/autocomplete-trigger.ts Outdated Show resolved Hide resolved

/** Clears the references to the listbox overlay element from any modals it was added to. */
private _clearFromModals() {
this._trackedModals.forEach(modal => {
Copy link
Member

@jelbourn jelbourn Mar 30, 2023

Choose a reason for hiding this comment

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

Prefer for to forEach
https://github.com/angular/angular/blob/main/docs/CODING_STANDARDS.md#iteration

(but if we only end up tracking one, you wouldn't need this anyway)

src/material/autocomplete/autocomplete-trigger.ts Outdated Show resolved Hide resolved
src/material/autocomplete/autocomplete-trigger.ts Outdated Show resolved Hide resolved
@jelbourn
Copy link
Member

It also occured to me that the code to add/remove an ID from an existing attribute string is very similar to what AriaDescriber does. We can probably refactor that bit out into a separate utility in cdk/a11y

@zarend zarend force-pushed the mat-select-aria-owns branch 6 times, most recently from 196a245 to d1ea562 Compare April 4, 2023 23:26
@zarend
Copy link
Contributor Author

zarend commented Apr 4, 2023

Still working on this PR. I noticed an issue that only happens when using VoiceOver with Firefox. It works fine when using Firefox without voiceover and it works fine on Chrome.

Pressing up/down keys does not move the active option. When the panel opens, <mat-select> defocuses and the <mat-option> focuses. The <mat-option> is not supposed to have browser focus. The combobox is supposed to always have DOM focus and the active option is referenced by aria-activedescendent. It looks like something about when the options are added to the DOM it causes a weird focus change. As far as I can tell, we're correctly implementing ARIA spec for combobox with listbox popup.

Work-around is to use VO + Up/Down, which work normally.

zarend added a commit to zarend/components that referenced this pull request Apr 6, 2023
Remove the tabindex attribute from MatOption. MatOption will no longer
set a tabindex. Tabindex is not needed since focus is managed on the
parent by setting `aria-activedescendant`.

Tabindex="-1" seems to be causing a problem in angular#26861 where VoiceOver
with Firefox moves DOM focus from the combobox to the option when
opening the listbox popup.
zarend added a commit to zarend/components that referenced this pull request Apr 6, 2023
Remove the tabindex attribute from MatOption. MatOption will no longer
set a tabindex. Tabindex is not needed since focus is managed on the
parent by setting `aria-activedescendant`.

Tabindex="-1" seems to be causing a problem in angular#26861 where VoiceOver
with Firefox moves DOM focus from the combobox to the option when
opening the listbox popup.
zarend added a commit to zarend/components that referenced this pull request Apr 6, 2023
Remove the tabindex attribute from MatOption. MatOption will no longer
set a tabindex. Tabindex is not needed since focus is managed on the
parent by setting `aria-activedescendant`.

Tabindex="-1" seems to be causing a problem in angular#26861 where VoiceOver
with Firefox moves DOM focus from the combobox to the option when
opening the listbox popup.
zarend added a commit to zarend/components that referenced this pull request Apr 11, 2023
Remove the tabindex attribute added to MatOption components. MatOption
does not need tabindex because the parent component manages focus by
setting `aria-activedescendant` attribute. Previously, MatOption set
tabindex but was also a referenced by aria-activedescendant. This was
not the correct ARIA semantics. Align closer to ARIA spec by using only
aria-activedescendant rather than both.

Tabindex="-1" seems to be causing a problem in angular#26861 where VoiceOver
with Firefox moves DOM focus from the combobox to the option when
opening the listbox popup.

Unblocks angular#26861.
@devversion devversion removed their request for review April 11, 2023 14:42
zarend added a commit to zarend/components that referenced this pull request Apr 11, 2023
Remove the tabindex attribute added to MatOption components. MatOption
does not need tabindex because the parent component manages focus by
setting `aria-activedescendant` attribute. Previously, MatOption set
tabindex but was also a referenced by aria-activedescendant. This was
not the correct ARIA semantics. Align closer to ARIA spec by using only
aria-activedescendant rather than both.

Tabindex="-1" seems to be causing a problem in angular#26861 where VoiceOver
with Firefox moves DOM focus from the combobox to the option when
opening the listbox popup.

Unblocks angular#26861.
zarend added a commit to zarend/components that referenced this pull request Apr 12, 2023
Remove the tabindex attribute added to MatOption components. MatOption
does not need tabindex because the parent component manages focus by
setting `aria-activedescendant` attribute. Previously, MatOption set
tabindex but was also a referenced by aria-activedescendant. This was
not the correct ARIA semantics. Align closer to ARIA spec by using only
aria-activedescendant rather than both.

Tabindex="-1" seems to be causing a problem in angular#26861 where VoiceOver
with Firefox moves DOM focus from the combobox to the option when
opening the listbox popup.

Unblocks angular#26861.
zarend added a commit to zarend/components that referenced this pull request Apr 13, 2023
Remove the tabindex attribute added to MatOption components. MatOption
does not need tabindex because the parent component manages focus by
setting `aria-activedescendant` attribute. Previously, MatOption set
tabindex but was also a referenced by aria-activedescendant. This was
not the correct ARIA semantics. Align closer to ARIA spec by using only
aria-activedescendant rather than both.

Tabindex="-1" seems to be causing a problem in angular#26861 where VoiceOver
with Firefox moves DOM focus from the combobox to the option when
opening the listbox popup.

Unblocks angular#26861.
zarend added a commit to zarend/components that referenced this pull request Apr 13, 2023
Remove the tabindex attribute added to MatOption components. MatOption
does not need tabindex because the parent component manages focus by
setting `aria-activedescendant` attribute. Previously, MatOption set
tabindex but was also a referenced by aria-activedescendant. This was
not the correct ARIA semantics. Align closer to ARIA spec by using only
aria-activedescendant rather than both.

Tabindex="-1" seems to be causing a problem in angular#26861 where VoiceOver
with Firefox moves DOM focus from the combobox to the option when
opening the listbox popup.

Unblocks angular#26861.
angular-robot bot pushed a commit that referenced this pull request Apr 14, 2023
Remove the tabindex attribute added to MatOption components. MatOption
does not need tabindex because the parent component manages focus by
setting `aria-activedescendant` attribute. Previously, MatOption set
tabindex but was also a referenced by aria-activedescendant. This was
not the correct ARIA semantics. Align closer to ARIA spec by using only
aria-activedescendant rather than both.

Tabindex="-1" seems to be causing a problem in #26861 where VoiceOver
with Firefox moves DOM focus from the combobox to the option when
opening the listbox popup.

Unblocks #26861.
angular-robot bot pushed a commit that referenced this pull request Apr 14, 2023
Remove the tabindex attribute added to MatOption components. MatOption
does not need tabindex because the parent component manages focus by
setting `aria-activedescendant` attribute. Previously, MatOption set
tabindex but was also a referenced by aria-activedescendant. This was
not the correct ARIA semantics. Align closer to ARIA spec by using only
aria-activedescendant rather than both.

Tabindex="-1" seems to be causing a problem in #26861 where VoiceOver
with Firefox moves DOM focus from the combobox to the option when
opening the listbox popup.

Unblocks #26861.

(cherry picked from commit 97410fa)
angular-robot bot pushed a commit that referenced this pull request Apr 14, 2023
Remove the tabindex attribute added to MatOption components. MatOption
does not need tabindex because the parent component manages focus by
setting `aria-activedescendant` attribute. Previously, MatOption set
tabindex but was also a referenced by aria-activedescendant. This was
not the correct ARIA semantics. Align closer to ARIA spec by using only
aria-activedescendant rather than both.

Tabindex="-1" seems to be causing a problem in #26861 where VoiceOver
with Firefox moves DOM focus from the combobox to the option when
opening the listbox popup.

Unblocks #26861.

(cherry picked from commit 97410fa)
@zarend zarend force-pushed the mat-select-aria-owns branch 4 times, most recently from 4c4146f to c3a0204 Compare April 18, 2023 03:56
src/material/core/option/option.html Outdated Show resolved Hide resolved
src/material/core/option/option.html Outdated Show resolved Hide resolved
@zarend zarend added the target: major This PR is targeted for the next major release label Apr 18, 2023
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.

This PR is currently tagged with major, but should it actually be minor?

Looks good, just a few last nits

Comment on lines +899 to +901
const modal = this._element.nativeElement.closest(
'body > .cdk-overlay-container [aria-modal="true"]',
);
Copy link
Member

Choose a reason for hiding this comment

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

Any reason this selector couldn't just be [aria-modal="true"] by itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm aligning this with how SnackBarContainer and LiveAnnouncer do it. They only intend to implement for Overlay component, but there's a TODO to consolidate the duplicated code and consider making it work more broadly.

http://github.com/angular/components/issues/26853

src/material/core/option/option.html Outdated Show resolved Hide resolved
src/material/core/option/option.html Outdated Show resolved Hide resolved
src/material/core/option/option.html Show resolved Hide resolved
src/material/core/option/option.html Outdated Show resolved Hide resolved
@zarend zarend added target: minor This PR is targeted for the next minor release and removed target: major This PR is targeted for the next major release labels Apr 19, 2023
@zarend
Copy link
Contributor Author

zarend commented Apr 19, 2023

Thanks for reviewing. This PR adds aria-reference.ts to public api. this PR doesn't actually remove or deprecate anything or make breaking changes.

Set the target to minor.

…emantics

For Select and Autcomplete components, fix issues where VoiceOver was confused
by the ARIA semantics of the combobox. Fix multiple behaviors:

 - Fix VoiceOver focus ring stuck on the combobox while navigating
   options.
 - Fix VoiceOver would sometimes reading option as a TextNode and not
   communicating the selected state and position in set.
 - Fix VoiceOver "flickering" behavior where VoiceOver would display one
   announcement then quickly change to another annoucement.

Fix the same issues for both Select and Autocomplete component.

Implement fix by correcting the combobox element and also invidual
options.

First, move the aria-owns reference to the overlay from the child of the
combobox to the parent modal of the comobobx. Having an aria-owns
reference inside the combobox element seemed to confuse VoiceOver.

Second, apply `aria-hidden="true"` to the ripple element and pseudo
checkboxes on mat-option. These DOM nodes are only used for visual
purposes, so it is most appropriate to remove them from the
accessibility tree. This seemed to make VoiceOver's behavior more
consistent.

Fix angular#23202
Fix angular#19798
@zarend zarend added the action: merge The PR is ready for merge by the caretaker label Apr 20, 2023
@angular-robot angular-robot bot merged commit 33a9543 into angular:main Apr 25, 2023
24 checks passed
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Accessibility This issue is related to accessibility (a11y) action: merge The PR is ready for merge by the caretaker area: material/autocomplete area: material/select dev-app preview When applied, previews of the dev-app are deployed to Firebase P2 The issue is important to a large percentage of users, with a workaround target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mac VoiceOver focus is stuck on initial option in multiple selection
3 participants