Page MenuHomePhabricator

[web] Added FocusTrap for modals
ClosedPublic

Authored by kuba on May 16 2023, 5:43 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Jun 15, 9:54 PM
Unknown Object (File)
Sat, Jun 15, 5:25 AM
Unknown Object (File)
Fri, Jun 14, 8:09 PM
Unknown Object (File)
Fri, Jun 14, 12:57 AM
Unknown Object (File)
Tue, Jun 11, 2:17 PM
Unknown Object (File)
Sun, Jun 9, 5:54 AM
Unknown Object (File)
Thu, Jun 6, 11:01 AM
Unknown Object (File)
Mon, Jun 3, 9:17 AM
Subscribers

Details

Summary

Previously:
[web] Disabling tab when in edit mode
We want to prevent the user from using the tab when he is editing the message. Otherwise, he could navigate to other threads in the background.

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.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

lib/components/modal-overlay.react.js
46 ↗(On Diff #26529)

What is 9? Can we add a constant for this value so the code is more readable?

Not a fan of this from an accessibility stand point. I found something like this: focus-trap, we could wrap all our modals with it. But I'm not sure if that's worth it. @ashoat any thoughts?

Good find @michal! It seems like simply adding the focus-trap-react NPM package, and then wrapping web/modals/modal.react.js in a <FocusTrap> component would make all of our modals "trap focus". This is probably a good thing to do for all modals, and I think it would solve the problem here.

@kuba, what do you think?

kuba retitled this revision from [web] Disabling tab when in edit mode to [web] Added FocusTrap for modals.May 23 2023, 2:55 AM
kuba edited the summary of this revision. (Show Details)
kuba edited the test plan for this revision. (Show Details)

Fixed types

web/flow-typed/npm/focus-trap-react_vx.x.x.js
19 ↗(On Diff #26860)

Not sure if also need to add if we don't use it.

Here are TypeScript types for that:
https://github.com/focus-trap/focus-trap/blob/1dabbb3b45714aff2b17d750c7238a6a844dfe72/index.d.ts#L28

Should I add it? And if so, probably it should be in the focus-trap types, not focus-trap-react.

22–28 ↗(On Diff #26860)

Could we just add it to ModalOverlay so it applies to every modal automatically?

web/flow-typed/npm/focus-trap-react_vx.x.x.js
19 ↗(On Diff #26860)

After looking through I think most of them should be pretty easy to add here and we might want to customize the behaviour a bit in the future.

I don't think we will be using the bare focus-trap lib so it's probably fine to add them here (also I think flow has some problems with imports between libdefs?).

In a previous comment, I said:

... then wrapping web/modals/modal.react.js in a <FocusTrap> component would make all of our modals "trap focus". This is probably a good thing to do for all modals, and I think it would solve the problem here.

@kuba, what do you think?

However, in this diff you've only added FocusTrap to your specific modal. Can you provide some detail as to why you took a different approach, and whether you considered the approach I suggested?

Could we just add it to ModalOverlay so it applies to every modal automatically?

I might've gotten the location wrong – if ModalOverlay is the right place, we should do it there.

In a previous comment, I said:

... then wrapping web/modals/modal.react.js in a <FocusTrap> component would make all of our modals "trap focus". This is probably a good thing to do for all modals, and I think it would solve the problem here.

@kuba, what do you think?

However, in this diff you've only added FocusTrap to your specific modal. Can you provide some detail as to why you took a different approach, and whether you considered the approach I suggested?

I added it to both my modal, and modal.react.js.

Could we just add it to ModalOverlay so it applies to every modal automatically?

I might've gotten the location wrong – if ModalOverlay is the right place, we should do it there.

I considered adding it here. However, ModalOverlay is in lib/shared, and used in the landing page.

Checking how to add it to the lib workspace.

Moved FocusTrap to the ModalOverlay

lib/package.json
44 ↗(On Diff #26890)

Not 100% sure if this is enough, or should be added to the web and landing workspaces as well. I tested it locally, and it worked here.

lib/package.json
44 ↗(On Diff #26890)

Should be enough, but note that by including it here, you're also increasing the size of the React Native bundle (even though this package is not used there). It's probably fine

This revision is now accepted and ready to land.May 24 2023, 2:47 AM