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 shadow-root bug closing Dialog containers #2217

Merged
merged 2 commits into from
Jan 27, 2023
Merged

Conversation

RobinMalfait
Copy link
Member

@RobinMalfait RobinMalfait commented Jan 27, 2023

We added better support for shadow roots being valid containers for the Dialog component in #2079. This means that if you have a 3rd party (or 1st party) tool that uses the shadow DOM, then the Dialog used to close when interacting with those items.

This was solved by @thecrypticace in the #2079 PR.

But I found that there is a bug here with one of these:

  • Clicks from shadow roots inside the panel (keeps dialog open)
  • Clicks from shadow roots outside the panel (closes the dialog)

There is an issue with the Grammarly extension that injects a shadow root component inside the Dialog.Panel. Yet, clicking on it closes the Dialog. This issue occurs on Campsite for example (by @brianlovin).

I can make it work by always allowing shadow roots with (before checking all the allowed containers):

// Ignore if the target is in a shadow root
if (target.getRootNode() !== document) return

The tree looks like this:
image

Looking at the <grammarly-extensions> if I log $0.shadowRoot I get the Shadow Content (open) log.

Looking at our check:

console.log(event.composed, event.composedPath().includes(domNode as EventTarget))

It results in true and false. The last value being false is what breaks things and is the confusing part. It looks like the parent of the <grammarly-extension> is the <grammarly-mirror> and then immediately the html
image

So we never saw the Dialog.Panel element and therefore closing the Dialog...

Time to dig deeper!

Well it turns out that the bug is very stupid actually and the solution is luckily very simple. Grammarly doesn't inject its container in the <body> but as a sibling of <body> so we don't have any valid containers that contain this...
image

So.. let's include html > * except for the body and head itself as valid containers and lo and behold:
image

Fixes: #1832

@vercel
Copy link

vercel bot commented Jan 27, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
headlessui-react ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 27, 2023 at 0:00AM (UTC)
headlessui-vue ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 27, 2023 at 0:00AM (UTC)

@brianlovin
Copy link

Wow, thank you for digging so deep into this problem @RobinMalfait!

@RobinMalfait
Copy link
Member Author

@brianlovin

You can already try it using npm install @headlessui/react@insiders can you try and play with it and see if everything seems to work for you now?

@brianlovin
Copy link

Testing this locally now and it works great! Any ETA on when this will officially ship?

@RobinMalfait
Copy link
Member Author

I'll get a release out by the end of my day for you. Should be within ~2-3 hours.

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