Increase ConfirmDialog overlay's z-index to render above popovers

[chad1008]

Description

This PR increases the z-index value for the new ConfirmDialog component so it will render above any Popovers.

By default, Popovers render above everything, with some limited exceptions. In the case of ConfirmDialog, this means that if the dialog is the child component of such a Popover, the parent component renders above the confirmation dialog.

How has this been tested?

To test, we first need an active example of ConfirmDialog, which is still being implemented.

1. Check out Migrate post privacy confirmation from confirm() to ConfirmDialog #37602
2. Next, cherry-pick the commit(s) on this PR to include the new CSS in your working copy
3. Create a new post in Gutenberg, and publish it
4. In the editor, click on the posts "Public" visibility
5. In the PostVisibility component that renders, select Private
6. Confirm that when the confirmation dialog opens, it renders above the PostVisibility popover (the popover should appear below the scrim along with the rest of the elements on the page)

Tested in latest Chrome with GB 6.0-alpha-52573 via wp-env.

Checklist:

• [ x ] My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• I've tested my changes with keyboard and screen readers.
• [ x ] My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).
• I've updated related schemas if appropriate.

[ciampo]

Should we apply the z-index change to the ConfirmDialog or to Modal directly? After all, Modal's description says:

A modal is a type of floating window that appears in front of content to provide critical information or ask for a decision. Modals disable all other functionality when they appear. A modal remains on screen until the user confirms it, dismisses it, or takes the required action.

In case we do make the change in the Modal component, we would need to make sure that it doesn't introduce any regressions in other parts of Gutenberg.

Also, could there ever be a case where the popover needs to be rendered on top of a confirm dialog? I can't personally think of any, as in my mind, a modal dialog should render on top of every other UI. But just wanted to double check before going ahead. (cc @diegohaz @mirka )

[ciampo]

Well spotted about leaving a comment here.

