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

check whether document.body.addEventListener exist #2387

Closed
wants to merge 1 commit into from
Closed

check whether document.body.addEventListener exist #2387

wants to merge 1 commit into from

Conversation

KairuiLiu
Copy link
Contributor

if Dialog component is used in shadowDOM, browser will throw an error

Uncaught TypeError: Cannot read properties of null (reading 'addEventListener')
    at ../node_modules/.pnpm/@headlessui+react@1.7.13_biqbaboplfbrettd7655fr4n2y/node_modules/@headlessui/react/dist/components/focus-trap/focus-trap.js

which is because document.body === null, but we only check whether document.body === undefined

@vercel
Copy link

vercel bot commented Mar 20, 2023 •
edited
Loading

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

Name Status Preview Comments Updated
headlessui-react ❌ Failed (Inspect) Mar 20, 2023 at 6:02PM (UTC)

@vercel
Copy link

vercel bot commented Mar 20, 2023

@KairuiLiu is attempting to deploy a commit to the Tailwind Labs Team on Vercel.

A member of the Team first needs to authorize it.

@thecrypticace thecrypticace self-assigned this Mar 21, 2023
@thecrypticace
Copy link
Contributor

As far as I can tell document.body is still available in Shadow DOM contexts. Can you provide a reproduction showcasing that this is necessary?

@KairuiLiu
Copy link
Contributor Author

KairuiLiu commented Mar 21, 2023 •
edited
Loading

In many website, there are no <body> in shadowDOM, just like this. document.body === null. So,

image

so replace document.body.addEventListener('click', handle, { capture: true }) with document.addEventListener('click', handle, { capture: true }) seems a better solution?

@thecrypticace
Copy link
Contributor

document in the Shadow DOM does not point to the Shadow DOM itself but to the owning document. In which case there is always a body element present but document.body is not available until after the DOM has parsed the opening <body> tag. This is likely a loading order problem where the script needs to be defer-ed.

@RobinMalfait Should we wrap history listener setup in a DOMContentLoaded listener or require users to use defer / place script tags at the end of the body tag?

@KairuiLiu KairuiLiu mentioned this pull request Mar 21, 2023
Merged
@KairuiLiu
Copy link
Contributor Author

document in the Shadow DOM does not point to the Shadow DOM itself but to the owning document. In which case there is always a body element present but document.body is not available until after the DOM has parsed the opening <body> tag. This is likely a loading order problem where the script needs to be defer-ed.

@RobinMalfait Should we wrap history listener setup in a DOMContentLoaded listener or require users to use defer / place script tags at the end of the body tag?

delay the event listener to document ready seems a good idea see: #2389

@RobinMalfait
Copy link
Member

@thecrypticace if the delay is safe and guaranteed than this is good in my opinion. I don't think we want the ability to customize this behaviour.

@thecrypticace
Copy link
Contributor

@RobinMalfait It is. It may have fired before script runs but if it has then the readyState of the document will no longer be loading in which case we can run the code immediately.

Closing in favor of #2389

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@thecrypticace thecrypticace

Labels
Needs more info
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@KairuiLiu @thecrypticace @RobinMalfait