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

Splaktar · 2021-07-22T21:55:59Z

notify screen reader users that they have entered a dialog
previously only the focused element would be read
i.e. "Close Button Press Search plus Space to activate"
now the screen reader user gets the normal dialog behavior, which is to
read the dialog title, role, content, and then tell the user about the
focused element
- this matches the guidance here:
https://www.w3.org/TR/wai-aria-practices-1.1/examples/dialog-modal/dialog.html
Avoid opening multiple of the same dialog before animations complete by returning
the previous MatDialogRef
update tests to use different dialog components when they need to open multiple
dialogs quickly

This PR takes a different approach than PR #23085 which required adding flush() to tests to ensure that the animations had a chance to complete. This PR uses a more naive approach that only gives animations 300ms to complete and won't allow another dialog of the same type to open within that time. The a11y piece of both PRs is the same (only move the focus to the dialog once).

zarend · 2021-07-23T15:28:45Z

Kicked off an internal presubmit

src/components-examples/material/dialog/dialog-harness/dialog-harness-example.spec.ts

src/material/dialog/dialog.ts

crisbeto · 2021-07-23T18:20:27Z

src/material/dialog/dialog.ts

+   * @return true if a dialog was opened less than 300ms ago, false otherwise.
+   */
+  _shouldBlockAnotherDialogOpening(): boolean {
+    return new Date().getTime() - this._lastDialogOpenTime < 300;


Where does the 300ms number come from? Currently the dialog animation is 150ms.

Also I think that this is pretty fragile. A better approach would be to have an isAnimating flag on the dialog container. It should be straightforward to do, because we already have _onAnimationStart and _onAnimationDone callbacks.

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

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

I agree and I don't prefer this naive approach, but that's what past reviews and presubmits suggested that we go with.

In https://github.com/angular/components/pull/23085/files#diff-fcade7d193ab41c7b9e2b072af496bedcf5b73ee519e237e32f8f5e00328246dR81, I had _dialogAnimatingOpen in _MatDialogBase, but you are suggesting using an attribute on the <mat-dialog-container> DOM element instead?

Once focus has been moved, the keyboard events won't be a problem anymore because they'll fire from inside the dialog. We could have another flag like _hasCapturedFocus which is flipped inside the focusInitialElementWhenReady callback.

The _hasCapturedFocus would be on the first dialog's _MatDialogContainerBase. Then when the 2nd dialog open is called and we see a matching componentOrTemplateRef, we would find the matching DialogRef in this.openDialogs and check it's _containerInstance. _hasCapturedFocus?

If it's false, then we throw an error or return the DialogRef from the first dialog.

If it's true, then what? Are we thinking that it shouldn't be possible to get a duplicate open event in this case and we should go ahead with opening the second dialog (with the same componentOrTemplateRef)?

Correct. If focus made it into the dialog, then it shouldn't be possible to accidentally trigger another dialog from the same trigger.

Would this introduce a race condition that could lead to inconsistent behavior?

Focus is moved as a result of the animation finishing so I don't think there would be a race condition.

Splaktar · 2021-07-26T17:38:46Z

Kicked off an internal presubmit

@zarend how did this go?

zarend · 2021-07-26T18:03:04Z

Kicked off an internal presubmit

@zarend how did this go?

187 test failures 😳

Splaktar · 2021-07-26T18:08:22Z

Doh. 🤦🏼‍♂️

Splaktar · 2021-07-27T18:57:41Z

src/material/dialog/dialog-animations.ts

@@ -14,6 +14,8 @@ import {
  AnimationTriggerMetadata,
 } from '@angular/animations';

+export const _transitionTime = 150;


@crisbeto I didn't want to copy this value all over the place. Instead I defined this const here, but I'm not confident that this is the right way to do it in this repo. Can you provide some guidance here please?

Splaktar · 2021-07-27T19:36:15Z

src/material/dialog/dialog.ts

+    this._lastDialogOpenTime = new Date().getTime();
+    this._lastDialogOpenType = componentOrTemplateRef;
+    this._lastDialogRef = dialogRef;


I wonder if these should get set to null in ngOnDestroy()? But I guess that would require changing the types to accept null.

I don't get why we need to deal with time at all. What's stopping from setting an isAnimating flag on the dialog container and reading it instead? We already have all the event handlers in place and the dialog refs are already tracked in MatDialog.openDialogs.

That's what caused the need for all of the flushing in the other PR. But I guess you feel like this specific approach would avoid that?

I thought that the flushing was necessary, because we introduced a hard error if you didn't flush. Since now we return a dialog ref, the flushing should be necessary.

Confirmed, when I removed the throwing of the error, I was able to remove the flush(); calls from the other PR.

zarend · 2021-07-27T20:16:50Z

presubmit (internal)

zarend · 2021-07-28T16:17:08Z

presubmit (internal)

Test run (internal) Resulted in 94 test failures

Splaktar · 2021-08-02T18:18:51Z

So the recently presubmit failures are due to a case where the MatDialog is wrapped by another component and then reused all over a large app. This wrapper always uses the same ComponentRef every time, just with different content. With the changes in this PR, the same dialog contents are often returned when in fact they should be different - i.e. this approach of checking the componentOrTemplateRef against the previously used componentOrTemplateRef doesn't work in this case.

crisbeto

LGTM, although I think that we can iterate on it afterwards so that we don't have to check the time to know if an animation is running.

crisbeto · 2021-08-03T15:09:57Z

tools/public_api_guard/material/dialog.md

@@ -290,6 +291,9 @@ export class MatDialogTitle implements OnInit {
 // @public
 export function throwMatDialogContentAlreadyAttachedError(): void;

+// @public
+export const _transitionTime = 150;


Can we avoid exposing the publicly?

I would like to, that's kind of what I was trying to ask in #23228 (review).

- notify screen reader users that they have entered a dialog - previously only the focused element would be read i.e. "Close Button Press Search plus Space to activate" - now the screen reader user gets the normal dialog behavior, which is to read the dialog title, role, content, and then tell the user about the focused element - this matches the guidance here: https://www.w3.org/TR/wai-aria-practices-1.1/examples/dialog-modal/dialog.html - Avoid opening multiple of the same dialog before animations complete by returning the previous `MatDialogRef` - update tests to use different dialog components when they need to open multiple dialogs quickly Fixes angular#21840

Splaktar · 2021-08-09T18:24:45Z

Currently the preferred solution for this is in PR #23085.

angular-automatic-lock-bot · 2021-09-16T00:02:25Z

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

Splaktar requested review from jelbourn and zarend July 22, 2021 21:56

Splaktar self-assigned this Jul 22, 2021

google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 22, 2021

Splaktar mentioned this pull request Jul 22, 2021

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

Merged

Splaktar requested a review from crisbeto July 22, 2021 21:57

Splaktar added area: material/dialog G This is is related to a Google internal issue P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent Accessibility This issue is related to accessibility (a11y) labels Jul 22, 2021

Splaktar force-pushed the dialog-fix-opening-a11y branch from c1f361f to 92a5dce Compare July 22, 2021 22:41

Splaktar added the target: patch This PR is targeted for the next patch release label Jul 23, 2021

Splaktar marked this pull request as ready for review July 23, 2021 00:31

Splaktar requested review from andrewseguin, devversion and a team as code owners July 23, 2021 00:31

crisbeto reviewed Jul 23, 2021

View reviewed changes

Splaktar force-pushed the dialog-fix-opening-a11y branch 2 times, most recently from 641cb48 to 8f247b0 Compare July 27, 2021 18:55

Splaktar commented Jul 27, 2021

View reviewed changes

Splaktar force-pushed the dialog-fix-opening-a11y branch from 8f247b0 to 6cc3644 Compare July 27, 2021 19:53

Splaktar removed the target: patch This PR is targeted for the next patch release label Jul 27, 2021

Splaktar requested a review from amysorto August 2, 2021 18:10

Splaktar force-pushed the dialog-fix-opening-a11y branch from 6cc3644 to 78ea4b4 Compare August 2, 2021 18:15

Splaktar force-pushed the dialog-fix-opening-a11y branch from 78ea4b4 to 087ed2c Compare August 2, 2021 19:24

crisbeto approved these changes Aug 3, 2021

View reviewed changes

Splaktar force-pushed the dialog-fix-opening-a11y branch from 087ed2c to a891034 Compare August 6, 2021 19:23

Splaktar force-pushed the dialog-fix-opening-a11y branch from a891034 to 72bd6c6 Compare August 6, 2021 19:45

Splaktar marked this pull request as draft August 9, 2021 18:24

josephperrott removed the request for review from a team August 10, 2021 18:38

Splaktar closed this Aug 16, 2021

Splaktar deleted the dialog-fix-opening-a11y branch August 16, 2021 17:39

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Splaktar commented Jul 22, 2021 •

edited

Loading

zarend commented Jul 23, 2021

crisbeto Jul 23, 2021

Splaktar Jul 23, 2021

Splaktar Jul 23, 2021

crisbeto Jul 23, 2021

Splaktar Jul 23, 2021

crisbeto Jul 23, 2021

Splaktar Jul 23, 2021

crisbeto Jul 24, 2021

Splaktar commented Jul 26, 2021

zarend commented Jul 26, 2021

Splaktar commented Jul 26, 2021

Splaktar Jul 27, 2021

Splaktar Jul 27, 2021

crisbeto Jul 27, 2021

Splaktar Jul 28, 2021

crisbeto Jul 28, 2021

Splaktar Aug 2, 2021

zarend commented Jul 27, 2021

zarend commented Jul 28, 2021

Splaktar commented Aug 2, 2021

crisbeto left a comment

crisbeto Aug 3, 2021

Splaktar Aug 3, 2021

Splaktar commented Aug 9, 2021

angular-automatic-lock-bot bot commented Sep 16, 2021

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

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

Conversation

Splaktar commented Jul 22, 2021 • edited Loading

zarend commented Jul 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Splaktar commented Jul 26, 2021

zarend commented Jul 26, 2021

Splaktar commented Jul 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zarend commented Jul 27, 2021

zarend commented Jul 28, 2021

Splaktar commented Aug 2, 2021

crisbeto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Splaktar commented Aug 9, 2021

angular-automatic-lock-bot bot commented Sep 16, 2021

Splaktar commented Jul 22, 2021 •

edited

Loading