Skip to content

fix(material/dialog): improve screen reader support when opened #23085

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

Merged
merged 1 commit into from
Aug 13, 2021
Merged

fix(material/dialog): improve screen reader support when opened #23085

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions src/material-experimental/mdc-dialog/dialog.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -774,14 +774,14 @@ describe('MDC-based MatDialog', () => {
.withContext('Expected reference to have been cleared.').toBeFalsy();
}));

it('should assign a unique id to each dialog', () => {
it('should assign a unique id to each dialog', fakeAsync(() => {
const one = dialog.open(PizzaMsg);
const two = dialog.open(PizzaMsg);

expect(one.id).toBeTruthy();
expect(two.id).toBeTruthy();
expect(one.id).not.toBe(two.id);
});
}));

it('should allow for the id to be overwritten', () => {
const dialogRef = dialog.open(PizzaMsg, {id: 'pizza'});
Expand Down Expand Up @@ -1200,7 +1200,7 @@ describe('MDC-based MatDialog', () => {
expect(document.activeElement!.id)
.not.toBe(
'dialog-trigger',
'Expcted the focus not to have changed before the animation finishes.');
'Expected the focus not to have changed before the animation finishes.');

flushMicrotasks();
viewContainerFixture.detectChanges();
Expand Down Expand Up @@ -1929,6 +1929,14 @@ describe('MDC-based MatDialog with animations enabled', () => {
flush();
expect(dialogRef.getState()).toBe(MatDialogState.CLOSED);
}));

it('should return the previous dialogRef if the previous dialog hasn\'t finished animating open',
() => {
let dialogRef1: MatDialogRef<PizzaMsg>, dialogRef2: MatDialogRef<PizzaMsg>;
dialogRef1 = dialog.open(PizzaMsg);
dialogRef2 = dialog.open(PizzaMsg);
expect(dialogRef1).toEqual(dialogRef2);
});
});

@Directive({selector: 'dir-with-view-container'})
Expand Down
7 changes: 5 additions & 2 deletions src/material-experimental/mdc-dialog/dialog.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {Inject, Injectable, InjectionToken, Injector, Optional, SkipSelf} from '
import {_MatDialogBase, MatDialogConfig} from '@angular/material/dialog';
import {MatDialogContainer} from './dialog-container';
import {MatDialogRef} from './dialog-ref';
import {ANIMATION_MODULE_TYPE} from '@angular/platform-browser/animations';

/** Injection token that can be used to access the data that was passed in to a dialog. */
export const MAT_DIALOG_DATA = new InjectionToken<any>('MatMdcDialogData');
Expand Down Expand Up @@ -52,9 +53,11 @@ export class MatDialog extends _MatDialogBase<MatDialogContainer> {
@Optional() location: Location,
@Optional() @Inject(MAT_DIALOG_DEFAULT_OPTIONS) defaultOptions: MatDialogConfig,
@Inject(MAT_DIALOG_SCROLL_STRATEGY) scrollStrategy: any,
@Optional() @SkipSelf() parentDialog: MatDialog, overlayContainer: OverlayContainer) {
@Optional() @SkipSelf() parentDialog: MatDialog, overlayContainer: OverlayContainer,
@Optional() @Inject(ANIMATION_MODULE_TYPE) animationMode?: 'NoopAnimations'|
'BrowserAnimations') {
super(
overlay, injector, defaultOptions, parentDialog, overlayContainer, scrollStrategy,
MatDialogRef, MatDialogContainer, MAT_DIALOG_DATA);
MatDialogRef, MatDialogContainer, MAT_DIALOG_DATA, animationMode);
}
}
11 changes: 7 additions & 4 deletions src/material/dialog/dialog-container.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,6 @@ export abstract class _MatDialogContainerBase extends BasePortalOutlet {
// Save the previously focused element. This element will be re-focused
// when the dialog closes.
this._capturePreviouslyFocusedElement();
// Move focus onto the dialog immediately in order to prevent the user
// from accidentally opening multiple dialogs at the same time.
this._focusDialogContainer();
}

/**
Expand Down Expand Up @@ -218,7 +215,13 @@ export abstract class _MatDialogContainerBase extends BasePortalOutlet {
break;
case true:
case 'first-tabbable':
this._focusTrap.focusInitialElementWhenReady();
this._focusTrap.focusInitialElementWhenReady().then(focusedSuccessfully => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Im not too sure if this can have a target of patch. This is touching code from #22780, which will go into the next minor release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, does sidenav or bottom sheet need special handling too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amysorto Special handling when opening to support screen readers? Probably not, unless they did the bouncing of focus that dialog did prior to this PR.

Copy link
Member

Choose a reason for hiding this comment

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

We should double-check that they don't

Copy link
Contributor Author

@Splaktar Splaktar Aug 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

  • Check that bottom sheet behaves properly in a screen reader when opened
  • Check that sidenav behaves properly in a screen reader when opened

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zarend found some similar issues with Bottom Sheet and he's investigating them now.

// If we weren't able to find a focusable element in the dialog, then focus the dialog
// container instead.
if (!focusedSuccessfully) {
this._focusDialogContainer();
}
});
break;
case 'first-heading':
this._focusByCssSelector('h1, h2, h3, h4, h5, h6, [role="heading"]');
Expand Down
27 changes: 20 additions & 7 deletions src/material/dialog/dialog.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1334,7 +1334,7 @@ describe('MatDialog', () => {

tick(500);
viewContainerFixture.detectChanges();
expect(lastFocusOrigin!).withContext('Expected the trigger button to be blurred').toBeNull();
expect(lastFocusOrigin!).toBe('program');

dispatchKeyboardEvent(document.body, 'keydown', ESCAPE);

Expand Down Expand Up @@ -1367,7 +1367,7 @@ describe('MatDialog', () => {

tick(500);
viewContainerFixture.detectChanges();
expect(lastFocusOrigin!).withContext('Expected the trigger button to be blurred').toBeNull();
expect(lastFocusOrigin!).toBe('program');

const backdrop = overlayContainerElement
.querySelector('.cdk-overlay-backdrop') as HTMLElement;
Expand Down Expand Up @@ -1398,12 +1398,18 @@ describe('MatDialog', () => {
// Patch the element focus after the initial and real focus, because otherwise the
// `activeElement` won't be set, and the dialog won't be able to restore focus to an element.
patchElementFocus(button);
expect(lastFocusOrigin!)
.withContext('Expected the trigger button to be focused via program')
.toBe('program');

dialog.open(ContentElementDialog, {viewContainerRef: testViewContainerRef});

tick(500);
viewContainerFixture.detectChanges();
expect(lastFocusOrigin!).withContext('Expected the trigger button to be blurred').toBeNull();
tick(500);
expect(lastFocusOrigin!)
.withContext('Expected the trigger button to be blurred')
.toBeNull();

const closeButton = overlayContainerElement
.querySelector('button[mat-dialog-close]') as HTMLElement;
Expand All @@ -1416,7 +1422,8 @@ describe('MatDialog', () => {
tick(500);

expect(lastFocusOrigin!)
.withContext('Expected the trigger button to be focused via keyboard').toBe('keyboard');
.withContext( 'Expected the trigger button to be focused via keyboard')
.toBe('keyboard');

focusMonitor.stopMonitoring(button);
document.body.removeChild(button);
Expand All @@ -1435,12 +1442,17 @@ describe('MatDialog', () => {
// Patch the element focus after the initial and real focus, because otherwise the
// `activeElement` won't be set, and the dialog won't be able to restore focus to an element.
patchElementFocus(button);
expect(lastFocusOrigin!)
.withContext('Expected the trigger button to be focused via program')
.toBe('program');

dialog.open(ContentElementDialog, {viewContainerRef: testViewContainerRef});

tick(500);
viewContainerFixture.detectChanges();
expect(lastFocusOrigin!).withContext('Expected the trigger button to be blurred').toBeNull();
tick(500);
expect(lastFocusOrigin!)
.withContext('Expected the trigger button to be blurred')
.toBeNull();

const closeButton = overlayContainerElement
.querySelector('button[mat-dialog-close]') as HTMLElement;
Expand All @@ -1454,7 +1466,8 @@ describe('MatDialog', () => {
tick(500);

expect(lastFocusOrigin!)
.withContext('Expected the trigger button to be focused via mouse').toBe('mouse');
.withContext( 'Expected the trigger button to be focused via mouse')
.toBe('mouse');

focusMonitor.stopMonitoring(button);
document.body.removeChild(button);
Expand Down
46 changes: 39 additions & 7 deletions src/material/dialog/dialog.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,12 @@ import {
TemplateRef,
Type,
} from '@angular/core';
import {defer, Observable, of as observableOf, Subject} from 'rxjs';
import {defer, Observable, of as observableOf, Subject, Subscription} from 'rxjs';
import {startWith} from 'rxjs/operators';
import {MatDialogConfig} from './dialog-config';
import {MatDialogContainer, _MatDialogContainerBase} from './dialog-container';
import {MatDialogRef} from './dialog-ref';
import {ANIMATION_MODULE_TYPE} from '@angular/platform-browser/animations';


/** Injection token that can be used to access the data that was passed in to a dialog. */
Expand Down Expand Up @@ -77,6 +78,9 @@ export abstract class _MatDialogBase<C extends _MatDialogContainerBase> implemen
private readonly _afterOpenedAtThisLevel = new Subject<MatDialogRef<any>>();
private _ariaHiddenElements = new Map<Element, string|null>();
private _scrollStrategy: () => ScrollStrategy;
private _dialogAnimatingOpen = false;
private _animationStateSubscriptions: Subscription;
private _lastDialogRef: MatDialogRef<any>;

/** Keeps track of the currently-open dialogs. */
get openDialogs(): MatDialogRef<any>[] {
Expand Down Expand Up @@ -111,7 +115,8 @@ export abstract class _MatDialogBase<C extends _MatDialogContainerBase> implemen
scrollStrategy: any,
private _dialogRefConstructor: Type<MatDialogRef<any>>,
private _dialogContainerType: Type<C>,
private _dialogDataToken: InjectionToken<any>) {
private _dialogDataToken: InjectionToken<any>,
private _animationMode?: 'NoopAnimations' | 'BrowserAnimations') {
this._scrollStrategy = scrollStrategy;
}

Expand Down Expand Up @@ -145,12 +150,35 @@ export abstract class _MatDialogBase<C extends _MatDialogContainerBase> implemen
throw Error(`Dialog with id "${config.id}" exists already. The dialog id must be unique.`);
}

// If there is a dialog that is currently animating open, return the MatDialogRef of that dialog
if (this._dialogAnimatingOpen) {
return this._lastDialogRef;
}

const overlayRef = this._createOverlay(config);
const dialogContainer = this._attachDialogContainer(overlayRef, config);
if (this._animationMode !== 'NoopAnimations') {
const animationStateSubscription =
dialogContainer._animationStateChanged.subscribe((dialogAnimationEvent) => {
if (dialogAnimationEvent.state === 'opening') {
this._dialogAnimatingOpen = true;
}
if (dialogAnimationEvent.state === 'opened') {
this._dialogAnimatingOpen = false;
animationStateSubscription.unsubscribe();
}
});
if (!this._animationStateSubscriptions) {
this._animationStateSubscriptions = new Subscription();
}
this._animationStateSubscriptions.add(animationStateSubscription);
}

const dialogRef = this._attachDialogContent<T, R>(componentOrTemplateRef,
dialogContainer,
overlayRef,
config);
this._lastDialogRef = dialogRef;

// If this is the first dialog that we're opening, hide all the non-overlay content.
if (!this.openDialogs.length) {
Expand Down Expand Up @@ -188,6 +216,10 @@ export abstract class _MatDialogBase<C extends _MatDialogContainerBase> implemen
this._closeDialogs(this._openDialogsAtThisLevel);
this._afterAllClosedAtThisLevel.complete();
this._afterOpenedAtThisLevel.complete();
// Clean up any subscriptions to dialogs that never finished opening.
if (this._animationStateSubscriptions) {
this._animationStateSubscriptions.unsubscribe();
}
}

/**
Expand Down Expand Up @@ -392,19 +424,19 @@ export abstract class _MatDialogBase<C extends _MatDialogContainerBase> implemen
@Injectable()
export class MatDialog extends _MatDialogBase<MatDialogContainer> {
constructor(
overlay: Overlay,
injector: Injector,
overlay: Overlay, injector: Injector,
/**
* @deprecated `_location` parameter to be removed.
* @breaking-change 10.0.0
*/
@Optional() location: Location,
@Optional() @Inject(MAT_DIALOG_DEFAULT_OPTIONS) defaultOptions: MatDialogConfig,
@Inject(MAT_DIALOG_SCROLL_STRATEGY) scrollStrategy: any,
@Optional() @SkipSelf() parentDialog: MatDialog,
overlayContainer: OverlayContainer) {
@Optional() @SkipSelf() parentDialog: MatDialog, overlayContainer: OverlayContainer,
@Optional() @Inject(ANIMATION_MODULE_TYPE) animationMode?: 'NoopAnimations'|
'BrowserAnimations') {
super(overlay, injector, defaultOptions, parentDialog, overlayContainer, scrollStrategy,
MatDialogRef, MatDialogContainer, MAT_DIALOG_DATA);
MatDialogRef, MatDialogContainer, MAT_DIALOG_DATA, animationMode);
}
}

Expand Down
6 changes: 3 additions & 3 deletions tools/public_api_guard/material/dialog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,9 @@ export function MAT_DIALOG_SCROLL_STRATEGY_PROVIDER_FACTORY(overlay: Overlay): (
// @public
export class MatDialog extends _MatDialogBase<MatDialogContainer> {
constructor(overlay: Overlay, injector: Injector,
location: Location_2, defaultOptions: MatDialogConfig, scrollStrategy: any, parentDialog: MatDialog, overlayContainer: OverlayContainer);
location: Location_2, defaultOptions: MatDialogConfig, scrollStrategy: any, parentDialog: MatDialog, overlayContainer: OverlayContainer, animationMode?: 'NoopAnimations' | 'BrowserAnimations');
// (undocumented)
static ɵfac: i0.ɵɵFactoryDeclaration<MatDialog, [null, null, { optional: true; }, { optional: true; }, null, { optional: true; skipSelf: true; }, null]>;
static ɵfac: i0.ɵɵFactoryDeclaration<MatDialog, [null, null, { optional: true; }, { optional: true; }, null, { optional: true; skipSelf: true; }, null, { optional: true; }]>;
// (undocumented)
static ɵprov: i0.ɵɵInjectableDeclaration<MatDialog>;
}
Expand All @@ -109,7 +109,7 @@ export const matDialogAnimations: {

// @public
export abstract class _MatDialogBase<C extends _MatDialogContainerBase> implements OnDestroy {
constructor(_overlay: Overlay, _injector: Injector, _defaultOptions: MatDialogConfig | undefined, _parentDialog: _MatDialogBase<C> | undefined, _overlayContainer: OverlayContainer, scrollStrategy: any, _dialogRefConstructor: Type<MatDialogRef<any>>, _dialogContainerType: Type<C>, _dialogDataToken: InjectionToken<any>);
constructor(_overlay: Overlay, _injector: Injector, _defaultOptions: MatDialogConfig | undefined, _parentDialog: _MatDialogBase<C> | undefined, _overlayContainer: OverlayContainer, scrollStrategy: any, _dialogRefConstructor: Type<MatDialogRef<any>>, _dialogContainerType: Type<C>, _dialogDataToken: InjectionToken<any>, _animationMode?: "NoopAnimations" | "BrowserAnimations" | undefined);
readonly afterAllClosed: Observable<void>;
get afterOpened(): Subject<MatDialogRef<any>>;
closeAll(): void;
Expand Down