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

Popovers and other UI should not appear on top of modal dialog overlays #55894

Open
afercia opened this issue Nov 6, 2023 · 10 comments
Open
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Feedback Needs general design feedback. [Package] Components /packages/components

Comments

@afercia
Copy link
Contributor

afercia commented Nov 6, 2023

Description

Splitting this out from #55785

Difficult to say what package this issue is about. It's more about how some components are used in combination with particular user flows.

In all cases, when a modal dialog opens nothing else should visually appear on top of the modal dialog overlay.

This doesn't appear to be the case when a popover is open and then invoking a modal dialog via keyboard shortcuts.

Step-by-step reproduction instructions

  • Edit a post.
  • In the Post editor top bar, click the 'Tools' button.
  • The popover with the Select / Edit mods opens.
  • While the popover is still open, press Command+K.
  • The Command Center modal dialog opens.
  • Observe the Tools popover stays open and appears on top of the modal dialog overlay.
  • Screenshot:

Screenshot 2023-11-06 at 14 27 16

  • Repeat the steps above but this time open the Keyboard Shortcuts modal dialog by pressing Control+Option+H.
  • Observe the Tools popover stays open and appears on top of the modal dialog overlay.

Screenshot:

Screenshot 2023-11-06 at 14 27 00

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@afercia afercia added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Feedback Needs general design feedback. labels Nov 6, 2023
@afercia
Copy link
Contributor Author

afercia commented Nov 6, 2023

It is worth mentioning that no other UI should appear on top of a modal dialog overlay so this is not limited to popovers but applies to any other UI that potentially may have the same incorrect behavior.

@ciampo
Copy link
Contributor

ciampo commented Nov 6, 2023

This is a fair point, and potentially a tricky one to solve:

  • the main issue is that many popover-based components do not have the concept of "modality" (only Modal does), and as a result, these components can be open at the same time. We are slowly working towards it by migrating our components to internally use ariakit, which natively supports "modality" for these components
  • for command palette may pose a slightly different challenge, since it implemented using a different third-party library (which I believe uses radix-ui in the background). So we'd need to somehow have a way for the command palette to notify the rest of the app that any other modal-like piece of UI should be hidden when the palette is opened.

@ciampo ciampo added the [Package] Components /packages/components label Nov 6, 2023
@afercia
Copy link
Contributor Author

afercia commented Nov 6, 2023

Thanks @ciampo for your feedback. I think there are two different aspects to consider here:

  • Whether the other popovers and the like should close when a modal dialog opens. This is tricky as you said.
  • Visibility: as a first step, simply revising the z-indexes may help to make sure the modal dialog overlay stays on toop of anything else.

The current z-index value for popovers is higher than the one for the modal dialog overlay:

.components-popover {
    will-change: transform;
    z-index: 1000000;
}
.components-modal__screen-overlay {
    animation: edit-post__fade-in-animation .2s ease-out 0s;
    ...
    left: 0;
    position: fixed;
    right: 0;
    top: 0;
    z-index: 100000;
}

@afercia afercia added the CSS Styling Related to editor and front end styles, CSS-specific issues. label Nov 6, 2023
@afercia
Copy link
Contributor Author

afercia commented Nov 6, 2023

Trying to inspect all the z-indexes with very high value I just noticed that also the new dropdown powered by ariakit uses a too high z-index value. The value is hardcoded in the component. I do realize why but that should be improved as noted in the comments in the code.

Screenshot of a dropdown menu appearing on top of a modal dialog overlay:

Screenshot 2023-11-06 at 15 53 54

@ciampo
Copy link
Contributor

ciampo commented Nov 6, 2023

One aspect to consider is that popover-based components (eg. dropdown menus, tooltips) can also be rendered inside a modal dialog, and therefore it's not straightforward to come up with z-index rules that work across all scenarios.

@peterwilsoncc
Copy link
Contributor

One aspect to consider is that popover-based components (eg. dropdown menus, tooltips) can also be rendered inside a modal dialog, and therefore it's not straightforward to come up with z-index rules that work across all scenarios.

Could these be modified to render within the modals. Doing so would resolve the issue as the popover would be created within the modal's stacking context.

@diegohaz
Copy link
Member

One aspect to consider is that popover-based components (eg. dropdown menus, tooltips) can also be rendered inside a modal dialog, and therefore it's not straightforward to come up with z-index rules that work across all scenarios.

Could these be modified to render within the modals. Doing so would resolve the issue as the popover would be created within the modal's stacking context.

If they render within the modal in the DOM, overflow issues might happen (with overflow: hidden, overflow: auto, overflow: scroll, and so on).

A different approach is to set up a React Context-based z-index stack. This way, nested dialogs can increase the z-index they obtain from the context based on the React tree, even if they're appended to another DOM node using React Portal.

@afercia
Copy link
Contributor Author

afercia commented Nov 20, 2023

Worth reminding there was also a problem with tooltips within the modal dialog, in the Site Editor and on mobile. See #47614 (comment) that was fixed only coincidentally with the refactoring of the tooltips with ariakit in #48440
It would be great to take into account all the potential cases and add E2E tests for each of them.

@ciampo
Copy link
Contributor

ciampo commented Nov 22, 2023

Overall, I'd like to reach a point where our popover-based UIs (modal or not) render correctly on top of each other, following the react tree nesting, and thus allowing us to move away from hardcoded z-index values (unless there's a real need).

For the rare cases where, while some modal UI is rendered on screen, a new piece of non-nested modal UI needs to be rendered (like in the issue's description), we have two choices:

  • we close the existing modal UI, before opening the new one
  • we prevent the new one from opening

Whatever solution we adopt, we'd likely need something like a global "modal context" to manage the logic.

@afercia
Copy link
Contributor Author

afercia commented Dec 28, 2023

One more evident case after #56817 is when creating a new template in the post editor:

  • Settings panel > click the template bane, click Create new template
  • Observe the popover stays open on top of the modal dialog overlay

Cc @youknowriad

Screenshot:

Screenshot 2023-12-28 at 14 32 40

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Feedback Needs general design feedback. [Package] Components /packages/components
Projects
None yet
Development

No branches or pull requests

4 participants