Page MenuHomePhabricator
Differential D7960

[web][fixed] Added FocusTrap for modals
ClosedPublic
Actions

Authored by • kuba on May 24 2023, 9:55 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 29, 1:22 AM
Unknown Object (File)
Sat, Mar 29, 1:15 AM
Unknown Object (File)
Tue, Mar 25, 1:19 PM
Unknown Object (File)
Sat, Mar 15, 5:23 PM
Unknown Object (File)
Mar 4 2025, 8:23 AM
Unknown Object (File)
Mar 4 2025, 8:23 AM
Unknown Object (File)
Mar 4 2025, 8:22 AM
Unknown Object (File)
Mar 4 2025, 8:22 AM
View All Files
Subscribers
ashoat
atul
tomek

Details

Reviewers
michal
inka
ashoat
ginsu
Commits
rCOMM0c8d23999927: [web][fixed] Added FocusTrap for modals
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
Branch
kuba/test-trap
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• kuba created this revision.May 24 2023, 9:55 AM
• kuba edited the summary of this revision. (Show Details)May 24 2023, 9:58 AM
• kuba edited the test plan for this revision. (Show Details)
• kuba added inline comments.
lib/components/modal-overlay.react.js
63

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 a reviewer: ginsu.May 24 2023, 10:13 AM
ashoat added inline comments.
lib/components/modal-overlay.react.js
63

All object params should be memoized in function components

Harbormaster completed remote builds in B19745: Diff 27004.May 24 2023, 10:14 AM
• kuba requested review of this revision.May 24 2023, 10:14 AM
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 updated this revision to Diff 27039.May 25 2023, 1:30 AM
• kuba marked an inline comment as done.
• kuba edited the summary of this revision. (Show Details)

Memoize param

Harbormaster completed remote builds in B19758: Diff 27039.May 25 2023, 1:48 AM
ashoat accepted this revision.May 25 2023, 3:58 AM
This revision is now accepted and ready to land.May 25 2023, 3:58 AM
Closed by commit rCOMM0c8d23999927: [web][fixed] Added FocusTrap for modals (authored by kosmydel <kukubaczek@gmail.com>). · Explain WhyMay 25 2023, 4:43 AM
This revision was automatically updated to reflect the committed changes.
kosmydel <kukubaczek@gmail.com> added a commit: rCOMM0c8d23999927: [web][fixed] Added FocusTrap for modals.

Revision Contents
Changeset List

PathSize
lib/
components/
26 lines
flow-typed/
npm/
61 lines
1 line
20 lines

Diff 27004

View Options

lib/components/modal-overlay.react.js

Show First 20 Lines • Show All 55 Lines • ▼ Show 20 Lines networks:
- services-net - services-net
build: build:
dockerfile: services/identity/Dockerfile dockerfile: services/identity/Dockerfile
context: ../ context: ../
image: commapp/identity-server:0.1 image: commapp/identity-server:0.1
container_name: identity-server container_name: identity-server
ports: ports:
- "${COMM_SERVICES_PORT_IDENTITY}:50051" - "${COMM_SERVICES_PORT_IDENTITY}:50051"
# localstack
localstack:
image: localstack/localstack
container_name: localstack
hostname: localstack
ports:
- "4566:4566"
environment:
- SERVICES=s3,dynamodb
- DATA_DIR=${TMPDIR}/localstack/data
- HOSTNAME_EXTERNAL=localstack
volumes:
jimUnsubmitted
Not Done
Inline Actions

Why bind mount a temp directory instead of using a data volume? It's just gonna create issues with permissions. Also ${TMPDIR} is not used to refer to a path in both the host and the container which is a bad idea.

jim: Why bind mount a temp directory instead of using a data volume? It's just gonna create issues…
- ${TMPDIR}:/tmp/localstack
networks:
- services-net
View Options

lib/flow-typed/npm/focus-trap-react_vx.x.x.js

Show First 20 Lines • Show All 55 Lines • ▼ Show 20 Lines networks:
- services-net - services-net
build: build:
dockerfile: services/identity/Dockerfile dockerfile: services/identity/Dockerfile
context: ../ context: ../
image: commapp/identity-server:0.1 image: commapp/identity-server:0.1
container_name: identity-server container_name: identity-server
ports: ports:
- "${COMM_SERVICES_PORT_IDENTITY}:50051" - "${COMM_SERVICES_PORT_IDENTITY}:50051"
# localstack
localstack:
image: localstack/localstack
container_name: localstack
hostname: localstack
ports:
- "4566:4566"
environment:
- SERVICES=s3,dynamodb
- DATA_DIR=${TMPDIR}/localstack/data
- HOSTNAME_EXTERNAL=localstack
volumes:
jimUnsubmitted
Not Done
Inline Actions

Why bind mount a temp directory instead of using a data volume? It's just gonna create issues with permissions. Also ${TMPDIR} is not used to refer to a path in both the host and the container which is a bad idea.

jim: Why bind mount a temp directory instead of using a data volume? It's just gonna create issues…
- ${TMPDIR}:/tmp/localstack
networks:
- services-net
View Options

lib/package.json

Show First 20 Lines • Show All 55 Lines • ▼ Show 20 Lines networks:
- services-net - services-net
build: build:
dockerfile: services/identity/Dockerfile dockerfile: services/identity/Dockerfile
context: ../ context: ../
image: commapp/identity-server:0.1 image: commapp/identity-server:0.1
container_name: identity-server container_name: identity-server
ports: ports:
- "${COMM_SERVICES_PORT_IDENTITY}:50051" - "${COMM_SERVICES_PORT_IDENTITY}:50051"
# localstack
localstack:
image: localstack/localstack
container_name: localstack
hostname: localstack
ports:
- "4566:4566"
environment:
- SERVICES=s3,dynamodb
- DATA_DIR=${TMPDIR}/localstack/data
- HOSTNAME_EXTERNAL=localstack
volumes:
jimUnsubmitted
Not Done
Inline Actions

Why bind mount a temp directory instead of using a data volume? It's just gonna create issues with permissions. Also ${TMPDIR} is not used to refer to a path in both the host and the container which is a bad idea.

jim: Why bind mount a temp directory instead of using a data volume? It's just gonna create issues…
- ${TMPDIR}:/tmp/localstack
networks:
- services-net
View Options

yarn.lock

Show First 20 Lines • Show All 55 Lines • ▼ Show 20 Lines networks:
- services-net - services-net
build: build:
dockerfile: services/identity/Dockerfile dockerfile: services/identity/Dockerfile
context: ../ context: ../
image: commapp/identity-server:0.1 image: commapp/identity-server:0.1
container_name: identity-server container_name: identity-server
ports: ports:
- "${COMM_SERVICES_PORT_IDENTITY}:50051" - "${COMM_SERVICES_PORT_IDENTITY}:50051"
# localstack
localstack:
image: localstack/localstack
container_name: localstack
hostname: localstack
ports:
- "4566:4566"
environment:
- SERVICES=s3,dynamodb
- DATA_DIR=${TMPDIR}/localstack/data
- HOSTNAME_EXTERNAL=localstack
volumes:
jimUnsubmitted
Not Done
Inline Actions

Why bind mount a temp directory instead of using a data volume? It's just gonna create issues with permissions. Also ${TMPDIR} is not used to refer to a path in both the host and the container which is a bad idea.

jim: Why bind mount a temp directory instead of using a data volume? It's just gonna create issues…
- ${TMPDIR}:/tmp/localstack
networks:
- services-net