Page MenuHomePhabricator

[web][fixed] Added FocusTrap for modals
ClosedPublic

Authored by kuba on May 24 2023, 9:55 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 6, 3:43 PM
Unknown Object (File)
Tue, Nov 5, 10:07 PM
Unknown Object (File)
Tue, Nov 5, 9:06 AM
Unknown Object (File)
Tue, Nov 5, 9:06 AM
Unknown Object (File)
Mon, Nov 4, 10:46 AM
Unknown Object (File)
Wed, Oct 30, 12:23 PM
Unknown Object (File)
Wed, Oct 30, 12:23 PM
Unknown Object (File)
Wed, Oct 30, 12:23 PM
Subscribers

Details

Summary

Diff which was reverted: https://phab.comm.dev/D7837
More info on why it was reverted: https://linear.app/comm/issue/ENG-3928/focustrap-causes-regressions-to-modals-in-landing

Added fallbackFocus option, which sets focus to div when there are no focusable elements inside the FocusTrap.

Old description:
We want to prevent users from focusing outside the modals. Added focus-trap-react library, to do that, and covered the modals that we use.

Test Plan

Checked if in edit mode and in other modals user can't focus outside the modal using the tab.
Checked if the modals without focusable elements (i.g. in the investor's tab on the landing page) also work.

Videos: https://linear.app/comm/issue/ENG-3928#comment-13a62270

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kuba edited the test plan for this revision. (Show Details)
kuba added inline comments.
lib/components/modal-overlay.react.js
63 ↗(On Diff #27004)

It fixes the issue: Your focus-trap must have at least one container with at least one tabbable node in it at all times raised in ENG-3928, when user clicked on the investor without buttons.

Solution source: https://github.com/focus-trap/focus-trap-react/issues/127#issuecomment-698966009

kuba retitled this revision from [web] Added FocusTrap for modals to [web][fixed] Added FocusTrap for modals.May 24 2023, 10:07 AM
kuba edited the test plan for this revision. (Show Details)
ashoat added inline comments.
lib/components/modal-overlay.react.js
63 ↗(On Diff #27004)

All object params should be memoized in function components

ashoat requested changes to this revision.May 24 2023, 11:14 AM

Requesting changes for comment above

This revision now requires changes to proceed.May 24 2023, 11:14 AM
kuba marked an inline comment as done.
kuba edited the summary of this revision. (Show Details)

Memoize param

This revision is now accepted and ready to land.May 25 2023, 3:58 AM