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

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

Closed
3 tasks done
infacto opened this issue Sep 29, 2023 · 6 comments · Fixed by #28950
Closed
3 tasks done
Labels
package: core @ionic/core package type: bug a confirmed bug report

Comments

@infacto
Copy link

infacto commented Sep 29, 2023

Prerequisites

Ionic Framework Version

v6.x

Current Behavior

When a modal is open, and then show a toast message: A click on the toast scroll the modal to the next e.g. button.

demo

It's a critical bug in my opinion or case. Because it always scrolls to the next focusable element either vertically or horizontally. If you have content with overflow: hidden and custom scroll button (like next in an intro slider) you jump to the element which is in overflow hidden. But anyway, this is a weird bug which needs to be fixed. I cannot figure out what the problem is. Maybe you have an idea?

Expected Behavior

Don't affect the scroll position. A click on toast (regardless if close button or everywhere else) should not scroll anything.

Steps to Reproduce

  1. Create a modal with scrollable content.
  2. Add a ion-button to the end of content.
  3. Add an element on top which is not focusable. In this case just a div with click event.
  4. The click event of div opens a toast message.
  5. Click on the message. In this demo you have to click twice. In my project a single click also causes the issue. Not sure why, but it does not matter. The issue can be reproduce in this demo here.
  6. See that a click on the toast scroll the content of the demo. It's maybe because of focus. But why? It should not happen!

Tip: Use mobile view in dev tools (Chrome "Toggle device toolbar". I'm on Windows 10 btw. But this bug is also present in Android).

Demo

Again: In my project, this bug is also triggered on a single click e.g. on X. In this demo here you need to click twice. Anywhere on toast body. The second click can be on X. But it does not matter where you click.

Code Reproduction URL

https://stackblitz.com/edit/ionic-6-toast-scroll-bug?file=src%2Fapp%2Fapp.component.html

Ionic Info

Ionic 6.6.1 (Angular 14). Not yet tested in Ionic 7. But I suspect the same. It's a candidate for patch fix in v6.

@ionitron-bot ionitron-bot bot added the triage label Sep 29, 2023
@liamdebeasi liamdebeasi self-assigned this Sep 29, 2023
@liamdebeasi
Copy link
Member

liamdebeasi commented Sep 29, 2023

Thanks for the report. I can reproduce this on Ionic 7 as well. It looks like this is another instance of #24646. That ticket references a non-Ionic overlay, but the underlying problem is the same. When the top-most overlay is an element that does not use Ionic's focus trapping (in this case ion-toast), the previous overlay tries to retain focus.

However, I'm going to keep the two tickets separate. We should be able to account for ion-toast automatically, though the TinyMCE use case in the linked thread will likely require a public API exposed. In other words, the solutions to solve each will be different. The overlay manager should be able to disable focus trapping on the non-top overlays to avoid this.

Also just to set expectations: We aren't maintaining Ionic 6 anymore, so we won't be supplying a patch for that version. However, we will supply a patch for the latest version of Ionic.

edit: A temporary workaround would be to add the .ion-disable-focus-trap class to the modal when the toast is presented (and then remove it when the toast is dismissed assuming the modal itself is still open): #24646 (comment)

@liamdebeasi liamdebeasi changed the title bug: Click on toast scroll content of e.g. modal. bug: focus trapping is not disabled on non-top overlay which prevents focus from moving to toast Sep 29, 2023
@liamdebeasi liamdebeasi changed the title bug: focus trapping is not disabled on non-top overlay which prevents focus from moving to toast bug: focus trapping is not disabled on non-top overlay which causes side effects with toast Sep 29, 2023
@liamdebeasi liamdebeasi added package: core @ionic/core package type: bug a confirmed bug report labels Sep 29, 2023
@liamdebeasi liamdebeasi removed their assignment Sep 29, 2023
@infacto
Copy link
Author

infacto commented Oct 4, 2023

Thanks for the .ion-disable-focus-trap workaround. I added it on cssClass with the modal controller without caring whether toast is present or not. I don't need the focus trap and wonder what the use cases are. It may fixes some issues but also creates new ones. I wouldn't focus elements like buttons or text inputs when an overlay is closed. It's annoying when e.g. the content scrolls or keyboard opens (mobile). I would understand it when the element was focused before by the user or dev. But in many other cases it behaves like a bug. Is there a use case where this is really useful? Otherwise I would disable this globally.

@liamdebeasi
Copy link
Member

Focus trapping is an accessibility feature for users who rely on keyboards or screen readers to navigate an application. It makes it so focus does not accidentally leave the modal. For users with low vision, relying on sight to determine if they are focused inside of the modal may be difficult. Additionally, having focus leave the modal unexpectedly can also be disorienting because users may not know where in the app they are focused.

We implement a similar pattern provided by the W3C: https://www.w3.org/WAI/ARIA/apg/patterns/dialog-modal/

@liamdebeasi
Copy link
Member

I was able to improve this behavior by ensuring that focus is returned to the last element that was focused in the modal. This should avoid the unexpected scrolling.

Here's a dev build if you are interested in testing the fix:

npm install @ionic/angular@7.7.1-dev.11707253408.186eea70

Note: I'd caution against this pattern in general because users relying on screen readers will be unable to navigate to the toast due to the focus trapping in the modal. A better pattern might be to create a "medium priority" component such as a banner and place it inside the modal. This could be an aria live region that politely announces the same information that is presented in the toast. We do have an open feature request for this if you are interested.

github-merge-queue bot pushed a commit that referenced this issue Feb 14, 2024
…toast (#28950)

Issue number: resolves #28261

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

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?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- 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
- [x] No

<!--
  If this introduces a breaking change:
1. Describe the impact and migration path for existing applications
below.
  2. Update the BREAKING.md file with the breaking change.
3. Add "BREAKING CHANGE: [...]" to the commit description when merging.
See
https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#footer
for more information.
-->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->


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).
@liamdebeasi
Copy link
Member

Thanks for the issue. This has been resolved via #28950, and a fix will be available in an upcoming release of Ionic Framework. Feel free to keep testing the dev build, and let me know if you run into any issues.

Copy link

ionitron-bot bot commented Mar 15, 2024

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: core @ionic/core package type: bug a confirmed bug report
Projects
None yet
2 participants