Skip to content

fix(material/paginator): add role="group" to host #22512

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

Merged
merged 1 commit into from
May 4, 2021

Conversation

jelbourn
Copy link
Member

In testing <mat-paginator> with VoiceOver, I found that the interaction was much nicer when the paginator controls were semantically grouped with role="group". This change adds the role and updates the documentation to explicitly instruct users to apply a meaningful label.

I was on the fence about adding a default label (something like "Pagination control"). I settled on this approach because it's the most consistent with our other controls that require developers to specify a label for host elements, even though we do have MatPaginatorIntl for the labels for child components within the control.

@jelbourn jelbourn added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent Accessibility This issue is related to accessibility (a11y) area: material/paginator labels Apr 19, 2021
@jelbourn jelbourn requested review from zelliott and annieyw April 19, 2021 20:11
@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Apr 19, 2021
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM. Do we need a test for it?

In testing `<mat-paginator>` with VoiceOver, I found that the interaction was much nicer when the paginator controls were semantically grouped with `role="group"`. This change adds the role and updates the documentation to explicitly instruct users to apply a meaningful label.
@jelbourn jelbourn added the target: patch This PR is targeted for the next patch release label Apr 21, 2021
@jelbourn
Copy link
Member Author

Added a test

Caretaker note: when this is synced into Google, we should communicate the change to users

@jelbourn jelbourn added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note action: merge The PR is ready for merge by the caretaker labels Apr 21, 2021
@zelliott
Copy link
Collaborator

LGTM

@annieyw annieyw merged commit f06ff2a into angular:master May 4, 2021
annieyw pushed a commit that referenced this pull request May 4, 2021
In testing `<mat-paginator>` with VoiceOver, I found that the interaction was much nicer when the paginator controls were semantically grouped with `role="group"`. This change adds the role and updates the documentation to explicitly instruct users to apply a meaningful label.

(cherry picked from commit f06ff2a)
annieyw pushed a commit that referenced this pull request May 4, 2021
In testing `<mat-paginator>` with VoiceOver, I found that the interaction was much nicer when the paginator controls were semantically grouped with `role="group"`. This change adds the role and updates the documentation to explicitly instruct users to apply a meaningful label.

(cherry picked from commit f06ff2a)
@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 4, 2021
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/paginator cla: yes PR author has agreed to Google's Contributor License Agreement merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants