radix-ui / primitives

Radix Primitives is an open-source UI component library for building high-quality, accessible design systems and web apps. Maintained by @workos.
https://radix-ui.com/primitives
MIT License
15.43k stars 774 forks source link

Z-index issues when combining Dialog.Portal with Popover, Dropdown, etc. #1317

Closed jgoz closed 2 years ago

jgoz commented 2 years ago

Bug report

Current Behavior

After switching over to the new <Dialog.Portal>, I'm having issues maintaining a proper z-index order for dialogs that are initiated from Popovers or Dropdown menus.

A use case for this might be a "Delete" option in a DropdownMenu. The user opens a dropdown, clicks "Delete", and then a Dialog prompts them to confirm. We choose to keep the dropdown list open while the dialog is on screen.

With the current behavior, the dropdown list renders above the dialog's overlay because the dialog's portal seems to be using a new system that doesn't set a value for "z-index", but Popovers and DropdownMenus are using radix-portal, which sets 2147483647.

image

Expected behavior

I would expect portals to layer in the order in which they are made visible. So if a Popover opens a Dialog, the Dialog should layer on top. If a Dialog opens a Popover, the Popover should layer on top.

Reproducible example

https://codesandbox.io/s/dreamy-curie-1rc2tf?file=/src/App.tsx

Suggested solution

Add documentation about how we can achieve this with the new Dialog API, if it's possible.

Your environment

Software Name(s) Version
Radix Package(s) @radix-ui/react-dialog, @radix-ui/react-popover 0.1.7, 0.1.6
React n/a
Browser Chrome 100
Assistive tech
Node n/a
npm/yarn
Operating System
benoitgrelard commented 2 years ago

Hi @jgoz,

It seems the reproducible Sandbox isn't saved properly, there's nothing in there apart from a title.

That being said, I think I can shed some light already.

Historically, all the primitives you mentioned used Portal internally and it wasn't exposed. We hardcoded the z-index on it to the maximum, to make it all work out of the box as you suggested here:

I would expect portals to layer in the order in which they are made visible.

Our users told us that they sometimes needed more control over the z-index when integrating with other libs, or inside apps where you do not necessarily have control over every aspect.

With that, we trialed the idea of exposing the portal part in Dialog which no longer sets any z-index so you have full control as a user. The reception has been good and so we will roll it out to all the other relevant components soon (#1302, #1303, #1304, #1305, #1306).

In the meantime, I can see how it might be a bit counter-intuitive when using a mixture of those.

I think for now, your best solution will be to manually set the old max z-index on the dialog ones, until we update all the components, at which point you will have full control to do whatever you want.

✌️

jgoz commented 2 years ago

Thanks for the detailed response @benoitgrelard and apologies about the sandbox.

I did actually try setting the maximum z-index on the Dialog portal but that doesn't apply it to the correct element, from what I can tell. If this is supposed to work, I can try to get a reproducible example of what it does instead.

I also tried rendering it in a portal imported from @radix-ui/react-portal but that didn't work reliably either.

My short term workaround is simply to downgrade and wait for the broader rollout of the other portals.

benoitgrelard commented 2 years ago

I believe it should work so if you could update the sandbox to reproduce it, we can then take a look for you.

jgoz commented 2 years ago

OK, I reproduced it in a sandbox: https://codesandbox.io/s/1rc2tf?file=/src/App.tsx

I needed to add another element inside the portal that sets max z-index and expand it to fill the screen. It layers correctly when the elements are opened by user interaction, but when both are set to defaultOpen, the popover layers on top of the dialog.

I'm not sure if this would actually affect us though, so it might be a suitable workaround until the new portals are rolled out more broadly.

benoitgrelard commented 2 years ago

I see the confusion.

You don't need to add an extra element. The way it works is that whatever is wrapped in Dialog.Portal (here Dialog.Overlay and Dialog.Content) gets portalled without the need to create an extra element for the portal. This means you can apply the styles directly to those parts like this for example:

https://codesandbox.io/s/inspiring-cloud-9e58kr?file=/src/App.tsx

but when both are set to defaultOpen, the popover layers on top of the dialog.

This however was always present before or after that change, and is due to how React renders children first, so here the dialog being a child of the popover, it will be rendered first, hence its portal will be first in the DOM.

This is unlikely to be an issue in real life as you shouldn't auto-open popover/dialogs typically. That being said, that is one of the reasons that pushed us to open up that portal part to give more control for z-index, as once again this is something you will be able to get around with once all primitives can be controlled via their own z-index.

Hope that makes sense.

jgoz commented 2 years ago

This however was always present before or after that change, and is due to how React renders children first, so here the dialog being a child of the popover, it will be rendered first, hence its portal will be first in the DOM.

This is unlikely to be an issue in real life as you shouldn't auto-open popover/dialogs typically. That being said, that is one of the reasons that pushed us to open up that portal part to give more control for z-index, as once again this is something you will be able to get around with once all primitives can be controlled via their own z-index.

Hope that makes sense.

Yep, that all makes sense. My example was definitely synthetic/not-real-world.

Thank you again for the response - I think we've got enough workarounds in here to consider closing this issue. Or you could close it when the other Portal components are merged.

benoitgrelard commented 2 years ago

Thanks, I will close this one then because we are tracking the others separately.

✌️

mattp0123 commented 2 years ago

@benoitgrelard Hi. So what am I supposed to do to make the dialog on top of the popover? Can they share the div[data-radix-portal]? https://codesandbox.io/s/dialog-inside-popover-m09fmx?file=/src/App.tsx

benoitgrelard commented 2 years ago

Hey @mattp0123,

With that, we trialed the idea of exposing the portal part in Dialog which no longer sets any z-index so you have full control as a user. The reception has been good and so we will roll it out to all the other relevant components soon (https://github.com/radix-ui/primitives/issues/1302, https://github.com/radix-ui/primitives/issues/1303, https://github.com/radix-ui/primitives/issues/1304, https://github.com/radix-ui/primitives/issues/1305, https://github.com/radix-ui/primitives/issues/1306).

All the improvements suggested above have been made, they are available in the latest release candidates, not public yet. But if you update your example to use the latest RCs for Popover and Dialog and add the new Popover.Portal part (just like the dialog one) then it should just work out of the box (and if you need more control, you can now set your own z-index as we don't set any).

Side node: I noticed you used asChild and then attached styles to divs, etc. You can pass your styles directly to the part, and not use asChild too. All the parts render an element by default. asChild is there mostly for composition or ability to change the tag types in some fringe cases.