fix(material/dialog): improve screen reader support when opened by Splaktar · Pull Request #23085 · angular/components

Splaktar · 2021-06-29T02:25:34Z

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 material/dialog API golden file

crisbeto

Also worth noting that the commit message mentions the MDC dialog, but all the changes are to the non-MDC one.

Splaktar · 2021-06-29T23:51:59Z

the commit message mentions the MDC dialog, but all the changes are to the non-MDC one.

@crisbeto I was only testing against the MDC-dialog, but they both use the same code for this. How should I write the commit message in that case? fix(multiple): or just use fix(material/dialog): even though it also effects mdc-dialog?

Splaktar · 2021-06-30T01:22:54Z

So after looking into this some more, here's what I've got to work with

_MatDialogBase is a "service" which holds all of the open dialogs and contains the open() method. It seems to be the right place to add a check to see if there is a dialog animating open before trying to open a new dialog.
- However, it does not currently care about whether a dialog that it is opening has animations or not.
MatDialogContainer is a Component that holds the _state of the dialog's animation and some private callback functions related to animation state. They emit the state via _MatDialogContainerBase._animationStateChanged. MatDialogContainer's host bindings hook up the state and callbacks to matDialogAnimations.
_MatDialogContainerBase is the abstract class extended by MatDialogContainer. It has _animationStateChanged = new EventEmitter<DialogAnimationEvent>(). This event holds the totalTime and the state ('opened' | 'opening' | 'closing' | 'closed').
- This class also contains _initializeWithAttachedContent() which is where we _setupFocusTrap(), _capturePreviouslyFocusedElement(), and _preventDuplicateDialogOpens(). However, by the time we've made it here, in _MatDialogBase we've already created the overlay, attached the dialog container, attached the dialog content, hidden non-dialog content from screen readers, pushed the dialog ref into the openDialogs array, subscribed to afterClosed(), and called afterOpened.next(dialogRef). We should really block a second attempt at opening the dialog (when the original attempt is still animating) before we get this far into the process.

crisbeto · 2021-06-30T16:33:12Z

I think the check should happen in the dialog service since it is the one that determines if a dialog can open. One thing that I hadn't really considered when I posted my comment is what we would return if we decide not to open a dialog. We can't create a dialog ref without actually creating the dialog.

Splaktar · 2021-06-30T16:38:47Z

One thing that I hadn't really considered when I posted my comment is what we would return if we decide not to open a dialog. We can't create a dialog ref without actually creating the dialog.

Right. For the moment, I'm throwing an Error in that case.

crisbeto · 2021-06-30T16:48:21Z

I'm not sure that throwing the error would be the right thing either, because it would be a breaking change and consumers wouldn't have a way of fixing it.

jelbourn · 2021-06-30T16:56:04Z

I don't quite follow how these changes would be fixing the dialog information not being read. What mechanically is changing about how focus is set that leads to the cause in behavior change?

Splaktar · 2021-06-30T17:05:34Z

I'm not sure that throwing the error would be the right thing either, because it would be a breaking change and consumers wouldn't have a way of fixing it.

Agreed, but I don't have a better approach in mind atm. We could do a different breaking change to have open() return a MatDialogRef | null, but that's not great either.

Splaktar · 2021-06-30T17:10:48Z

I don't quite follow how these changes would be fixing the dialog information not being read. What mechanically is changing about how focus is set that leads to the cause in behavior change?

The quick swap of the focus from the trigger button to the dialog container and then to the first focusable element in the dialog is what causes the screen reader to bail out and not announce anything other than being on a focusable element. This approach does work to avoid multiple dialogs being opened while one is animating open.

I've verified two paths to fixing this, while still avoiding multiple dialogs being opened while one is animating open

the previous approach of blurring the trigger button on open
- which causes some test failures on iOS as it puts the focus on the body which iOS doesn't like since it's not a form element
the current approach in this PR which blocks opening of new dialogs while a dialog is animating open

jelbourn · 2021-06-30T18:32:39Z

So to make sure I understand correctly:

Previously, the dialog always focused itself, and then moved focus to the first tabbable elements / cdkFocusInitial element once the animation finishes?

With the PR, this focus only ever goes to the configured element once the animation finishes without going to the container?

How does the case of multiple dialogs opening simultaneously play into this? Is it just that they fight for focus in a way that messes up the reader? Rather than throwing an error for this case, we could try to figure out if we can decide not to care as much about that case, since multiple/nested modal dialogs are already somewhat of an anti-pattern.

crisbeto · 2021-06-30T18:35:32Z

To provide some context: this bouncing around of focus was introduced, because if you have a button that opens a dialog and you press it using the keyboard, you could end up triggering multiple "clicks" if your finger lingers on the button a bit longer than expected. This is made more likely by the fact that we only move focus away from the button once the ~300ms dialog animation has finished.

jelbourn · 2021-06-30T20:21:42Z

Ah, I see. What do you think about setting the activeElement to pointer-events: none until the focus lands in the new place?

crisbeto · 2021-06-30T20:25:14Z

I haven't actually tried it, but I would assume that setting pointer-events: none would either:

Not block keyboard events since the keyboard isn't a pointer.
Blur the element, similarly to what happens a button becomes disabled.

We can always try it and see what happens. We could also try to do something where we save the trigger element when a dialog is opened. If another one is triggered while the dialog is animating, we return the previous dialog ref.

zarend · 2021-08-03T16:59:21Z

Yes, I'll run the presubmit tests overnight tonight. 👍
…
On Mon, Aug 2, 2021 at 12:16 PM Michael Prentice @.***> wrote: I was also able to remove all of the flush(); calls and the unit tests still pass. @zarend https://github.com/zarend can you please presubmit? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23085 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB242NJJ75ZECRA7J3OZR4TT23VJHANCNFSM47PELB7Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

presubmit (internal) had build failures last night 🤦 . I fixed the build failures, and will run the tests tonight 🤞 .

Splaktar · 2021-08-03T18:44:54Z

@devversion this code seems to run fine on all platforms other than iOS (Sauce Labs).

components/src/material-experimental/mdc-dialog/dialog.spec.ts

Lines 1330 to 1339 in 087ed2c

// Fake the behavior of pressing the SPACE key on a button element. Browsers fire a `click`
// event with a MouseEvent, which has coordinates that are out of the element boundaries.
dispatchMouseEvent(closeButton,'click',0,0);
viewContainerFixture.detectChanges();
tick(500);
expect(lastFocusOrigin!)
.withContext('Expected the trigger button to be focused via keyboard')
.toBe('keyboard');

components/src/material-experimental/mdc-dialog/dialog.spec.ts

Lines 1370 to 1380 in 087ed2c

// The dialog close button detects the focus origin by inspecting the click event. If
// coordinates of the click are not present, it assumes that the click has been triggered
// by keyboard.
dispatchMouseEvent(closeButton,'click',10,10);
viewContainerFixture.detectChanges();
tick(500);
expect(lastFocusOrigin!)
.withContext('Expected the trigger button to be focused via mouse')
.toBe('mouse');

I'm not sure why it was passing before, but now it's failing with the changes in this PR, but only on iOS.

devversion · 2021-08-03T19:46:41Z

@Splaktar Not sure from the top of my head. This seems like something that needs to be investigated within the emulator.

zarend · 2021-08-05T20:30:26Z

The internal test results are available, 13 broken targets.

Splaktar · 2021-08-05T22:47:17Z

Great! Are any of those related to iOS?

Splaktar · 2021-08-06T18:18:45Z

I am able to reproduce these iOS test failures in a Simulator locally, investigating...

zarend · 2021-08-06T19:39:20Z

Great! Are any of those related to iOS?

I did a quick glance at the internal failures. They're mostly screenshot failures, and don't look iOS related. Jeremy and I are going over them in more detail later this afternoon.

Splaktar · 2021-08-06T19:46:44Z

OK, I think that I've got a solution for the iOS test failures on Sauce Labs. It works locally and now I'm just waiting on this CI run to confirm.

- 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 material/dialog API golden file Fixesangular#21840

Splaktar · 2021-08-09T17:52:03Z

Resolved conflict in test.

Splaktar · 2021-08-12T19:55:18Z

@zarend any progress with these presubmit failures?

zarend · 2021-08-12T20:10:09Z

@zarend any progress with these presubmit failures?

yes, I have a passing presubmit, which I got to pass by patching failing tests 🎉 . I am the caretaker today and tomorrow, so I'll merge and sync this PR after we finish the sync that's in progress.

zarend · 2021-08-12T20:12:33Z

ohh, we still need PR reviews for this.

amysorto · 2021-08-12T20:42:47Z

 case true:
 case 'first-tabbable':
