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(material/button-toggle): use radio pattern for single select Mat toggle button group #28548

Merged
merged 1 commit into from
Mar 28, 2024

Conversation

clamli
Copy link
Contributor

@clamli clamli commented Feb 6, 2024

This commit updates the single-select Mat toggle button group to use radio pattern. This way the exclusiveness of the selection can be conveyed to the screen reader users.

@clamli clamli requested a review from andrewseguin as a code owner February 6, 2024 18:42
@andrewseguin andrewseguin requested a review from zarend February 6, 2024 23:12
@clamli clamli force-pushed the mat-button-toggle-2 branch 5 times, most recently from f79a32e to 0a923f3 Compare February 8, 2024 18:34
@clamli clamli marked this pull request as draft February 9, 2024 18:20
@clamli clamli marked this pull request as ready for review February 9, 2024 18:21
@clamli clamli marked this pull request as draft February 9, 2024 18:21
@clamli clamli marked this pull request as ready for review February 9, 2024 21:19
src/material/button-toggle/button-toggle.ts Outdated Show resolved Hide resolved
src/material/button-toggle/button-toggle.ts Outdated Show resolved Hide resolved
src/material/button-toggle/button-toggle.ts Outdated Show resolved Hide resolved
@clamli clamli force-pushed the mat-button-toggle-2 branch from 0a923f3 to 5588c4f Compare February 15, 2024 19:36
@clamli
Copy link
Contributor Author

clamli commented Feb 15, 2024

I added some unit test around the tab index update.

@zarend zarend requested a review from crisbeto March 14, 2024 01:30
@clamli clamli force-pushed the mat-button-toggle-2 branch from 5588c4f to fb07270 Compare March 15, 2024 17:54
@clamli clamli force-pushed the mat-button-toggle-2 branch 2 times, most recently from 1b2b465 to 14d8591 Compare March 15, 2024 20:54
@clamli clamli force-pushed the mat-button-toggle-2 branch 2 times, most recently from f037df4 to 4c914c3 Compare March 21, 2024 16:20
Copy link
Contributor

@zarend zarend left a comment

Choose a reason for hiding this comment

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

LGTM, good work

@zarend zarend added Accessibility This issue is related to accessibility (a11y) target: minor This PR is targeted for the next minor release labels Mar 21, 2024
@zarend zarend added action: merge The PR is ready for merge by the caretaker dev-app preview When applied, previews of the dev-app are deployed to Firebase labels Mar 21, 2024
Copy link

github-actions bot commented Mar 21, 2024

Deployed dev-app for 657939d to: https://ng-dev-previews-comp--pr-angular-components-28548-dev-p16tl3gz.web.app

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

@clamli clamli force-pushed the mat-button-toggle-2 branch 2 times, most recently from c75dcc6 to b67aeb1 Compare March 22, 2024 19:10
@zarend
Copy link
Contributor

zarend commented Mar 27, 2024

On ChromeOS using screen reader, I'm not able to navigate to the button toggle component with the "Next Form Field" navigation feature. On my system that's Search + F. ChromeOS navigates past the single-select button toggle to the first button on a multiple-select button toggle. Pressing Shift + Search + F takes me in the reverse direction and puts focus on the first checkbox before the single-select button toggles. Tested on deployed dev-app. I'm surprised that Screen Reader doesn't seem to be aware that mat button toggle is a form field. We're using radiogroup role. I can look at this more tomorrow.

@zarend
Copy link
Contributor

zarend commented Mar 28, 2024

"Next Form Field" seems to be working for me on VoiceOver, JAWS, NVDA. This is likely a ChromeOS specific problem. Action item to file issue report on ChromeOS Chromevox.

@zarend
Copy link
Contributor

zarend commented Mar 28, 2024

I can reproduce the error with MatRadioGroup demos. I can repro both on this branch and on docs site

Action item to file issue report on ChromeOS. Filing an issue report does not block landing this PR.

@clamli clamli force-pushed the mat-button-toggle-2 branch from b67aeb1 to 657939d Compare March 28, 2024 19:28
@zarend zarend removed the request for review from andrewseguin March 28, 2024 23:20
@zarend zarend merged commit 49901c6 into angular:main Mar 28, 2024
22 of 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 Apr 28, 2024
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 dev-app preview When applied, previews of the dev-app are deployed to Firebase target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants