Skip to content

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

Merged
mmalerba merged 2 commits into
angular:masterfrom
annieyw:autcomplete-aria-label
Dec 3, 2020
Merged

fix(material/autocomplete): add missing aria-label for autocomplete panel#20892
mmalerba merged 2 commits into
angular:masterfrom
annieyw:autcomplete-aria-label

Conversation

@annieyw

@annieyw annieyw commented Oct 27, 2020

Copy link
Copy Markdown
Contributor

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

@crisbeto crisbeto left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/material/autocomplete/autocomplete.html Outdated
@annieyw
annieyw force-pushed the autcomplete-aria-label branch from e68c30a to dea2122 Compare October 28, 2020 20:44
@annieyw

annieyw commented Oct 28, 2020

Copy link
Copy Markdown
Contributor Author
  • 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
Comment thread src/material-experimental/mdc-autocomplete/autocomplete.ts Outdated
Comment thread src/material/autocomplete/autocomplete.ts Outdated
Comment thread src/material/autocomplete/autocomplete.ts Outdated
Comment thread src/material/autocomplete/autocomplete.ts Outdated
@annieyw
annieyw force-pushed the autcomplete-aria-label branch 2 times, most recently from 1f26c2f to 19dfec2 Compare October 28, 2020 21:30
@annieyw

annieyw commented Oct 28, 2020

Copy link
Copy Markdown
Contributor Author

Addressed comments.

crisbeto
crisbeto approved these changes Oct 28, 2020
View reviewed changes

@crisbeto crisbeto left a comment

Copy link
Copy Markdown
Member

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

annieyw commented Oct 29, 2020

Copy link
Copy Markdown
Contributor Author

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
Comment thread src/material/autocomplete/autocomplete-trigger.ts Outdated

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

@jelbourn jelbourn Nov 5, 2020

Copy link
Copy Markdown
Contributor

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
Comment thread 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"

@jimunoz1990 jimunoz1990 Nov 19, 2020

Copy link
Copy Markdown

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

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

Copy link
Copy Markdown

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

@crisbeto crisbeto crisbeto approved these changes
@mmalerba mmalerba Awaiting requested review from mmalerba
+2 more reviewers
@jelbourn jelbourn jelbourn left review comments
@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