render a new Root as the portal container
#2173
Conversation
mayank99
force-pushed
the
mayank/portal-root
branch
from
56c6553
to
84c8bf5
Compare
mayank99
requested review from
r100-stack and
Ben-Pusey-Bentley
and removed request for
a team
| <div style={{ display: 'contents' }} ref={setPortalContainer} id={id}> | ||
| <Root |
There was a problem hiding this comment.
Could you share some examples of where Root will be needed to be outside of ThemeProvider? i.e. the goal of this PR (if I'm not mistaken).
I'm not sure if this will lead to some bugs or confusion? i.e. could the portaled content somehow read the theme from the wrong <div>? Also, could having two iui-roots cause some bugs in the future? Could these lead to multiple sources of truths kind of problems that could be difficult to debug?
There was a problem hiding this comment.
The goal of this PR is to help with my future PR, where I'm planning to render the portal container as a child of <body>, rather than as a child of <ThemeProvider>. This will fix many of the popover/stacking-related issues.
Could you elaborate on what kinds of bugs/confusion you're worried about? I don't know why it would be problematic to have two roots, but making this change is a requirement, because the portaled elements will no longer be DOM descendants of the main Root. Maybe I can add some tests to ease your concerns.
For some more context: the portal container in iTwinUI v1 used to be a child of <body>, but we moved away from that because we wanted to enable incremental adoption of sub-trees. The reason we were able to do this in v1 is that we were imperatively setting the theme attributes on the <html>/<body> elements. Whereas now we don't do such global side-effects, which means we must render a new root for portaled elements to be themed.
Old old DOM (simplified)
<html data-iui-theme="dark">
<body class="iui-root">
<button class="iui-button" aria-controls="1">…</button>
<div>
<div id="1" class="iui-menu" role="menu">…</div>
</div>
</body>
</html>New DOM (simplified)
<html>
<body>
<div class="iui-root" data-iui-theme="dark">
<button class="iui-button" aria-controls="1">…</button>
</div>
<div class="iui-root" data-iui-theme="dark">
<div id="1" class="iui-menu" role="menu">…</div>
</div>
</body>
</html>There was a problem hiding this comment.
Could you elaborate on what kinds of bugs/confusion you're worried about? I don't know why it would be problematic to have two roots, but making this change is a requirement, because the portaled elements will no longer be DOM descendants of the main
Root. Maybe I can add some tests to ease your concerns.
I felt that having two iui-root may make it confusing for which is the real content and which is the portal container. This may be more apparent with nested ThemeProviders which will bring many iui-roots making it harder to know which is which.
But looking at the new DOM you shared, I guess your goal is to have any content (main or floating) under a separate iui-root. So, rethinking about my concerns and understanding the goal of this PR, this change LGTM.
> my future PR, where I'm planning to render the portal container as a child of <body>, rather than as a child of <ThemeProvider>
To confirm, if floating content will be portaled to the body, will it be possible from now to have some floating content in one theme and the other in another theme, for example? I believe I misunderstood the new DOM. Since each DOM will be in its own iui-root, I think it'll be possible.
There was a problem hiding this comment.
Maybe I can add some tests to ease your concerns.
Optional: Yes, if possible, it would be safer to have these tests.
There was a problem hiding this comment.
Since each
DOMtheme will be in its own iui-root, I think it'll be possible.
That's right, we can create as many portal roots as we want. I expect we will need one per theme per version.
Yes, if possible, it would be safer to have these tests.
I added an e2e test in 51610f4 that verifies that theme is kept in sync between the main root and the portaled root. To make the portal container easy to find in e2e tests, I also added a data-iui-portal attribute (05b103c); as a bonus, this will make it easier to debug/disambiguate.
| <div style={{ display: 'contents' }} ref={setPortalContainer} id={id}> | ||
| <Root |
There was a problem hiding this comment.
Maybe I can add some tests to ease your concerns.
Optional: Yes, if possible, it would be safer to have these tests.
Changes
This is a very small change: Instead of rendering a plain
<div>as the portal container, we now render a<Root data-iui-portal>. This will allow us to render the portal container outside of the main<ThemeProvider>in the future.To facilitate this change I made the
Rootcomponent more portable by movingfindOwnerDocumentFromRefinto a separateMainRootcomponent. Unrelated: I also removed the types forshouldApplyBackgroundsince it wasn't being used anywhere.Testing
Manually verified that the classes/attributes from
Rootare being added.Existing tests pass! Added one new e2e test to ensure theme is synchronized.
Docs
N/A. This is too small of a change for users to even care.