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.

Sign up for GitHub

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

Jump to bottom

Modal loses focus trap after clicking inside #6281

Closed
nwhittaker opened this issue Jan 13, 2023 · 12 comments
Closed

Modal loses focus trap after clicking inside #6281

nwhittaker opened this issue Jan 13, 2023 · 12 comments
Assignees
@geospatialem
Labels
4 - verified Issues that have been released and confirmed resolved. ArcGIS Field Apps Issues logged by ArcGIS Field Apps team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. estimate - 5 A few days of work, definitely requires updates to tests. p - high Issue should be addressed in the current milestone, impacts component or core functionality
Milestone
2023 April Priori...

Comments

@nwhittaker
Copy link
Contributor

nwhittaker commented Jan 13, 2023 •
edited

Actual Behavior

Clicking inside a modal causes the modal's focus trap to become deactivated.

Expected Behavior

Clicking inside a modal does not deactivate the modal's focus trap.

Reproduction Sample

https://codepen.io/nwhittaker-esri/pen/JjBJYee

Reproduction Steps

  1. Visit the sample and tab around to see the focus trap is in place.
  2. Click on the modal's header, its content, or on any of its buttons (slotted or otherwise).
  3. Tab around and see focus now moves outside of the modal.

Reproduction Version

next.721

Relevant Info

No response

Regression?

No response

Impact

A very high adverse impact to accessibility.

Esri team

ArcGIS Field Apps
@nwhittaker nwhittaker added bug Bug reports for broken functionality. Issues should include a reproduction of the bug. 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Jan 13, 2023
@github-actions github-actions bot added the ArcGIS Field Apps Issues logged by ArcGIS Field Apps team members. label Jan 13, 2023
@nwhittaker nwhittaker changed the title Modal loses focus trap after button click Modal loses focus trap after clicking inside it Jan 13, 2023
@nwhittaker nwhittaker changed the title Modal loses focus trap after clicking inside it Modal loses focus trap after clicking inside Jan 18, 2023
@jcfranco jcfranco added p - high Issue should be addressed in the current milestone, impacts component or core functionality and removed needs triage Planning workflow - pending design/dev review. labels Jan 25, 2023
@jcfranco jcfranco self-assigned this Feb 3, 2023
@jcfranco
Copy link
Member

jcfranco commented Feb 3, 2023

Preliminary findings on this one:

  • focus-trap is configured to disable the trap when clicking outside.
  • By default, clickOutsideDeactivates: true is deactivating the trap when clicking inside the modal
  • Using a custom clickOutsideDeactivates handler (deactivating if the click target is outside of the modal trap) helps fix this issue, but is now causing another issue where clicking on the focusable items within the modal (buttons in the example case) aren't consistently focused. This is the main blocker at the moment.
  • document is set to the trap's owner document instead of the component's shadow host (as recommended in the doc).
  • The tab index for the fallback focus element is set to -1 via prop instead of attribute (as recommended in the doc). This doesn't reflect and could potentially mess up anything querying by the attribute.

At this point, it's unclear to me whether this is a limitation of focus-trap + shadow DOM, a bug or something we're missing in our focus-trap options. We'll have to dig a bit more.

@jcfranco jcfranco removed their assignment Feb 3, 2023
@jcfranco jcfranco added the estimate - 3 A day or two of work, likely requires updates to tests. label Feb 3, 2023
@jcfranco jcfranco added 1 - assigned Issues that are assigned to a sprint and a team member. and removed 0 - new New issues that need assignment. labels Feb 7, 2023
@anveshmekala anveshmekala mentioned this issue Feb 7, 2023
Merged
@anveshmekala anveshmekala added 2 - in development Issues that are actively being worked on. and removed 1 - assigned Issues that are assigned to a sprint and a team member. labels Feb 7, 2023
@geospatialem
Copy link
Member

Migrating to the upcoming February milestone for the fix to land later this month.

anveshmekala added a commit that referenced this issue Feb 9, 2023
@anveshmekala
fix(modal): no longer loses focus trap after clicking inside the comp…
df144dc 
…onent. (#6434)

**Related Issue:** #6281 

## Summary
This PR will fix `focusTrap` issue when user clicks inside the `modal`.
When user clicks inside the modal, `focusTrap` behavior wont be
affected.
@anveshmekala anveshmekala added 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed 2 - in development Issues that are actively being worked on. labels Feb 9, 2023
@jcfranco jcfranco mentioned this issue Feb 23, 2023
Closed
@jcfranco
Copy link
Member

jcfranco commented Mar 4, 2023

I finally managed to reproduce the issue that required us to revert previously. Need to spend some extra time digging into this one.

@brittneytewks brittneytewks added milestone adjusted research Issues that require additional research in order to resolve or make decision. labels Mar 4, 2023
@geospatialem geospatialem added estimate - 5 A few days of work, definitely requires updates to tests. and removed estimate - 3 A day or two of work, likely requires updates to tests. labels Apr 4, 2023
@geospatialem geospatialem added 1 - assigned Issues that are assigned to a sprint and a team member. and removed 0 - new New issues that need assignment. labels Apr 4, 2023
@geospatialem
Copy link
Member

I finally managed to reproduce the issue that required us to revert previously. Need to spend some extra time digging into this one.

@jcfranco Provide research findings to the April milestone, work will be carried out in a future milestone.

@jcfranco jcfranco mentioned this issue Apr 10, 2023
Closed
@driskull
Copy link
Member

At this point, it's unclear to me whether this is a limitation of focus-trap + shadow DOM

That's my guess. I'm guessing focus-trap and shadow DOM don't work well here.

It may just need to be acceptable for now that clicking disables the focus trap.

@jcfranco
Copy link
Member

This was fixed via #6581. Testing looks great on #6454 too!

Thanks @driskull! 🚀

cc @anveshmekala

@jcfranco jcfranco added 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed research Issues that require additional research in order to resolve or make decision. 1 - assigned Issues that are assigned to a sprint and a team member. labels Apr 13, 2023
@github-actions github-actions bot assigned geospatialem and unassigned jcfranco Apr 13, 2023
@github-actions
Copy link
Contributor

Installed and assigned for verification.

@driskull
Copy link
Member

Sweeet!

@geospatialem geospatialem added 4 - verified Issues that have been released and confirmed resolved. and removed 3 - installed Issues that have been merged to master branch and are ready for final confirmation. labels Apr 13, 2023
@geospatialem
Copy link
Member

Verified with 1.3.0-next.3
modal-verify-focus-trap-click-inside

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@geospatialem geospatialem

Labels
4 - verified Issues that have been released and confirmed resolved. ArcGIS Field Apps Issues logged by ArcGIS Field Apps team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. estimate - 5 A few days of work, definitely requires updates to tests. p - high Issue should be addressed in the current milestone, impacts component or core functionality
Projects
None yet
Milestone
2023 April Priorities - No breaking c...
Development

No branches or pull requests
8 participants
@jcfranco @nwhittaker @driskull @alisonailea @geospatialem @benelan @anveshmekala @brittneytewks