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.

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

fix(dialog): don't move focus to dialog container if focus is already inside the dialog #16297

Merged

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Jun 14, 2019

This is a follow-up to #16221. What we hadn't accounted for in the aforementioned PR is that the consumer may have turned off autoFocus so that they can handle it themselves and with our changes focus would be reset back to the container. These changes add an extra check which will ensure that focus is only moved if it's not inside the dialog already.

Note: I haven't written a test for this for the same reason as #16221: in order to capture the behavior properly, we need to be able to flush a Promise.resolve and the animations in a specific sequence, whereas the tests flush them all together.

… inside the dialog

This is a follow-up to angular#16221. What we hadn't accounted for in the aforementioned PR is that the consumer may have turned off `autoFocus` so that they can handle it themselves and with our changes focus would be reset back to the container. These changes add an extra check which will ensure that focus is only moved if it's not inside the dialog already.
@crisbeto crisbeto added P2 The issue is important to a large percentage of users, with a workaround G This is is related to a Google internal issue target: patch This PR is targeted for the next patch release labels Jun 14, 2019
@crisbeto crisbeto requested a review from jelbourn as a code owner June 14, 2019 17:41
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 14, 2019
@mmalerba mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jun 18, 2019
@andrewseguin andrewseguin merged commit 0ed9416 into angular:master Jun 26, 2019
crisbeto added a commit to crisbeto/material2 that referenced this pull request Jul 1, 2019
…bled and focus was moved while animating

These changes incorporate angular#16297 and angular#16221 into the bottom sheet since it follows a similar focus capturing behavior to `MatDialogContainer`. They ensure that focus is on the bottom sheet container when the animation is over, because it could've moved while we were animating. It also has an extra check to ensure that we don't move focus unnecessarily if the consumer decided to move focus themselves somewhere within the bottom sheet.
andrewseguin pushed a commit that referenced this pull request Jul 2, 2019
… inside the dialog (#16297)

This is a follow-up to #16221. What we hadn't accounted for in the aforementioned PR is that the consumer may have turned off `autoFocus` so that they can handle it themselves and with our changes focus would be reset back to the container. These changes add an extra check which will ensure that focus is only moved if it's not inside the dialog already.
crisbeto added a commit to crisbeto/material2 that referenced this pull request Jul 3, 2019
…is disabled and focus was moved while animating

These changes incorporate angular#16297 and angular#16221 into the experimental dialog since it follows a similar focus capturing behavior to `MatDialogContainer`. They ensure that focus is on the dialog container container when the animation is over, because it could've moved while we were animating. It also has an extra check to ensure that we don't move focus unnecessarily if the consumer decided to move focus themselves somewhere within the container.
jelbourn pushed a commit that referenced this pull request Jul 9, 2019
…is disabled and focus was moved while animating (#16446)

These changes incorporate #16297 and #16221 into the experimental dialog since it follows a similar focus capturing behavior to `MatDialogContainer`. They ensure that focus is on the dialog container container when the animation is over, because it could've moved while we were animating. It also has an extra check to ensure that we don't move focus unnecessarily if the consumer decided to move focus themselves somewhere within the container.
jelbourn pushed a commit that referenced this pull request Jul 17, 2019
…bled and focus was moved while animating

These changes incorporate #16297 and #16221 into the bottom sheet since it follows a similar focus capturing behavior to `MatDialogContainer`. They ensure that focus is on the bottom sheet container when the animation is over, because it could've moved while we were animating. It also has an extra check to ensure that we don't move focus unnecessarily if the consumer decided to move focus themselves somewhere within the bottom sheet.
jelbourn pushed a commit that referenced this pull request Jul 19, 2019
…bled and focus was moved while animating (#16418)

These changes incorporate #16297 and #16221 into the bottom sheet since it follows a similar focus capturing behavior to `MatDialogContainer`. They ensure that focus is on the bottom sheet container when the animation is over, because it could've moved while we were animating. It also has an extra check to ensure that we don't move focus unnecessarily if the consumer decided to move focus themselves somewhere within the bottom sheet.
andrewseguin pushed a commit that referenced this pull request Jul 29, 2019
…bled and focus was moved while animating (#16418)

These changes incorporate #16297 and #16221 into the bottom sheet since it follows a similar focus capturing behavior to `MatDialogContainer`. They ensure that focus is on the bottom sheet container when the animation is over, because it could've moved while we were animating. It also has an extra check to ensure that we don't move focus unnecessarily if the consumer decided to move focus themselves somewhere within the bottom sheet.
@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 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants