Skip to content

fix(cdk/a11y): set AriaDescriber messages container to visibility:hidden - #21186

Merged
wagnermaciel merged 1 commit into
angular:masterfrom
annieyw:ariadescriber
Dec 10, 2020
Merged

fix(cdk/a11y): set AriaDescriber messages container to visibility:hidden#21186
wagnermaciel merged 1 commit into
angular:masterfrom
annieyw:ariadescriber

Conversation

@annieyw

@annieyw annieyw commented Dec 1, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Previously searching for AriaDescriber messages in the browser would result in a ctrl-f stop that was invisible. This is because the cdk-messages-container had the cdk-visually-hidden class but that is not enough to remove the ctrl-f stops.
This fix maintains functionality while preventing extra ctrl-f stops.

Tested with:
macOS/VoiceOver
Windows/NVDA

@google-cla google-cla Bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 1, 2020
@annieyw annieyw added G This is is related to a Google internal issue P4 A relatively minor issue that is not relevant to core functions target: patch This PR is targeted for the next patch release labels Dec 3, 2020
@annieyw
annieyw requested a review from jelbourn December 3, 2020 23:00
@annieyw
annieyw marked this pull request as ready for review December 3, 2020 23:00
@annieyw
annieyw requested a review from devversion as a code owner December 3, 2020 23:00
jelbourn
jelbourn reviewed Dec 4, 2020
View reviewed changes
Comment thread src/cdk/a11y/aria-describer/aria-describer.ts
Comment thread src/cdk/a11y/aria-describer/aria-describer.ts
jelbourn
jelbourn approved these changes Dec 4, 2020
View reviewed changes

@jelbourn jelbourn left a comment

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.

LGTM

@jelbourn jelbourn added action: merge The PR is ready for merge by the caretaker Accessibility This issue is related to accessibility (a11y) labels Dec 4, 2020
crisbeto
crisbeto reviewed Dec 5, 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.

The lint check is failing as well.

Comment thread src/cdk/a11y/aria-describer/aria-describer.ts
// when the description element is not visible.
messagesContainer.style.visibility = 'hidden';
// Even though we use `visibility: hidden`, we still apply `cdk-visually-hidden` so that
// the description element doesn't impact page layout.

@crisbeto crisbeto Dec 5, 2020

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.

You should be able to remove the messagesContainer.setAttribute('aria-hidden', canBeAriaHidden + ''); below since visibility: hidden hides the element from screen readers as well. I'm not sure whether the note about IE and Edge doesn't apply to visibility as well though.

Also the unit tests need to be updated to reflect the new changes.

@crisbeto crisbeto removed the action: merge The PR is ready for merge by the caretaker label Dec 5, 2020
crisbeto
crisbeto approved these changes Dec 7, 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. Can be marked as "merge ready" once the CI issue is fixed.

@annieyw annieyw added the action: merge The PR is ready for merge by the caretaker label Dec 7, 2020
@wagnermaciel wagnermaciel added target: major This PR is targeted for the next major release target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release target: major This PR is targeted for the next major release labels Dec 10, 2020
@wagnermaciel
wagnermaciel merged commit 2e7407c into angular:master Dec 10, 2020
@annieyw
annieyw deleted the ariadescriber branch December 10, 2020 23:07
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Jan 10, 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 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@crisbeto crisbeto crisbeto approved these changes
@devversion devversion Awaiting requested review from devversion
+1 more reviewer
@jelbourn jelbourn jelbourn approved these changes
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 P4 A relatively minor issue that is not relevant to core functions target: minor This PR is targeted for the next minor release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@annieyw @jelbourn @crisbeto @wagnermaciel