ionic-team / ionic-framework

A powerful cross-platform UI toolkit for building native-quality iOS, Android, and Progressive Web Apps with HTML, CSS, and JavaScript.
https://ionicframework.com
MIT License
51.08k stars 13.51k forks source link

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

Closed infacto closed 9 months ago

infacto commented 1 year ago

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.

liamdebeasi commented 1 year ago

Thanks for the report. I can reproduce this on Ionic 7 as well. It looks like this is another instance of https://github.com/ionic-team/ionic-framework/issues/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): https://github.com/ionic-team/ionic-framework/issues/24646#issuecomment-1538250165

infacto commented 1 year ago

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 commented 1 year ago

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 commented 9 months ago

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.

liamdebeasi commented 9 months ago

Thanks for the issue. This has been resolved via https://github.com/ionic-team/ionic-framework/pull/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.

ionitron-bot[bot] commented 8 months ago

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.