fix(material/dialog): don't wait for animation before moving focus by crisbeto · Pull Request #24121 · angular/components

crisbeto · 2021-12-20T06:56:59Z

For a long time we used to move focus into the dialog after the opening animation was finished, because it allowed for the content to settle in case something like an autocomplete needed to open. This approach has caused us accessibility issues in the past, because we've had to find ways to prevent the user from interacting with content behind the dialog while the opening animation is running.

Initially we used to move focus to the dialog container before transferring it into the dialog, but that ended up interrupting the screen reader. In a later change we started returning the previous dialog ref if the same kind of dialog is opened while the animation is still running. The problem with this approach is that it doesn't allow for multiple dialog to be opened quickly (see #24037).

These changes aim to address the root cause by moving focus immediately after the dialog content is attached. If this causes regressions with autocompletes inside dialogs, we now have APIs that allow users to customize the focus behavior.

Fixes#24037.

Splaktar

I want to run this locally with a screen reader to test it out, but I'll have to find another time to do that on another laptop since I can't run the dev-app on my primary machine (m1Max mac) due to #23983.

Which screen reader, browser, OS combinations did you test this against?

crisbeto · 2022-01-06T19:48:59Z

@Splaktar the bot should post a link to the deployed dev app soon.

github-actions · 2022-01-06T19:54:15Z

Deployed dev-app to: https://ng-comp-dev--pr-24121-d4f9d3ee70676a2c43a9635cdb67593a-qjt9uxko.web.app

jelbourn

LGTM. I tried this in VoiceOver and NVDA

Splaktar · 2022-01-13T18:42:24Z

I'll test in ChromeVox since that was what the original a11y fix was meant to address in #21840.

Splaktar

Verified in ChromeVox on ChromeOS 96.0.4664.111.

This LGTM, thank you!

zarend · 2022-02-09T00:30:26Z

This caused 105 failures on an internal test run from 5 days ago. I looked at a sample of 12 of them and they were all screenshot diff changes where a different element was focused after opening a dialog. Most of the changes looked reasonable to me, but there was one case that was questionable. There's some example of highlighting the text of an input field on opening a dialog. With this change, it no longer highlights that text.

