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

feat(DocumentContext): Add support for React Portals in new windows & document picture-in-picture #3357

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

arturbien
Copy link

@arturbien arturbien commented Feb 3, 2025

Description

This PR adds support for using Radix Primitives in a new window popups and Document Picture in Picture.

Screen.Recording.2025-02-03.at.14.28.30.mov

Use cases

One of the common use cases are popup windows and Document Picture-in-Picture . Document Picture-in-Picture API is used for example by apps like Google Meet where users can enter PIP mode with livestream and chat portalled to a floating window.

This is a use case we're currently facing at Whop, we're building a live-streaming app with Document PIP support.

Why doesn't it work currently in Radix

Since JS and React is running in the parent window, and bunch of components rely on calling document, they break when being portalled to a new document. For example when you try to open <Dialog /> or <Tooltip /> in a new document, it will actually open in the parent window, because all event listeners and portals are attached to the parent window.

Solution

There were attempts at fixing this before , but the solutions mostly relied on tweaking individual components instead of creating a more global and simpler solution:
#1721
#1676
#1677
#1845

This PR adds an optional <DocumentContext.Provider value={documentElement} /> component where users can pass reference to the popup window document, and useDocument() hook which returns document passed to the provider with a fallback to globalThis.document. The hook is used internally within all primitives that rely on the document. All that user has to do to make Radix work with portals to another window is to wrap portalled content with the provider like so:

createPortal(
   <DocumentContext.DocumentProvider document={portalElement.ownerDocument}>
        {content} // Radix components here
   </DocumentContext.DocumentProvider>,
   portalElement
 )

This PR is not introducing any breaking changes or new props to any of the modified components. The provider is completely optional.

TODO:

  • Swap document references with providedDocument
  • Swap window references with providedDocument.defaultView
  • Storybook example
  • SSR page example
  • Tests
  • Bump package versions (minor or or patch?) for all updated primitives
  • Add <DocumentContext /> page to the docs website

(cc @chaance @benoitgrelard @hugotiger @seleckis @2DTW @gregorybolkenstijn @william-will-angi @LordZardeck @ritz078 @joaodematte)

Closes #1721
Closes #1715

@arturbien arturbien marked this pull request as ready for review February 3, 2025 14:14
@chaance
Copy link
Member

chaance commented Feb 3, 2025

Thanks for this! It's been on my todo list for a while to fix document references for a while.

Curious though why this needs to be a separate context and not a utility function that checks the ownerDocument of a given ref node. I feel like there's a good bit of bloat with the React tree in Radix components as-is, so if we can accomplish this without a new provider I think that would be simpler and a bit more efficient. Thoughts?

@arturbien
Copy link
Author

arturbien commented Feb 3, 2025

@chaance thanks for quick response! 🫡

Curious though why this needs to be a separate context and not a utility function that checks the ownerDocument of a given ref node

  • Not every component renders an actual HTML element (e.g., Portal, DescriptionWarning, FocusGuards), making it difficult to reliably access a DOM reference. A utility function wouldn’t work in cases where there’s no direct element to check.

  • Some components might rely on document even before returning anything (e.g., attaching event listeners, reading window size before returning, using MutationObserver/ResizeObserver, or reading DOM attributes). The provider approach is more future-proof since it ensures access to the correct document in all scenarios.

  • I think utility function approach would require additional prop drilling and managing refs across multiple components, adding unnecessary complexity.

  • useDocument improves DX. In our case at Whop, the same component is rendered both on the main page and in a new window via a portal. Some of its child components (made by us) also rely on document (for event listeners etc), so accessing it via a hook is far more convenient than manually handling refs. Otherwise, developers would have to implement their own DocumentProvider anyway.

  • It's easier to explain how to build PIP and popup windows with Radix - "just wrap portalled content in provider, then use useDocument whenever you need access to the relavant document). Otherwise a developer will build let's say a Dialog that renders their own custom component that relies on the document, and we'll get bunch of questions about listeners being attached to the wrong window. Exposing and documenting a hook for getting relevant document solves this

I feel like there's a good bit of bloat with the React tree in Radix components as-is

  • The provider is optional and only needed in specific cases like popups and Document Picture-in-Picture. For most users, it won’t introduce any additional React tree complexity.

@chaance
Copy link
Member

chaance commented Feb 3, 2025

All valid points when it comes to DX inside of Radix itself but we've made similar decisions with that mindset and I think it's been at the expense of both DX on the consumer's side (the Raect Dev tools components tab is a giant mess) and end user performance. I'd like to recalibrate how we think about and prioritize DX in general.

Might not have time to dig into this in depth right away. Competing priorities and whatnot. It's on my TODO list.

@chaance chaance self-requested a review February 3, 2025 18:05
@chaance chaance self-assigned this Feb 3, 2025
@arturbien
Copy link
Author

arturbien commented Feb 3, 2025

All valid points when it comes to DX inside of Radix itself but we've made similar decisions with that mindset and I think it's been at the expense of both DX on the consumer's side (the Raect Dev tools components tab is a giant mess) and end user performance. I'd like to recalibrate how we think about and prioritize DX in general.

Might not have time to dig into this in depth right away. Competing priorities and whatnot. It's on my TODO list.

Not sure I understand the impact of the DX on the consumer side though 🤔 - the provider will only ever be used to wrap components passed to ReactDom.createPortal() and only for the niche use case for cross-document popups and DPIP like:

ReactDOM.createPortal(
  <DocumentContext.Provider value={popupWindow.document}>{content} </DocumentContext.Provider>,
  popupWindow.document.body
)

So 99% of consumers won't ever have to use the provider in their apps.

From the performance POV - the value passed to the provider won't ever change so it won't be causing any unnecessary rerenders.

I'm happy to explore other options too, just wanted to better understand your concerns.🫡

@chaance
Copy link
Member

chaance commented Feb 4, 2025

@arturbien to be clear, I haven't reviewed in depth yet so take my feedback at this stage with a grain of salt :) Initial reaction was assuming that the provider would be used internally but that does not appear to be the case, so I'm open to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[All] Ensure all document handlers are attached to ownerDocument
2 participants