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

Add observer to focus-trap #429

Merged
merged 10 commits into from
Aug 23, 2024
Merged

Conversation

TylerJDev
Copy link
Contributor

@TylerJDev TylerJDev commented Aug 6, 2024

Summary

Adds a mutation observer to focus-trap that ensures that the sentinel elements stay at the start/end. The observer only checks for added elements that are direct descendants of the container, meaning that children of those descendants shouldn't trigger the observer.


New Tests:

  • Added a test to verify that sentinel elements remain at the start and end of the inner container when new elements are added. (src/__tests__/focus-trap.test.tsx)
  • Added a test to ensure the mutation observer is removed when the focus trap is released. (src/__tests__/focus-trap.test.tsx)

Focus Trap Enhancements:

  • Implemented observeFocusTrap function to create a MutationObserver that maintains sentinel elements at the correct positions. (src/focus-trap.ts)
  • Integrated the observeFocusTrap function into the focusTrap function to observe changes in the container and reposition sentinel elements as needed. (src/focus-trap.ts)
  • Ensured the MutationObserver is disconnected when the focus trap is released to prevent memory leaks. (src/focus-trap.ts)

Copy link

changeset-bot bot commented Aug 6, 2024

🦋 Changeset detected

Latest commit: 07e1664

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/behaviors Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@TylerJDev TylerJDev marked this pull request as ready for review August 7, 2024 13:45
@TylerJDev TylerJDev requested a review from a team as a code owner August 7, 2024 13:45
@siddharthkp
Copy link
Member

Let's test the canary version with primer/react#4779 to make sure we don't have any performance regressions

@jonrohan
Copy link
Member

gonna leave this review to @siddharthkp

@jonrohan jonrohan removed their request for review August 19, 2024 17:34
@TylerJDev
Copy link
Contributor Author

@siddharthkp - Looks like there shouldn't be any big performance regressions. I did not do an in-depth analysis, but I can confirm that the mutation observer should only operate under the following conditions:

  • Element was added before start sentinel
  • Element was added after end sentinel
  • Element is added directly as a child of <Dialog>
    • If a child is added to the body, footer, or header subcomponents, it won't notify the mutation observer

In either case, we only re-append the sentinel to the start/end depending on which sentinel needs to be adjusted. Curious on what you think!

Screen.Recording.2024-08-21.at.2.01.20.PM.mov

Video description: Showcasing items being appended in Dialog's body and before/after the sentinels. Shows me enabling/disabling the focus trap, ensuring the mutation observer is disconnected properly.

siddharthkp

This comment was marked as outdated.

Copy link
Member

@siddharthkp siddharthkp 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 to me!

Let's try this with PRC to test further

Copy link
Member

@siddharthkp siddharthkp left a comment

Choose a reason for hiding this comment

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

Let's go!

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.

3 participants