For a long time we used to move focus into the dialog after the opening animation was finished, because it allowed for the content to settle in case something like an autocomplete needed to open. This approach has caused us accessibility issues in the past, because we've had to find ways to prevent the user from interacting with content behind the dialog while the opening animation is running. Initially we used to move focus to the dialog container before transferring it into the dialog, but that ended up interrupting the screen reader. In a later change we started returning the previous dialog ref if the same kind of dialog is opened while the animation is still running. The problem with this approach is that it doesn't allow for multiple dialog to be opened quickly (see angular#24037). These changes aim to address the root cause by moving focus immediately after the dialog content is attached. If this causes regressions with autocompletes inside dialogs, we now have APIs that allow users to customize the focus behavior. Fixesangular#24037.

crisbeto · 2022-03-04T09:22:43Z

Given that a lot of folks seem to be blocked by this externally and that the current behavior is difficult to land internally, I've pushed an extra commit to make the instant focus opt-in. Hopefully this makes the change easier to land.

…ocus

…24121) * fix(material/dialog): don't wait for animation before moving focus For a long time we used to move focus into the dialog after the opening animation was finished, because it allowed for the content to settle in case something like an autocomplete needed to open. This approach has caused us accessibility issues in the past, because we've had to find ways to prevent the user from interacting with content behind the dialog while the opening animation is running. Initially we used to move focus to the dialog container before transferring it into the dialog, but that ended up interrupting the screen reader. In a later change we started returning the previous dialog ref if the same kind of dialog is opened while the animation is still running. The problem with this approach is that it doesn't allow for multiple dialog to be opened quickly (see #24037). These changes aim to address the root cause by moving focus immediately after the dialog content is attached. If this causes regressions with autocompletes inside dialogs, we now have APIs that allow users to customize the focus behavior. Fixes#24037. * fixup! fix(material/dialog): don't wait for animation before moving focus (cherry picked from commit 92863cc)

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@angular/cdk](https://github.com/angular/components) | dependencies | patch | [`13.2.5` -> `13.2.6`](https://renovatebot.com/diffs/npm/@angular%2fcdk/13.2.5/13.2.6) | | [@angular/material](https://github.com/angular/components) | dependencies | patch | [`13.2.5` -> `13.2.6`](https://renovatebot.com/diffs/npm/@angular%2fmaterial/13.2.5/13.2.6) | --- ### Release Notes <details> <summary>angular/components</summary> ### [`v13.2.6`](https://github.com/angular/components/blob/HEAD/CHANGELOG.md#1326-suede-spaghetti-2022-03-09) [Compare Source](angular/components@13.2.5...13.2.6) ##### cdk | Commit | Type | Description | | -- | -- | -- | | [39929a815d](angular/components@39929a8) | fix | **overlay:** backdrop timeouts not being cleared in some cases ([#23972](angular/components#23972)) | | [2f2b0c7cf4](angular/components@2f2b0c7) | fix | **testing:** dispatch mouseover and mouseout events in UnitTestElement ([#24490](angular/components#24490)) | | [edca54f2d0](angular/components@edca54f) | fix | **testing:** require at least one argument for locator functions ([#23619](angular/components#23619)) | ##### material | Commit | Type | Description | | -- | -- | -- | | [c4993ac171](angular/components@c4993ac) | fix | **button:** avoid setting a tabindex on all link buttons ([#22901](angular/components#22901)) | | [c47d30e0e5](angular/components@c47d30e) | fix | **dialog:** don't wait for animation before moving focus ([#24121](angular/components#24121)) | | [70b8248568](angular/components@70b8248) | fix | **expansion:** able to tab into descendants with visibility while closed ([#24045](angular/components#24045)) | | [d22d73ab8d](angular/components@d22d73a) | fix | **select:** disabled state out of sync when swapping form group with a disabled one ([#17872](angular/components#17872)) | | [911d6b71d4](angular/components@911d6b7) | fix | **slide-toggle:** clear name from host node ([#15505](angular/components#15505)) | | [4b5363d160](angular/components@4b5363d) | fix | **tooltip:** decouple removal logic from change detection ([#19432](angular/components#19432)) | ##### material-experimental | Commit | Type | Description | | -- | -- | -- | | [8414646d79](angular/components@8414646) | fix | **mdc-card:** remove extra margin if header doesn't have an avatar ([#19072](angular/components#19072)) | | [f66486dc5b](angular/components@f66486d) | fix | **mdc-slider:** fix a few null pointer exceptions ([#23659](angular/components#23659)) | ##### multiple | Commit | Type | Description | | -- | -- | -- | | [6ee0089ce6](angular/components@6ee0089) | fix | don't block child component animations on open ([#24529](angular/components#24529)) | #### Special Thanks Andrew Seguin, Jeri Peier, Kristiyan Kostadinov and Paul Gschwendtner  </details> --- ### Configuration 📅 **Schedule**: At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1214 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

…ngular#24121) * fix(material/dialog): don't wait for animation before moving focus For a long time we used to move focus into the dialog after the opening animation was finished, because it allowed for the content to settle in case something like an autocomplete needed to open. This approach has caused us accessibility issues in the past, because we've had to find ways to prevent the user from interacting with content behind the dialog while the opening animation is running. Initially we used to move focus to the dialog container before transferring it into the dialog, but that ended up interrupting the screen reader. In a later change we started returning the previous dialog ref if the same kind of dialog is opened while the animation is still running. The problem with this approach is that it doesn't allow for multiple dialog to be opened quickly (see angular#24037). These changes aim to address the root cause by moving focus immediately after the dialog content is attached. If this causes regressions with autocompletes inside dialogs, we now have APIs that allow users to customize the focus behavior. Fixesangular#24037. * fixup! fix(material/dialog): don't wait for animation before moving focus

When I added the `delayFocusTrap` option in angular#24121, I assumed that it would be inherited into the MDC dialog. Since it looks like that's not actually the case, these changes implement it.

#24727) When I added the `delayFocusTrap` option in #24121, I assumed that it would be inherited into the MDC dialog. Since it looks like that's not actually the case, these changes implement it.

angular-automatic-lock-bot · 2022-04-07T00:03:36Z

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

crisbeto added P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release labels Dec 20, 2021

crisbeto requested review from Splaktar, andrewseguin and jelbourn December 20, 2021 06:56

crisbeto requested a review from devversion as a code owner December 20, 2021 06:57

crisbeto requested a review from a teamDecember 20, 2021 06:57

josephperrott removed the request for review from a teamDecember 20, 2021 17:22

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 29, 2021

Splaktar reviewed Jan 6, 2022
View reviewed changes

Comment threadtools/public_api_guard/material/dialog.md

crisbeto added the dev-app preview When applied, previews of the dev-app are deployed to Firebase label Jan 6, 2022

jelbourn approved these changes Jan 12, 2022
View reviewed changes

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

Splaktar approved these changes Jan 13, 2022
View reviewed changes

Splaktar mentioned this pull request Jan 19, 2022
bug(MatDialog): When opening multiple dialogs in a short amount of time, some of them do not open #24037
Closed
1 task

crisbeto removed the dev-app preview When applied, previews of the dev-app are deployed to Firebase label Mar 4, 2022

crisbeto force-pushed the 24037/dialog-focus branch from d4f9d3e to 94d6f88Compare March 4, 2022 09:14

fixup! fix(material/dialog): don't wait for animation before moving f…
6998080
…ocus

crisbeto force-pushed the 24037/dialog-focus branch from 94d6f88 to 6998080Compare March 4, 2022 13:36

crisbeto merged commit 92863cc into angular:masterMar 7, 2022

sync-by-unitoBot mentioned this pull request Mar 14, 2022
build(deps): bump @angular/cdk from 12.2.4 to 13.2.6 numbersprotocol/capture-cam#1370
Closed

sync-by-unitoBot mentioned this pull request Mar 22, 2022
build(deps): bump @angular/cdk from 12.2.4 to 13.3.0 numbersprotocol/capture-cam#1403
Closed

crisbeto mentioned this pull request Apr 4, 2022
fix(material-experimental/mdc-dialog): implement delayFocusTrap option #24727
Merged

angular-automatic-lock-botBot locked and limited conversation to collaborators Apr 7, 2022

Navigation Menu

Uh oh!

fix(material/dialog): don't wait for animation before moving focus - #24121

fix(material/dialog): don't wait for animation before moving focus#24121
crisbeto merged 2 commits into
angular:masterfrom
crisbeto:24037/dialog-focus

crisbeto commented Dec 20, 2021

Uh oh!

Splaktar left a comment

Uh oh!

Uh oh!

crisbeto commented Jan 6, 2022

Uh oh!

github-actionsBot commented Jan 6, 2022

Uh oh!

jelbourn left a comment

Uh oh!

Splaktar commented Jan 13, 2022

Uh oh!

Splaktar left a comment

Uh oh!

zarend commented Feb 9, 2022

Uh oh!

crisbeto commented Mar 4, 2022

Uh oh!

angular-automatic-lock-botBot commented Apr 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

crisbeto commented Dec 20, 2021

Uh oh!

Splaktar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

crisbeto commented Jan 6, 2022

Uh oh!

github-actionsBot commented Jan 6, 2022

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Splaktar commented Jan 13, 2022

Uh oh!

Splaktar left a comment

Choose a reason for hiding this comment

Uh oh!

zarend commented Feb 9, 2022

Uh oh!

crisbeto commented Mar 4, 2022

Uh oh!

angular-automatic-lock-botBot commented Apr 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants