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.

Sign up for GitHub

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

Jump to bottom

fix(modal, popover): fix focus-trap from preventing first click #6769

Merged
merged 7 commits into from Apr 12, 2023
Merged

fix(modal, popover): fix focus-trap from preventing first click #6769

merged 7 commits into from Apr 12, 2023
+31 −42

Conversation

driskull
Copy link
Member

@driskull driskull commented Apr 11, 2023 •
edited

Related Issue: #6581

Summary

  • Uses the component's host element for the focus trap. This seems to fix the issue in the related PR. I guess we should always use the host element in order to not run into any issues. Previously, it was using an element inside the shadowRoot.
  • Sets up a mutation observer to update focusable elements when new elements are slotted
  • Remove setting tabIndex on an element as a fallback

@driskull
fix(modal, popover): fix focus-trap from preventing first click #6581
a2559f6 
- Uses the component's host element for the focus trap
- Sets up a mutation observer to update focusable elements when new elements are slotted
- If no focusable elements are found it will just call setFocus on the component.
@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Apr 11, 2023
@driskull driskull marked this pull request as ready for review April 11, 2023 22:38
@driskull driskull requested a review from a team as a code owner April 11, 2023 22:38
jcfranco
jcfranco reviewed Apr 12, 2023
View reviewed changes
src/components/modal/modal.tsx Show resolved Hide resolved
src/components/popover/popover.tsx
@@ -255,6 +256,10 @@ export class Popover
//
// --------------------------------------------------------------------------

mutationObserver: MutationObserver = createObserver("mutation", () =>
Copy link
Member

Choose a reason for hiding this comment

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

As more components may need focus traps, we might need to provide a util to reuse a single mutation observer for all focus-traps.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. Should we do this part of this PR or later?

Copy link
Member

Choose a reason for hiding this comment

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

We can tackle this separately. #6668 may lead to similar components implementing focusTrapComponent too, so I could look into it for that one.

jcfranco
jcfranco approved these changes Apr 12, 2023
View reviewed changes
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️
⌨️🪤⌨️⌨️⌨️🪤⌨️⌨️🪤🪤⌨️⌨️🪤🪤🪤⌨️⌨️🪤🪤🪤⌨️🪤🪤🪤🪤⌨️
⌨️🪤🪤⌨️⌨️🪤⌨️🪤⌨️⌨️🪤⌨️⌨️🪤⌨️⌨️🪤⌨️⌨️⌨️⌨️🪤⌨️⌨️⌨️⌨️
⌨️🪤⌨️🪤⌨️🪤⌨️🪤⌨️⌨️🪤⌨️⌨️🪤⌨️⌨️🪤⌨️⌨️⌨️⌨️🪤🪤🪤⌨️⌨️
⌨️🪤⌨️⌨️🪤🪤⌨️🪤⌨️⌨️🪤⌨️⌨️🪤⌨️⌨️🪤⌨️⌨️⌨️⌨️🪤⌨️⌨️⌨️⌨️
⌨️🪤⌨️⌨️⌨️🪤⌨️⌨️🪤🪤⌨️⌨️🪤🪤🪤⌨️⌨️🪤🪤🪤⌨️🪤🪤🪤🪤⌨️
⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️⌨️

src/components/popover/popover.tsx
@@ -255,6 +256,10 @@ export class Popover
//
// --------------------------------------------------------------------------

mutationObserver: MutationObserver = createObserver("mutation", () =>
Copy link
Member

Choose a reason for hiding this comment

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

We can tackle this separately. #6668 may lead to similar components implementing focusTrapComponent too, so I could look into it for that one.

@jcfranco
Copy link
Member

This can't be easily tested with our setup, right? If so, can you add some info regarding testing?

@jcfranco
Copy link
Member

Remove setting tabIndex on an element as a fallback

Forgot to bring this up earlier, but can we emphasize in the focusTrapComponent doc about focusable content being a requirement for components implementing focus trapping?

@driskull
Copy link
Member Author

This can't be easily tested with our setup, right? If so, can you add some info regarding testing?

Correct, the best way to test is to use the codepen example.

https://codepen.io/mac_and_cheese/pen/rNZYxZE?editors=1000

  1. Close the modal that is open initially
  2. Open the modal using the button
  3. Type in the search until a suggestion appears
  4. Click the suggestion and make sure it navigates to that suggestion.

@driskull driskull merged commit be4a63a into master Apr 12, 2023
7 checks passed
@driskull driskull deleted the dris0000/focus-trap-update branch April 12, 2023 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcfranco jcfranco jcfranco approved these changes

Assignees

@driskull driskull

Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug.
Projects
None yet
Milestone
2023 April Priorities - No breaking c...
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@driskull @jcfranco