Skip to content

fix(material/autocomplete): add missing aria-label for autocomplete panel #20892

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Dec 3, 2020
Merged

fix(material/autocomplete): add missing aria-label for autocomplete panel #20892

merged 2 commits into from
Dec 3, 2020

Conversation

@annieyw
Copy link
Contributor

@annieyw annieyw commented Oct 27, 2020

Fixes #20888

@annieyw annieyw requested review from jelbourn and mmalerba October 27, 2020 00:44
@annieyw annieyw requested a review from crisbeto as a code owner October 27, 2020 00:44
@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 27, 2020
crisbeto
crisbeto reviewed Oct 27, 2020
View reviewed changes
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.

We'll also have to apply the same fix to the MDC-based autocomplete and write a unit test or two to verify the fix.

@annieyw annieyw force-pushed the autcomplete-aria-label branch from e68c30a to dea2122 Compare October 28, 2020 20:44
@annieyw
Copy link
Contributor Author

annieyw commented Oct 28, 2020

  • Change implementation to take aria-label input or set aria-labelledby to trigger form field id.
  • Reflect changes in MDC
  • Add unit tests

@annieyw annieyw force-pushed the autcomplete-aria-label branch from dea2122 to 2d4bc9e Compare October 28, 2020 20:47
crisbeto
crisbeto reviewed Oct 28, 2020
View reviewed changes
@annieyw annieyw force-pushed the autcomplete-aria-label branch 2 times, most recently from 1f26c2f to 19dfec2 Compare October 28, 2020 21:30
@annieyw
Copy link
Contributor Author

annieyw commented Oct 28, 2020

Addressed comments.

crisbeto
crisbeto approved these changes Oct 28, 2020
View reviewed changes
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 once the CI failure has been fixed.

@annieyw
Copy link
Contributor Author

annieyw commented Oct 29, 2020

I think the CI failure is a previous commit message error

@annieyw annieyw added action: merge The PR is ready for merge by the caretaker P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release labels Oct 29, 2020
jelbourn
jelbourn reviewed Nov 5, 2020
View reviewed changes
src/material/autocomplete/autocomplete-trigger.ts Outdated

this.autocomplete._setVisibility();
this.autocomplete._isOpen = this._overlayAttached = true;
this.autocomplete._formFieldLabelId = this._formField?._labelId;
Copy link
Member

@jelbourn jelbourn Nov 5, 2020

Choose a reason for hiding this comment

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

@crisbeto we're seeing this cause a ChangedAfterChecked error in Google because change detection runs for the first time during the attach call above, but then this changes _formFieldLabelId (and thus the `aria-labelledby expression) in the check pass. I can think of a couple hacky ways to work around this, but nothing super clean. Any ideas?

@annieyw annieyw added Accessibility This issue is related to accessibility (a11y) G This is is related to a Google internal issue labels Nov 6, 2020
jimunoz1990
jimunoz1990 reviewed Nov 19, 2020
View reviewed changes
src/material/autocomplete/autocomplete.html Outdated
<ng-template>
<div class="mat-autocomplete-panel" role="listbox" [id]="id" [ngClass]="_classList" #panel>
<div class="mat-autocomplete-panel"
role="listbox" [id]="id"
Copy link

@jimunoz1990 jimunoz1990 Nov 19, 2020

Choose a reason for hiding this comment

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

nit: can this get split into 2 lines? 😄

@annieyw annieyw force-pushed the autcomplete-aria-label branch from 19dfec2 to 5d1bf98 Compare November 23, 2020 19:49
@annieyw annieyw force-pushed the autcomplete-aria-label branch from 5d1bf98 to 1371e31 Compare November 24, 2020 18:38
@mmalerba mmalerba merged commit 8ee56a5 into angular:master Dec 3, 2020
mmalerba pushed a commit that referenced this pull request Dec 3, 2020
@annieyw @mmalerba
fix(material/autocomplete): add missing aria-label for autocomplete p…
e878ca7 
…anel (#20892)

* fix(material/autocomplete): add missing aria-label for autocomplete panel

* fix(material/autocomplete): pass in form field id in ng-template

(cherry picked from commit 8ee56a5)
@angular-automatic-lock-bot
Copy link

angular-automatic-lock-bot bot commented Jan 3, 2021

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 Jan 3, 2021
@annieyw annieyw deleted the autcomplete-aria-label branch January 6, 2021 11:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jelbourn jelbourn jelbourn left review comments

@crisbeto crisbeto crisbeto approved these changes

@mmalerba mmalerba Awaiting requested review from mmalerba

+1 more reviewer

@jimunoz1990 jimunoz1990 jimunoz1990 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Accessibility This issue is related to accessibility (a11y) action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement G This is is related to a Google internal issue P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bug(mat-autocomplete): listbox missing label

5 participants

@annieyw @jelbourn @jimunoz1990 @crisbeto @mmalerba