Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

Conversation

Splaktar
Copy link
Contributor

@Splaktar Splaktar commented Aug 15, 2018

PR Checklist

Please check that your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added or this is not a bug fix / enhancement
  • Docs have been added, updated, or were not required

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Enhancement
[ ] Documentation content changes
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

applying aria-labelledby to md-autocomplete has no effect because the attribute is not forwarded on to the input.

Issue Number:
Fixes #10815

What is the new behavior?

add support for input-aria-label and input-aria-labelledby which will apply those values to the input element of the autocomplete.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

It looks like the previous PR #11407 had a problem with https://github.com/GoogleChrome/accessibility-developer-tools/wiki/Audit-Rules#ax_text_01:

Error: AX_TEXT_01 (Controls and media elements should have labels) failed on the following element: #input-16

@Splaktar Splaktar added the a11y This issue is related to accessibility label Aug 15, 2018
@Splaktar Splaktar added this to the 1.1.11 milestone Aug 15, 2018
@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Aug 15, 2018
@Splaktar Splaktar added type: enhancement P4: minor Minor issues. May not be fixed without community contributions. in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs pr: merge ready This PR is ready for a caretaker to review and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Aug 15, 2018
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.

LGTM

@mmalerba mmalerba removed their assignment Aug 20, 2018
@Splaktar
Copy link
Contributor Author

@jelbourn I am actually going to change this once more after doing some further testing on a larger chips a11y PR. I think that applying input-aria-labelledby is more accurate. If we keep with just aria-labelledby, it seems to cause some duplication with screen readers.

I'll send over the chips PR for your initial review later today.

@Splaktar Splaktar force-pushed the autocompleteAriaLabelledBy branch from f4cc640 to 66d31ed Compare August 20, 2018 22:29
@Splaktar Splaktar changed the title feat(autocomplete): add support for aria-labelledby feat(autocomplete): add input-aria-label and input-aria-labelledby Aug 20, 2018
@Splaktar
Copy link
Contributor Author

@jelbourn OK, this PR is updated. It also adds an input-aria-label similar to md-chips. This is useful since the placeholder and label are not always the same.

This PR also updates aria-describedby to be input-aria-describedby for md-autocomplete.

It's possible that this could solve the a11y error that was seen by Miles' presubmit. However, the changes were made for general correctness after doing some additional screen reader testing with chips.

It's still possible to apply an aria-label to the md-autocomplete and then a different input-aria-label which would apply that string to the aria-label of the embedded input.

@Splaktar Splaktar added pr: lgtm This PR has been approved by the reviewer P3: important Important issues that really should be fixed when possible. pr: presubmit-failures and removed P4: minor Minor issues. May not be fixed without community contributions. labels Aug 24, 2018
@Splaktar
Copy link
Contributor Author

Splaktar commented Aug 30, 2018

Feedback on presubmit failures: This PR "is failing an accessibility check in one project where an element that formerly had a label now doesn't. Need to check that the change isn't overwriting an aria-label with nothing."

I'll try to reproduce this in a unit test.

  • check that existing aria-labels are not lost/overwritten with nothing

@Splaktar Splaktar added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs and removed pr: merge ready This PR is ready for a caretaker to review labels Aug 30, 2018
@Splaktar Splaktar force-pushed the autocompleteAriaLabelledBy branch from 66d31ed to 7c09bd9 Compare September 7, 2018 23:45
@Splaktar Splaktar added pr: merge ready This PR is ready for a caretaker to review and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs pr: presubmit-failures labels Sep 7, 2018
@Splaktar Splaktar assigned josephperrott and unassigned Splaktar Sep 7, 2018
@mmalerba
Copy link
Contributor

I'm seeing a couple of these errors:

Error: AX_TEXT_01 (Controls and media elements should have labels) failed on the following element:
#input-0
See https://github.com/GoogleChrome/accessibility-developer-tools/wiki/Audit-Rules#ax_text_01 for more information.

@Splaktar
Copy link
Contributor Author

@mmalerba I'll need more detailed information. As mentioned above, I've written up a lot of test cases for this. Please let me know the use case that is failing so that I can add another unit test that covers it.

@Splaktar Splaktar added Blocked Progress on this issue is blocked. Primarily used for PRs that are blocked by presubmit feedback. pr: presubmit-failures P1: urgent Urgent issues that should be addressed in the next minor or patch release. and removed P3: important Important issues that really should be fixed when possible. labels Sep 21, 2018
@Splaktar
Copy link
Contributor Author

Bumped this to P1 as getting this in is blocking some work on P1 issues.

@Splaktar Splaktar removed the pr: merge ready This PR is ready for a caretaker to review label Sep 21, 2018
@Splaktar Splaktar added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Sep 26, 2018
@Splaktar Splaktar force-pushed the autocompleteAriaLabelledBy branch from 7c09bd9 to de4d43c Compare October 17, 2018 00:38
@Splaktar
Copy link
Contributor Author

Rebased.

@Splaktar Splaktar removed the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Oct 25, 2018
@Splaktar Splaktar added the pr: merge ready This PR is ready for a caretaker to review label Nov 1, 2018
@Splaktar Splaktar force-pushed the autocompleteAriaLabelledBy branch from de4d43c to 8b32f18 Compare November 7, 2018 18:50
@Splaktar
Copy link
Contributor Author

Splaktar commented Nov 7, 2018

Rebased ✅

@Splaktar Splaktar removed Blocked Progress on this issue is blocked. Primarily used for PRs that are blocked by presubmit feedback. pr: presubmit-failures labels Nov 15, 2018
@Splaktar Splaktar assigned andrewseguin and unassigned jelbourn and mmalerba Nov 15, 2018
don't apply a duplicate aria-label and aria-labelledby to the same input
change aria-describedby to input-aria-describedby

Fixes #10815
@Splaktar Splaktar force-pushed the autocompleteAriaLabelledBy branch from 8b32f18 to 3fdafd0 Compare November 15, 2018 23:17
@Splaktar
Copy link
Contributor Author

Rebased ✔️

@andrewseguin andrewseguin merged commit 534beea into master Nov 15, 2018
@Splaktar Splaktar deleted the autocompleteAriaLabelledBy branch November 15, 2018 23:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a11y This issue is related to accessibility cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ P1: urgent Urgent issues that should be addressed in the next minor or patch release. pr: lgtm This PR has been approved by the reviewer pr: merge ready This PR is ready for a caretaker to review type: enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(autocomplete): add support for aria-labelledby
6 participants