One more example of how having some components written in Sass and some in CSS-in-JS can damage readability and modularity of this library (cc @mirka as we have already discussed, it'd feel real nice to refactor this to CSS-in-JS)

[chad1008]

kudos to @fullofcaffeine for the guidance and suggestions regarding the commenting 😄

[chad1008]

In case we do make the change in the Modal component, we would need to make sure that it doesn't introduce any regressions in other parts of Gutenberg.
Also, could there ever be a case where the popover needs to be rendered on top of a confirm dialog? I can't personally think of any, as in my mind, a modal dialog should render on top of every other UI.

I had the same thoughts, until I saw the base styles, where it looks like popovers are rendered above modals intentionally - so I'm guessing there's some logic/nuance I'm not thinking of 🤔

[ciampo]

In case we do make the change in the Modal component, we would need to make sure that it doesn't introduce any regressions in other parts of Gutenberg.
Also, could there ever be a case where the popover needs to be rendered on top of a confirm dialog? I can't personally think of any, as in my mind, a modal dialog should render on top of every other UI.

I had the same thoughts, until I saw the base styles, where it looks like popovers are rendered above modals intentionally - so I'm guessing there's some logic/nuance I'm not thinking of 🤔

Hey @diegohaz @youknowriad @jasmussen (sorry for the ping!), do you have any insights on if and why popovers may be intentionally rendered on top of modals? Thank you!

[youknowriad]

Hey @diegohaz @youknowriad @jasmussen (sorry for the ping!), do you have any insights on if and why popovers may be intentionally rendered on top of modals? Thank you!

Sure, I guess dropdowns and tooltips for UIs rendered inside a modal.

[mirka]

<Modal>
  <Tooltip text="world">
    <span>hello</span>
  </Tooltip>
</Modal>

^ Given that this kind of thing is already not working, I would think it's not really something that can be fully addressed by z-index alone. Should we reconsider where the portals are inserted, and/or whether the stacking contexts are properly isolated?

[ciampo]

<Modal>
  <Tooltip text="world">
    <span>hello</span>
  </Tooltip>
</Modal>

^ Given that this kind of thing is already not working, I would think it's not really something that can be fully addressed by z-index alone. Should we reconsider where the portals are inserted, and/or whether the stacking contexts are properly isolated?

Yes, the root of the cause definitely seems to be related to these components not rendering in the correct stacking context — although I'm not super familiar with how exactly they render, and I assume that we would need to investigate what's the best approach here that would allow us to make improvements without introducing breaking changes.

Should we also abandon the changes proposed in this PR, since they would introduce a regression where dropdowns/tooltips are supposed to render on top/within a modal?

[chad1008]

I feel like I may have missed a step here, so I wanted to clarify my understanding... when I test the above example (a Tooltip nested inside a Modal) it renders as I'd expect it to:

Is that something that's actually broken in the current implementation, or is breaking as a result of this PR?

I also just ran a quick test, and if I place a Tooltip or even a Popover inside a ConfirmDialog they both appear to render as expected with this PR applied.

I'm not very familiar with stacking contexts (added to my reading list!) so I wanted to ask so I could fully understand the potential regression here in the meantime 😄

[ciampo]

Thank you @chad1008 for looking more into this!

I'm going to write down a few considerations:

Tooltip inside a Modal

I feel like I may have missed a step here, so I wanted to clarify my understanding... when I test the above example (a Tooltip nested inside a Modal) it renders as I'd expect it to:

I have also looked into this — in my tests, the Tooltip component doesn't render correctly in the Modal, but not for z-index reasons: the problem seems, instead, related to the wrong positioning of the tooltip in combination with the overflow: hidden rule set on the Modal's container:

(note: I disabled the mouseout event on the tooltip, to make it easier to debug)

modal-tooltip.mp4

Testing this PR's fix

I also went ahead and tested the fix by adding a Popover in the same story with a ConfirmDialog, and the Popover still renders on top of the ConfirmDialog for me:

(I'm going to add an inline comment on how to increase the specificity of the CSS rule that overrides the z-indez)

I also went ahead and added a Popover inside a ConfirmDialog, with similar results to the tooltip example above

Basically, there doesn't seem to be a z-index issue when rendering Popover inside ConfirmDialog (although there may be other unrelated issues preventing the Popover to show correctly)

Applying the higher z-index only to ConfirmDialog vs to Modal

There's a comment above the current z-index value for the Modal component:

"Show modal under the wp-admin menus and the popover"

This means that it is expected of the Modal component to render under Popovers & co. Personally I don't really understand the reason behind this (in my mind, within the same "stacking context", a modal should always render on top).

But, in my opinion, that's a matter that needs to be discussed separately and carefully, since it has the potential of breaking some UIs across Gutenberg.

Probably the better thing to do, at the moment, is to apply a higher z-index only to ConfirmDialog (like this PR suggests), in order to unblock the set of PRs using that component.

What do folks think?

[chad1008]

Probably the better thing to do, at the moment, is to apply a higher z-index only to ConfirmDialog (like this PR suggests), in order to unblock the set of PRs using that component.
What do folks think?

Sounds logical to me. As you say, the overflow issue feels like a separate, larger issue that should be addressed separately. I'm in favor of adjusting the z-index to address the short term ConfirmDialog needs for now.

[fullofcaffeine]

Probably the better thing to do, at the moment, is to apply a higher z-index only to ConfirmDialog (like this PR suggests), in order to unblock the set of PRs using that component.

What do folks think?

@ciampo Agreed! Given you already pre-approved this this solution is isolated to the ConfirmDialog component and that it tests well with #37602, I'm approving this now.

[ciampo]

Before merging, @chad1008 could you add an entry to the CHANGELOG ? Thank you!

[chad1008]

Before merging, @chad1008 could you add an entry to the CHANGELOG ? Thank you!

Done! thank you all for the review and input!