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

[Dialog, Popover, Menu, Select, PreviewCard] Unify backdrop implementation #841

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

michaldudak
Copy link
Member

@michaldudak michaldudak commented Nov 20, 2024

Unified and simplified backdrop implementation:

  • removed inline styles
  • set role: 'presentation' in all backdrops
  • removed use*Backdrop hooks
  • added an internal backdrop (FloatingOverlay) for modal dialogs and alert dialogs to block pointer interaction
  • added the container prop to DialogBackdrop and AlertDialogBackdrop
  • added the MenuBackdrop

part of #623

@michaldudak michaldudak added component: select This is the name of the generic UI component, not the React module! component: menu This is the name of the generic UI component, not the React module! component: alert dialog This is the name of the generic UI component, not the React module! component: dialog This is the name of the generic UI component, not the React module! component: preview card The React component. labels Nov 20, 2024
@mui-bot
Copy link

mui-bot commented Nov 20, 2024

Netlify deploy preview

https://deploy-preview-841--base-ui.netlify.app/

Generated by 🚫 dangerJS against 7f6be6a

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 21, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 21, 2024
Copy link
Contributor

@atomiks atomiks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Menu didn't have a Backdrop in the first place, but should?

@michaldudak michaldudak changed the title [Dialog, Menu, Select, PreviewCard] Unify backdrop implementation [Dialog, Popover, Select, PreviewCard] Unify backdrop implementation Nov 22, 2024
@michaldudak michaldudak changed the title [Dialog, Popover, Select, PreviewCard] Unify backdrop implementation [Dialog, Popover, Menu, Select, PreviewCard] Unify backdrop implementation Nov 22, 2024
@michaldudak michaldudak added the component: popover The React component. label Nov 22, 2024
@michaldudak
Copy link
Member Author

Added.

@@ -114,6 +114,7 @@ const DialogPopup = React.forwardRef(function DialogPopup(

return (
<FloatingPortal root={container}>
{modal && mounted && <FloatingOverlay />}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about z-index?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, right, this is going to be tricky. We could figure out the z-index of the popup and set this one lower, but this will be unreliable in some cases. We might have to expose the className of this part, or the whole part itself, to be consistent with the rest of the API.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this another problem solved by a separate Portal part? 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, unless I'm missing something. Having an explicit Portal wouldn't help determining the right z-index of the internal backdrop.
Perhaps a better solution is not rendering the internal backdrop, but adding the inert attribute to all elements except the popup?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a better solution is not rendering the internal backdrop, but adding the inert attribute to all elements except the popup?

This could work. guards={false} on FloatingFocusManager does this, but it allows escaping to the browser address bar of course, which we didn't want to allow. Guess this is relevant: floating-ui/floating-ui#3098

The other option is to get the computed z-index style of the Popup and apply the z-index to be just one less on FloatingOverlay?

Copy link
Member Author

@michaldudak michaldudak Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could work. guards={false} on FloatingFocusManager does this, but it allows escaping to the browser address bar of course, which we didn't want to allow.

I'll take a look at Floating UI implementation and see what I can do, then (floating-ui/floating-ui#3131)

The other option is to get the computed z-index style of the Popup and apply the z-index to be just one less on FloatingOverlay?

This was my first idea, but I realized someone might style a child of the popup for some reason. Or the z-index could change depending on how many dialogs are open, so this might not be the most reliable solution.

Copy link
Contributor

@atomiks atomiks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the bug that causes the Backdrop to be inserted into the DOM after the Popup fixed with this? Not sure why it would occur exactly but I'm assuming the way FloatingPortal creates a root container could be an issue. There is one minor part that has asynchronicity related to cleanup, but not sure if that would be the cause.

@michaldudak
Copy link
Member Author

michaldudak commented Nov 25, 2024

I don't know TBH. I could reproduce it neither on master nor here.
@vladmoroz, do you have a reliable repro?

@vladmoroz
Copy link
Contributor

I don't know TBH. I could reproduce it neither on master nor here.
@vladmoroz, do you have a reliable repro?

If its about Backdrop overlaying the Popup, I think I had a transition on the Popup, and no transition on the Backdrop

@atomiks
Copy link
Contributor

atomiks commented Nov 25, 2024

If its about Backdrop overlaying the Popup, I think I had a transition on the Popup, and no transition on the Backdrop

I was able to reproduce it consistently (once every ~5-10 times) on master previously when clicking very quickly on the trigger and backdrop - the backdrop did have a normal transition along with the popup. But on latest master, and this PR on the new dialog page, I haven't been able to reproduce it at all. Might be fixed?

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 26, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 27, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: alert dialog This is the name of the generic UI component, not the React module! component: dialog This is the name of the generic UI component, not the React module! component: menu This is the name of the generic UI component, not the React module! component: popover The React component. component: preview card The React component. component: select This is the name of the generic UI component, not the React module! PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants