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.

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

fix: Disable cross-frame focus lock in <Modal> #840

Merged
merged 1 commit into from
Aug 30, 2024

Conversation

pawelgrimm
Copy link
Contributor

@pawelgrimm pawelgrimm commented Aug 29, 2024

Short description

When using <Modal> in a Storybook story, controls would stop working. This was determined to be caused by the <FocusLock> component, which would prevent interaction with Storybooks preview/add-ons pane once focus was established within the modal (which would happen automatically as autoFocus defaults to true). To fix this, we're limiting <FocusLock>'s focus trap to the document/iframe the Modal is contained within, preventing it from bleeding over into Storybook's preview pane.

PR Checklist

  • Added tests for bugs / new features
  • Updated docs (storybooks, readme)
  • Executed npm run validate and made sure no errors / warnings were shown
  • Described changes in CHANGELOG.md
  • Bumped version in package.json and package-lock.json (npm --no-git-tag-version version <major|minor|patch>) ref
  • Reviewed and approved Chromatic visual regression tests in CI

Versioning

@pawelgrimm
Copy link
Contributor Author

@gnapse @frankieyan Do you forsee any issues with this approach? It's been a LONG time since we last touched this code (#591 (comment)), but I thought I'd check with y'all.

@pawelgrimm pawelgrimm marked this pull request as ready for review August 30, 2024 00:34
@pawelgrimm pawelgrimm force-pushed the pawel/modal/disable-cross-frame-focus-lock branch from a3e0379 to f292977 Compare August 30, 2024 00:34
@frankieyan
Copy link
Member

I just realized that the focus lock is actually causing problems for us right now

If you open up a template's task detail in https://todoist.com/templates/todoist-ceo-setup (you'll have to use a browser window without a Todoist logged in session), and once you focus on its elements, you cannot focus on any of the host page's elements anymore, similar to what we're seeing in Storybook.

While we do allow Todoist to be embedded (we have no X-Frame-Options header), I'm not aware of any situations where we'd want to take over control from the host page, so I'd imagine this change to be safe to make 👍

@pawelgrimm pawelgrimm changed the title fix: Disable cross-frame focus lock fix: Disable cross-frame focus lock in <Modal> Aug 30, 2024
@pawelgrimm pawelgrimm force-pushed the pawel/modal/disable-cross-frame-focus-lock branch from f292977 to c0550c0 Compare August 30, 2024 16:05
@pawelgrimm pawelgrimm self-assigned this Aug 30, 2024
@pawelgrimm pawelgrimm merged commit e2e97b4 into main Aug 30, 2024
5 checks passed
@pawelgrimm pawelgrimm deleted the pawel/modal/disable-cross-frame-focus-lock branch August 30, 2024 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants