iTwin / iTwinUI

A design system for building beautiful and well-working web interfaces.
https://itwin.github.io/iTwinUI/
MIT License
105 stars 38 forks source link

Popover is rendered above the dialog #2156

Open GerardasB opened 1 month ago

GerardasB commented 1 month ago

Describe the bug (current behavior)

Popover is displayed above the dialog when rendered as a sibling to Dialog or Modal components.

Expected Behavior

Popover should be rendered below the Dialog and it's backdrop. To render the Popover above the Dialog it should be portalled into the Dialog (or rendered as a child of the dialog). Currently, each Popover portal component is wrapped by the ThemeProvider to handle nested popups, while Dialog is not and would require an explicit ThemeProvider or target prop to be passed into each popover, which seems inconsistent.

Link to minimal repro

https://stackblitz.com/edit/github-xa43bc?file=src%2FApp.tsx

Steps To Reproduce

Anything else?

No response

Ben-Pusey-Bentley commented 1 month ago

Seems that the z-index for the Popover is being set to such a high number that it gets drawn over absolutely everything currently. Setting it from 9999 to 999 has the Popover get drawn underneath the Dialog and it's backdrop correctly. Video:

https://github.com/user-attachments/assets/6ab36ed8-92b6-4aa7-9ca4-b93b98851e6f

@mayank99 is there a reason that the z-index is being set so high? Would it make sense for us to lower it to 999?

mayank99 commented 1 month ago

This issue is more complicated than just adjusting the z-index. We discussed this a bit in an internal Teams thread, but I'll summarize my thoughts:

Generally, I agree that modal dialogs should appear on top of everything else. Beyond that, I'm thinking we might need to maintain a stacking order: the one that was opened last should appear on top. This also matches how the browser-native top-layer mechanism works.

As a bonus, maintaining a stacking order would also allow us to reorder the stack as needed, which should help with #2104.


Further thoughts that have been in the back of my mind:

For the stack to work, we would likely need to make our portal containers work more reliably. Every ThemeProvider on the page should use the same portal container. Currently, this kinda works, assuming that React context is behaving as intended. But we have seen repeatedly that our users are unable to make context work in their applications, because of version/module conflicts. So we need a DOM-native (React-agnostic) way of inheriting portal containers across ThemeProviders from different versions.