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(overlays): focus is returned to last focus element when focusing toast #28950

Merged
merged 9 commits into from
Feb 14, 2024

Conversation

liamdebeasi
Copy link
Contributor

@liamdebeasi liamdebeasi commented Jan 31, 2024

Issue number: resolves #28261


What is the current behavior?

When moving focus from a focus-trapped overlay to a toast, focus is moved back to the overlay. This is the correct behavior as focus should never leave a focus-trapped overlay (unless the overlay is dismissed or focus is moved to a new top-most overlay). However, the way we return focus is a bit unexpected because it always returns focus to the last focusable element in the overlay.

This means that if you were focused on the first focusable element, presented the toast, and then focused the toast, focus might not be moved back to that first focusable element. In the case of the linked issue, this was causing an unexpected scroll so that the last focused element could be in view.

What is the new behavior?

  • This fix adds an exception for ion-toast (as it is the only overlay that is not focus trapped) that ensures that focus is moved back to the last focus element.

Does this introduce a breaking change?

  • Yes
  • No

Other information

Dev build: 7.7.1-dev.11707253408.186eea70

Note: We don't recommend this pattern in general because it would be impossible for a screen reader user to focus the toast. However, we can at least improve the experience for developers who continue to implement this pattern by returning focus in a more predictable manner.

Docs: ionic-team/ionic-docs#3432

Testing:

Reviewers should manually test the following behaviors:

  1. Create a modal with 2 buttons. Have one of the buttons present a toast. Open the toast and verify that you can still Tab to cycle through the buttons in the modal.
  2. Create a modal with 2 buttons. Have one of the buttons present a toast. Open the toast. Move focus to the toast and verify that you can still Tab to cycle through the buttons in the modal (once focus is returned to the modal).

@github-actions github-actions bot added the package: core @ionic/core package label Jan 31, 2024
@liamdebeasi liamdebeasi changed the title Fw 5273 fix(overlays): focus is returned to last focus element when focusing toast Feb 6, 2024
@liamdebeasi liamdebeasi marked this pull request as ready for review February 6, 2024 21:25
@liamdebeasi liamdebeasi requested a review from a team as a code owner February 6, 2024 21:25
@liamdebeasi liamdebeasi requested review from averyjohnston and removed request for a team February 6, 2024 21:25
};

const isOverlayHidden = (overlay: Element) => overlay.classList.contains('overlay-hidden');
Copy link
Contributor Author

@liamdebeasi liamdebeasi Feb 6, 2024

Choose a reason for hiding this comment

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

there should be no behavior change to focusFirstDescendant and focusLastDescendant. All I did here was abstract the focusing logic to focusElementInOverlay and had those functions call it instead. I needed to use focusElementInOverlay to implement the fix, so I figured I should consolidate otherwise we're writing the same code 3x.

if (lastInput) {
lastInput.focus();
if (elementToFocus) {
focusVisibleElement(elementToFocus);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One thing I noticed was that focusLastDescendant called .focus() whereas focusFirstDescendant calls focusVisibleElement. I believe this was an oversight on my end when I originally implemented this since I was only concerned about moving focus to the first element in the select popover: 8be9ae1

However, focusVisibleElement also calls .focus so there should be no behavior change.

@liamdebeasi
Copy link
Contributor Author

I decided to consolidate some code under the hood otherwise I was going to write the same code 3x. This isn't strictly necessary to ship this fix (though it makes it easier to implement the fix), so if the reviewers would prefer I pull this out to a separate PR let me know!

Copy link
Contributor

@averyjohnston averyjohnston left a comment

Choose a reason for hiding this comment

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

Functionality looks great in my local testing 👍 Just had a question about the tests.

core/src/utils/test/overlays/overlays.e2e.ts Outdated Show resolved Hide resolved
@liamdebeasi liamdebeasi requested a review from a team February 12, 2024 22:05
@liamdebeasi liamdebeasi added this pull request to the merge queue Feb 14, 2024
Merged via the queue into main with commit 2ed0ada Feb 14, 2024
46 checks passed
@liamdebeasi liamdebeasi deleted the FW-5273 branch February 14, 2024 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: focus trapping is not disabled on non-top overlay which causes side effects with toast
2 participants