search

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.02k stars 13.52k forks source link

bug: modal overlay not automatically focussed when ModalOption keyboardClose is false #28775

Open RasmusKjeldgaard opened 9 months ago

RasmusKjeldgaard commented 9 months ago

Prerequisites

Ionic Framework Version

v7.6.2

Current Behavior

For both the inline and controller-based modal, setting keyboardClose to false results in a lack of focus trapping inside the modal. Focus remains on the trigger element and subsequent tabs will traverse through the tabbable elements behind the modal.

Furthermore this leads to applications needing to guard against accidentally instantiating multiple modals on top of each other when using the ModalController. Users might press enter/space repeatedly and unknowingly instantate many modals as they are not immediately discoverable.

Expected Behavior

I expect to be able to immediately tab to the cancel button of the modal once the modal is opened, and cycle through only the elements inside the modal when tabbing.

Steps to Reproduce

  1. clone, install and serve: https://github.com/RasmusKjeldgaard/ionic-modal-keyboardClose-repro
    • Alternatively this stackblitz with a minor tweak to the default docs example can be used for quick verification
  2. tab to or click on the 'open' button so the modal opens
  3. tab again and see that focus still behind the modal

Code Reproduction URL

https://github.com/RasmusKjeldgaard/ionic-modal-keyboardClose-repro

Ionic Info

Ionic:

Ionic CLI : 7.1.1 (/Users/rkjeldgaard/.nvm/versions/node/v18.13.0/lib/node_modules/@ionic/cli) Ionic Framework : @ionic/angular 7.6.2 @angular-devkit/build-angular : 17.0.8 @angular-devkit/schematics : 17.0.8 @angular/cli : 17.0.8 @ionic/angular-toolkit : 9.0.0

Capacitor:

Capacitor CLI : 5.6.0 @capacitor/android : not installed @capacitor/core : 5.6.0 @capacitor/ios : not installed

Utility:

cordova-res : not installed globally native-run : 2.0.0

System:

NodeJS : v18.13.0 (/Users/rkjeldgaard/.nvm/versions/node/v18.13.0/bin/node) npm : 8.19.3 OS : macOS Unknown

Additional Information

No response

ionitron-bot[bot] commented 9 months ago

Thanks for the issue! This issue has been labeled as holiday triage. With the winter holidays quickly approaching, much of the Ionic Team will soon be taking time off. During this time, issue triaging and PR review will be delayed until the team begins to return. After this period, we will work to ensure that all new issues are properly triaged and that new PRs are reviewed. In the meantime, please read our Winter Holiday Triage Guide for information on how to ensure that your issue is triaged correctly. Thank you!

liamdebeasi commented 9 months ago

Thanks for the report. Ionic is working as intended here, though the behavior of keyboardClose is not obvious and can likely be improved.

Before the overlay animation runs we blur any focused input/textarea: https://github.com/ionic-team/ionic-framework/blob/75ffeee933ae353d2601670178896116c81923e0/core/src/utils/overlays.ts#L625-L631

We also optionally move focus into the overlay (after the animation) if keyboardClose is true: https://github.com/ionic-team/ionic-framework/blob/75ffeee933ae353d2601670178896116c81923e0/core/src/utils/overlays.ts#L508-L510

When keyboardClose is false we skip that second line of code I mentioned. Focus trapping is still active, but since focus is not in the overlay then focus trapping appears to not work. If you were to manually focus the modal then focus trapping would become active.

Fixing this is a little risky given both the age and vague behavior of the keyboardClose property. As a result, I'd probably make these changes in a major release and mark it as a breaking change to be safe. I'll discuss a course of action with the team.

RasmusKjeldgaard commented 9 months ago

Thank you for such a speedy and thorough reply! I had missed the detail about focus trapping still being functional. With that in mind there seems to be feasible workarounds, and since it is working as intended I wont argue if you think this issue should just be closed.

Thanks again for your help.