fix: add shadow dom support for portal provider (S2 v1.0.0, S1 v3.46.0) #9369
Conversation
bsmiley1
changed the title
|
Thanks for the PR, I know you said some more things are coming including a detailed description of the changes. Can you include any references to existing/prior work? I'm starting to refresh myself on all of this work now that our release and docs are out. A bit unfortunate that it's also time for the holiday shutdown, but I am spending some time on this topic this week and a little bit of next week. |
| colorScheme: props.colorScheme ?? colorScheme ?? Object.keys(theme).filter(k => k === 'light' || k === 'dark').join(' ') | ||
| }; | ||
|
|
||
| // Skip isolation: isolate when rendering in a shadow root to prevent |
There was a problem hiding this comment.
do you have an example of how this interferes? I know in the PDF it has an ascii art describing the issue, but it'd be good to see this one in practice. Feel free to make a storybook story for it or a chromatic story so that we can verify the behaviour
| state.close(); | ||
| } : undefined); | ||
|
|
||
| // Add interact outside handling for the popover to support Shadow DOM contexts |
There was a problem hiding this comment.
does this fix: ComboBox Popover does not close when clicking on a sibling GridList component or Can't click ModalOverlay to close an open Combobox
Seems tangentially related to shadow dom possibly but also happens outside of shadow doms?
| let getVisualViewport = () => typeof document !== 'undefined' ? window.visualViewport : null; | ||
|
|
||
| function getContainerDimensions(containerNode: Element, visualViewport: VisualViewport | null): Dimensions { | ||
| function getContainerDimensions(containerNode: Element, visualViewport: VisualViewport | null, containerBounds?: DOMRect | null): Dimensions { |
There was a problem hiding this comment.
note to self, I want to move changes to calculate position to a separate PR since they are so complicated
also, with baseline support for anchor positioning, we should check if that solves these issues so that we don't need to make any changes to calculate position if at all possible
| <PortalContext.Provider | ||
| value={{ | ||
| getContainer: getContainer === null ? undefined : getContainer ?? ctxGetContainer, | ||
| getContainerBounds: getContainerBounds === null ? undefined : getContainerBounds ?? ctxGetContainerBounds |
There was a problem hiding this comment.
hopefully unneeded, but otherwise, see if we can use boundaryElement instead of introducing a new/different way of doing boundaries
|
|
||
| let visualViewport = typeof document !== 'undefined' && window.visualViewport; | ||
|
|
||
| // Lazy import to avoid circular dependency issues |
There was a problem hiding this comment.
we can't really do lazy imports in our source, instead it should be structured such that users can choose how to code split if they want and necessary things are bundled together
what is the circular dependency issue?
| export function useViewportSize(): ViewportSize { | ||
| let isSSR = useIsSSR(); | ||
| let [size, setSize] = useState(() => isSSR ? {width: 0, height: 0} : getViewportSize()); | ||
| let portalModule = getPortalContext(); |
There was a problem hiding this comment.
I don't think that useViewportSize should be changed to return the boundaries of a container. Then the name no longer makes sense and it's functionality isn't what it was intended for.
Instead, we should make use of the boundaryElement if possible, did you try that? and what issues did you run into?
| width: typeVariant === 'fullscreen' ? 'calc(100% - 80px)' : '100%', | ||
| height: typeVariant === 'fullscreen' ? 'calc(100% - 80px)' : '100%' |
There was a problem hiding this comment.
shouldn't need width/height if top/left/right/bottom are specified?
| // This ensures modals are centered within the web component's bounds, not the full viewport | ||
| applyContainerBounds(wrapperStyle, containerBounds, {center: true}); | ||
|
|
||
| // For fullscreen modals, override the fixed positioning to be relative to the wrapper |
There was a problem hiding this comment.
I don't think this is correct, it should be relative to the full screen, not to any container. You are not allowed to move focus out of a dialog, so you can't interact with the Shell anyways. If anything, the shell should be following the same pattern that UnifiedShell follows and it should have an API to open a modal for the sub-app which goes over everything.
It also does not align with the name "fullscreen"
If you need something which covers all of your shadow dom but you want it to have the appearance of an overlay just over that area, I suggest making a custom component which is in the normal dom of your app, apply inert to your background and render your "overlay" just after it as a sibling but position it over the inert stuff. Then it can just be a normal part of your app as if you'd switched tabs or something and you can still interact with the top bar.
2 participants
This PR attempts to expand Shadow DOM support by allowing portal components to be opened within a shadow root through the PortalProvider context api.
PDF for a more detailed description of the changes:
REACT_SPECTRUM_SHADOW_DOM_FIXES.pdf
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: