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

re-add shadow DOM styles when reparented #2252

Merged
merged 9 commits into from
Sep 20, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 29 additions & 13 deletions packages/itwinui-react/src/utils/components/ShadowRoot.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,24 @@ function useShadowRoot(
const latestCss = useLatestRef(css);
const latestShadowRoot = useLatestRef(shadowRoot);

const createStyleSheet = React.useCallback(() => {
const shadow = latestShadowRoot.current;

if (shadow && supportsAdoptedStylesheets) {
// create an empty stylesheet and add it to the shadowRoot
const currentWindow = shadow.ownerDocument.defaultView || globalThis;
if (!(styleSheet.current instanceof currentWindow.CSSStyleSheet)) {
styleSheet.current = new currentWindow.CSSStyleSheet();
shadow.adoptedStyleSheets.push(styleSheet.current);
}

// add the CSS immediately to avoid FOUC (one-time)
if (latestCss.current) {
styleSheet.current.replaceSync(latestCss.current);
}
}
}, [latestCss, latestShadowRoot]);

useLayoutEffect(() => {
const parent = templateRef.current?.parentElement;
if (!parent) {
Expand All @@ -94,31 +112,21 @@ function useShadowRoot(

const shadow = parent.shadowRoot || parent.attachShadow({ mode: 'open' });

if (supportsAdoptedStylesheets) {
// create an empty stylesheet and add it to the shadowRoot
const currentWindow = shadow.ownerDocument.defaultView || globalThis;
styleSheet.current = new currentWindow.CSSStyleSheet();
shadow.adoptedStyleSheets = [styleSheet.current];

// add the CSS immediately to avoid FOUC (one-time)
if (latestCss.current) {
styleSheet.current.replaceSync(latestCss.current);
}
}

// Flush the state immediately after shadow-root is attached, to ensure that layout
// measurements in parent component are correct.
// Without this, the parent component may end up measuring the layout when the shadow-root
// is attached in the DOM but React hasn't rendered any slots or content into it yet.
ReactDOM.flushSync(() => setShadowRoot(shadow));

createStyleSheet();
};

queueMicrotask(() => {
setupOrReuseShadowRoot();
});

return () => void setShadowRoot(null);
}, [templateRef, latestCss, latestShadowRoot]);
}, [templateRef, createStyleSheet, latestShadowRoot]);

// Synchronize `css` with contents of the existing stylesheet
useLayoutEffect(() => {
Expand All @@ -127,5 +135,13 @@ function useShadowRoot(
}
}, [css]);

// Re-create stylesheet if the element is moved to a different window (by AppUI)
React.useEffect(() => {
window.addEventListener('appui:reparent', createStyleSheet);
return () => {
window.removeEventListener('appui:reparent', createStyleSheet);
};
}, [createStyleSheet, latestShadowRoot]);
Copy link
Contributor Author

@mayank99 mayank99 Sep 18, 2024

Choose a reason for hiding this comment

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

@GerardasB What do you think would be the best way to test this? It would be difficult to add a proper test for this in iTwinUI without manually recreating the custom event / reparenting logic.

I'm thinking we first manually verify that it works within the AppUI repo (by editing @itwin/itwinui-react in node_modules) and then after this is released, we can update the existing AppUI test scenario to ensure the styles look fine without any special handling on top of ProgressRadial.

Choose a reason for hiding this comment

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

Sounds good, I'll verify if this change works and let you know.

Copy link

@GerardasB GerardasB Sep 19, 2024

Choose a reason for hiding this comment

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

I can confirm, this works correctly in the reparenting use-case (I made sure to remove the custom useEffect workaround https://github.com/iTwin/appui/blob/b1c1d0c95a58ebbf2985bb2e61ad01d639e7f795/apps/test-app/src/frontend/appui/frontstages/TestPopoutFrontstage.tsx#L90)

However, this change breaks the regular use-case, where no reparenting is done and the component is simply mounted in a new window. (reparentPopoutWidgets=0 to configure in AppUI test-app: https://github.com/iTwin/appui/blob/b1c1d0c95a58ebbf2985bb2e61ad01d639e7f795/e2e-tests/tests/popout-widget.test.ts#L210)

This happens because useEffect that is scheduled in useLatestRef(shadowRoot) runs after the createStyleSheet called after ReactDOM4.flushSync(), which results in createStyleSheet returning early due to null latestShadowRoot.current.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for testing it. I believe I've fixed the regular popout case in my latest changes. I'm passing the already existing shadow into createStyleSheet (rather than relying on useLatestRef).

I also added a basic test for it, since this test does not require us to reimplement any reparenting logic.

Choose a reason for hiding this comment

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

Looks good now!


return shadowRoot;
}
Loading