Details

Reviewers

michal
inka

Commits

rCOMM51a753adf418: [web] Added FocusTrap for modals

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

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• kuba created this revision.May 16 2023, 5:43 AM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptMay 16 2023, 5:43 AM

• kuba edited the summary of this revision. (Show Details)May 16 2023, 5:43 AM

• kuba added a parent revision: D7836: [web] Displaying modal of editing message GUI.

• kuba added a child revision: D7838: [web] Disabled auto focus on overlay when entering edit mode.May 16 2023, 5:45 AM

Harbormaster completed remote builds in B19391: Diff 26529.May 16 2023, 6:00 AM

• kuba requested review of this revision.May 16 2023, 6:00 AM

ashoat added inline comments.May 16 2023, 6:13 AM

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?

Adressed review

Harbormaster completed remote builds in B19400: Diff 26543.May 16 2023, 7:21 AM

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?

Rebase

Harbormaster completed remote builds in B19480: Diff 26645.May 18 2023, 8:38 AM

Added FocusTrap for modals

• 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)

Added newline

Harbormaster completed remote builds in B19644: Diff 26852.May 23 2023, 3:08 AM

Harbormaster completed remote builds in B19645: Diff 26853.May 23 2023, 3:17 AM

Added types for FocusTrap

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)	Here are `TypeScript` types of that: https://github.com/focus-trap/focus-trap-react/blob/a4691484a26dfd729a03d2e6c0cc1a1af025e565/index.d.ts#LL7C64-L7C64

Harbormaster completed remote builds in B19651: Diff 26859.May 23 2023, 4:04 AM

Harbormaster completed remote builds in B19652: Diff 26860.May 23 2023, 4:09 AM

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:

In D7837#233153, @ashoat wrote:

... 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?

In D7837#235208, @michal wrote:

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 D7837#235331, @ashoat wrote:

In a previous comment, I said:

In D7837#233153, @ashoat wrote:

... 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.

In D7837#235208, @michal wrote:

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.

• kuba planned changes to this revision.May 23 2023, 6:35 AM

Moved FocusTrap to the ModalOverlay

• kuba added inline comments.May 23 2023, 7:35 AM

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.

Harbormaster completed remote builds in B19682: Diff 26890.May 23 2023, 7:52 AM

ashoat added inline comments.May 23 2023, 11:28 AM

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