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

2575: Focus trap bottom action sheet #2576

Merged
merged 3 commits into from
Nov 23, 2023

Conversation

f1sh1918
Copy link
Contributor

@f1sh1918 f1sh1918 commented Nov 20, 2023

Short description

Focus trap lib since 10.x can not detect nodes from a portal (bottom action sheet) and will throw an error (opening feedback modal)

Proposed changes

  • added a legacy displayCheck for finding the node in the portal

Side effects

Resolved issues

Fixes: #2575


@f1sh1918 f1sh1918 force-pushed the 2575-focus-trap-bottom-action-sheet branch from 5f069a5 to 36d41e3 Compare November 20, 2023 18:24
Copy link
Member

@steffenkleinle steffenkleinle left a comment

Choose a reason for hiding this comment

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

Tested on firefox

@@ -62,9 +62,9 @@ const Modal = ({ title, closeModal, children, direction, wrapInPortal = false }:

return () => layoutElement?.setAttribute('aria-hidden', 'false')
}, [])

// display check option is needed for portals - https://github.com/focus-trap/tabbable/blob/master/CHANGELOG.md#532
Copy link
Member

Choose a reason for hiding this comment

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

I think you linked the wrong release, shouldnt it be https://github.com/focus-trap/tabbable/blob/master/CHANGELOG.md#600?

It says

restore old (incorrect) behavior

Should we investigate a better fix?

Copy link
Contributor Author

@f1sh1918 f1sh1918 Nov 21, 2023

Choose a reason for hiding this comment

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

Gonna adjust the link.
I just investigated some time and i didn't find an easy solution. The trap works well with this option, so no idea if its really worth to investigate more time @steffenkleinle

Copy link
Member

@ztefanie ztefanie left a comment

Choose a reason for hiding this comment

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

Looks good.

@f1sh1918 f1sh1918 merged commit ebee697 into main Nov 23, 2023
8 checks passed
@f1sh1918 f1sh1918 deleted the 2575-focus-trap-bottom-action-sheet branch November 23, 2023 12:36
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.

Error Open Feedback in BottomActionSheet
3 participants