Skip to content
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

fix(dialog): hide all non-overlay content from assistive technology #9016

Merged
merged 1 commit into from
Jan 4, 2018
Merged

fix(dialog): hide all non-overlay content from assistive technology #9016

merged 1 commit into from
Jan 4, 2018

Conversation

crisbeto
Copy link
Member

Hides all non-overlay content from assistive technology by applying aria-hidden to it. This prevents users from being able to move focus out of the dialog using the screen reader navigational shortcuts.

Fixes #7787.

@crisbeto crisbeto added Accessibility This issue is related to accessibility (a11y) pr: needs review labels Dec 16, 2017
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 16, 2017
@jelbourn jelbourn added the target: patch This PR is targeted for the next patch release label Dec 18, 2017
@jelbourn jelbourn mentioned this pull request Dec 18, 2017
Closed
jelbourn
jelbourn reviewed Dec 18, 2017
View reviewed changes
src/lib/dialog/dialog.ts
@@ -67,6 +69,7 @@ export class MatDialog {
private _openDialogsAtThisLevel: MatDialogRef<any>[] = [];
private _afterAllClosedAtThisLevel = new Subject<void>();
private _afterOpenAtThisLevel = new Subject<MatDialogRef<any>>();
private _ariaHiddenElements = new Map<Element, string|null>();
Copy link
Member

Choose a reason for hiding this comment

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

WeakMap as a precaution against memory leaks?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't use a WeakMap, because we need to be able to iterate over the elements.

src/lib/dialog/dialog.ts Outdated
/**
* Hides all of the content that isn't an overlay from assistive technology.
*/
private _hideContent() {
Copy link
Member

Choose a reason for hiding this comment

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

_hideNonDialogContentFromAssistiveTechnology?

src/lib/dialog/dialog.ts Outdated
const overlayContainer = this._overlayContainer.getContainerElement();
const siblings = overlayContainer.parentElement!.children;

for (let i = 0; i < siblings.length; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: prefer iteration from length - 1 to zero for HTMLCollection

src/lib/dialog/dialog.ts Outdated
let sibling = siblings[i];

if (sibling !== overlayContainer &&
sibling !== this._liveAnnouncer.liveElement &&
Copy link
Member

Choose a reason for hiding this comment

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

Instead of checking this element directly, just check that the element doesn't have aria-live? it will accomplish the same thing without needing to change the live announcer API and also safeguards against something that does its own live announcements.

@crisbeto
Copy link
Member Author

Addressed the feedback @jelbourn.

jelbourn
jelbourn approved these changes Dec 18, 2017
View reviewed changes
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, add merge ready when ready

src/lib/dialog/dialog.ts Outdated
/**
* Hides all of the content that isn't an overlay from assistive technology.
*/
private __hideNonDialogContentFromAssistiveTechnology() {
Copy link
Member

Choose a reason for hiding this comment

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

Extra _

@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Dec 18, 2017
@jelbourn jelbourn added the G This is is related to a Google internal issue label Dec 20, 2017
@josephperrott
Copy link
Member

We are seeing a number of presubmit failures from how overlays are tested.

A common pattern is to provide the OverlayContainer within a test as a newly created unattached div.

{
  provide: OverlayContainer,
  useFactory: () => {
    overlayContainerElement = document.createElement('div');
    return {getContainerElement: () => overlayContainerElement};
  }
}

This change assumes that the overlayContainer will have a parent. Is this a check we can do and only take action if a parentElement is found?

@josephperrott josephperrott added presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged and removed action: merge The PR is ready for merge by the caretaker labels Dec 21, 2017
@crisbeto
fix(dialog): hide all non-overlay content from assistive technology
873f708 
Hides all non-overlay content from assistive technology by applying `aria-hidden` to it. This prevents users from being able to move focus out of the dialog using the screen reader navigational shortcuts.

Fixes angular#7787.
@crisbeto
Copy link
Member Author

Sure, done @josephperrott.

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged labels Dec 21, 2017
@jelbourn jelbourn merged commit d82124d into angular:master Jan 4, 2018
jelbourn pushed a commit to jelbourn/components that referenced this pull request Jan 8, 2018
@crisbeto @jelbourn
fix(dialog): hide all non-overlay content from assistive technology (a…
ef06a9c 
…ngular#9016)

Hides all non-overlay content from assistive technology by applying `aria-hidden` to it. This prevents users from being able to move focus out of the dialog using the screen reader navigational shortcuts.

Fixes angular#7787.
jelbourn pushed a commit to jelbourn/components that referenced this pull request Jan 9, 2018
@crisbeto @jelbourn
fix(dialog): hide all non-overlay content from assistive technology (a…
5a800d7 
…ngular#9016)

Hides all non-overlay content from assistive technology by applying `aria-hidden` to it. This prevents users from being able to move focus out of the dialog using the screen reader navigational shortcuts.

Fixes angular#7787.
jelbourn pushed a commit that referenced this pull request Jan 9, 2018
@crisbeto @jelbourn
fix(dialog): hide all non-overlay content from assistive technology (#…
6b2a71e 
…9016)

Hides all non-overlay content from assistive technology by applying `aria-hidden` to it. This prevents users from being able to move focus out of the dialog using the screen reader navigational shortcuts.

Fixes #7787.
tinayuangao pushed a commit that referenced this pull request Jan 10, 2018
@crisbeto @tinayuangao
fix(dialog): hide all non-overlay content from assistive technology (#…
f110064 
…9016)

Hides all non-overlay content from assistive technology by applying `aria-hidden` to it. This prevents users from being able to move focus out of the dialog using the screen reader navigational shortcuts.

Fixes #7787.
@dgfranklin dgfranklin mentioned this pull request Mar 7, 2018
Open
@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 Sep 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn approved these changes

@devversion devversion Awaiting requested review from devversion

Assignees

@jelbourn jelbourn

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 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.

[Accessibility] MatDialog is trapping tab key but not headings navigation shortcuts
4 participants
@crisbeto @josephperrott @jelbourn @googlebot