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(Tearsheet): avoid focus traps with stacked tearsheets #1360

Merged
merged 4 commits into from
Nov 1, 2021

Conversation

dcwarwick
Copy link
Contributor

@dcwarwick dcwarwick commented Oct 28, 2021

Closes #1332

This PR:

  • adds tearsheets to the modal exit selector list so that stacking tearsheets do not "trap" the focus in the first tearsheet.
  • catches focus entry into a non-topmost tearsheet and transfers focus to the topmost tearsheet.
  • checks as each tearsheet becomes topmost that focus is within the tearsheet, and if not claims focus.

This should avoid the focus trap issue and also clean up the handling of focus when multiple tearsheets are being opened and closed.

What did you change?

Tearsheet.js, also added to the stacked tearsheet stories so the issue can be verified. Also updated the tests to expect warnings from Carbon -- these should eventually be removed (see carbon-design-system/carbon#10000).

How did you test and verify your work?

Ran storybook.

@dcwarwick dcwarwick requested a review from a team as a code owner October 28, 2021 23:48
@netlify
Copy link

netlify bot commented Oct 28, 2021

✔️ Deploy Preview for ibm-cloud-cognitive ready!

🔨 Explore the source changes: 36fd7e4

🔍 Inspect the deploy log: https://app.netlify.com/sites/ibm-cloud-cognitive/deploys/617dd73d2b73ba00088521c4

😎 Browse the preview: https://deploy-preview-1360--ibm-cloud-cognitive.netlify.app

@dcwarwick dcwarwick marked this pull request as draft October 28, 2021 23:57
@dcwarwick dcwarwick marked this pull request as ready for review October 31, 2021 00:06
@dcwarwick
Copy link
Contributor Author

@matthewgallo @lee-chase I looked at the approach to this that replaced the ComposedModal with plain <div>s except for the topmost tearsheet -- ingenious, but I couldn't fix the animation problem. Basically, when React switches the rendering from the ComposedModal render method to the fragment is rerenders each side of an animation frame, so the transitions will not trigger correctly. I did explore first replacing the modal with <div>s in the OLD state, then after a delay switching the classes to try to get the transitions to occur, but it was becoming very fiddly.

Fortunately I found this alternative, which uses a rather obscure prop selectorsFloatingMenus. So obscure, in fact, that I think no one else has ever used it, because it actually has several bugs! I've opened carbon-design-system/carbon#10000 on Carbon to resolve those bugs. Fortunately, they only cause console warnings in non-production builds, so this is still a viable fix in the mean time. I've updated the tests to expect those warnings, and if/when Carbon is fixed we can remove those updates :-)

Copy link
Member

@matthewgallo matthewgallo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad you found a work around for this! 🥇

@matthewgallo matthewgallo merged commit 17f48df into carbon-design-system:main Nov 1, 2021
@dcwarwick dcwarwick deleted the issue1332 branch November 2, 2021 15:17
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.

Focus Trap issue with Stacked Tearsheet
2 participants