- this._focusTrap.focusInitialElementWhenReady();
+ this._focusTrap.focusInitialElementWhenReady().then(focusedSuccessfully => {


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.

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

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

We should double-check that they don't

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

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

zarend

this seems reasonable to me 👍

jelbourn

lgtm

jelbourn · 2021-08-12T22:39:23Z

 case true:
 case 'first-tabbable':
- this._focusTrap.focusInitialElementWhenReady();
+ this._focusTrap.focusInitialElementWhenReady().then(focusedSuccessfully => {


We should double-check that they don't

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 a review from crisbeto June 29, 2021 02:25

Splaktar requested a review from jelbourn as a code owner June 29, 2021 02:25

google-claBot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 29, 2021

crisbeto reviewed Jun 29, 2021
View reviewed changes

Comment threadsrc/material/dialog/dialog-container.ts Outdated

Splaktar marked this pull request as draft June 29, 2021 16:10

Splaktar force-pushed the mdc-dialog-fix-opening-a11y branch 3 times, most recently from 0e636aa to abffbf3Compare June 30, 2021 04:49

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

Splaktar force-pushed the mdc-dialog-fix-opening-a11y branch from abffbf3 to d59d5ebCompare June 30, 2021 04:56

Splaktar changed the title ~~fix(material-experimental/mdc-dialog): improve screen reader support when opened~~fix(material/dialog): improve screen reader support when openedJun 30, 2021

Splaktar marked this pull request as ready for review June 30, 2021 05:12

Splaktar requested a review from devversion as a code owner June 30, 2021 05:12

Splaktar force-pushed the mdc-dialog-fix-opening-a11y branch from e6b838d to 235b0bcCompare August 6, 2021 19:37

Splaktar commented Aug 6, 2021
View reviewed changes

Comment threadtools/public_api_guard/material/dialog.md

Splaktar requested a review from crisbeto August 6, 2021 20:00

Splaktar force-pushed the mdc-dialog-fix-opening-a11y branch from 235b0bc to 5dd0503Compare August 9, 2021 17:51

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

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

amysorto reviewed Aug 12, 2021
View reviewed changes

Splaktar added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged labels Aug 12, 2021

zarend approved these changes Aug 12, 2021
View reviewed changes

jelbourn approved these changes Aug 12, 2021
View reviewed changes

jelbourn added the action: merge The PR is ready for merge by the caretaker label Aug 12, 2021

zarend merged commit 728cf1c into angular:masterAug 13, 2021

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

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

Uh oh!

Conversation

Splaktar commented Jun 29, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Splaktar commented Jun 29, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Splaktar commented Jun 30, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crisbeto commented Jun 30, 2021

Uh oh!

Splaktar commented Jun 30, 2021

Uh oh!

crisbeto commented Jun 30, 2021

Uh oh!

jelbourn commented Jun 30, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Splaktar commented Jun 30, 2021

Uh oh!

Splaktar commented Jun 30, 2021

Uh oh!

jelbourn commented Jun 30, 2021

Uh oh!

crisbeto commented Jun 30, 2021

Uh oh!

jelbourn commented Jun 30, 2021

Uh oh!

crisbeto commented Jun 30, 2021

Uh oh!

zarend commented Aug 3, 2021

Uh oh!

Splaktar commented Aug 3, 2021

Uh oh!

devversion commented Aug 3, 2021

Uh oh!

zarend commented Aug 5, 2021

Uh oh!

Splaktar commented Aug 5, 2021

Uh oh!

Splaktar commented Aug 6, 2021

Uh oh!

zarend commented Aug 6, 2021

Uh oh!

Splaktar commented Aug 6, 2021

Uh oh!

Uh oh!

Splaktar commented Aug 9, 2021

Uh oh!

Splaktar commented Aug 12, 2021

Uh oh!

zarend commented Aug 12, 2021

Uh oh!

zarend commented Aug 12, 2021

Uh oh!

amysortoAug 12, 2021

Choose a reason for hiding this comment

Uh oh!

amysortoAug 12, 2021

Choose a reason for hiding this comment

Uh oh!

SplaktarAug 12, 2021

Choose a reason for hiding this comment

Uh oh!

jelbournAug 12, 2021

Choose a reason for hiding this comment

Uh oh!

SplaktarAug 13, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SplaktarSep 17, 2021

Choose a reason for hiding this comment

Uh oh!

Splaktar commented Jun 29, 2021•
edited
Loading

Splaktar commented Jun 29, 2021•
edited
Loading

Splaktar commented Jun 30, 2021•
edited
Loading

jelbourn commented Jun 30, 2021•
edited
Loading

SplaktarAug 13, 2021•
edited
Loading