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

[POC] Anchored modality changes #1231

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

atomiks
Copy link
Contributor

@atomiks atomiks commented Dec 26, 2024

This changes the modality of anchored popups that primarily open on click, and adds the modal prop to Popover.

  • Scroll locking is only enabled if the Backdrop part is rendered (where obscuring content justifies the lock)
  • The internal backdrop becomes inert (no longer blocks pointer-events) if the anchor is hidden from view (possible alternative: if the popup is hidden, but Floating UI doesn't provide a hook for this out of the box). This allows the user to instantly click any item beneath once they've scrolled away from the popover and no longer 'care' about it being open. Outside clicks are presumed to be careful and intentional at this point

@atomiks atomiks 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: popover The React component. labels Dec 26, 2024
@mui-bot
Copy link

mui-bot commented Dec 26, 2024

Netlify deploy preview

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

Generated by 🚫 dangerJS against 91f4a63

@colmtuite
Copy link
Contributor

I've lived with this the past few days (checking it intermittently). I'm not seeing much value/sense in this yet.

When a Menu is open, it's usually important to me that clicks outside it are blocked. When clicks outside are blocked, it's important that scroll is locked. These things have nothing to do with a backdrop being rendered.

I can potentially see an argument for closing the Menu when you scroll it out of view, or when you scroll x pixels away from where you were when it opened, or something like that. But just changing the behaviour of the Menu when its state has not changed, seems bad/odd.

@atomiks
Copy link
Contributor Author

atomiks commented Jan 2, 2025

@colmtuite to make #1234 work well for infotips, Popovers also need to add a modal prop which is true by default. When this is added, it will also by default lock the scroll for Popover. This doesn't match Radix (which doesn't block outside clicks) and feels overly restrictive.

When clicks outside are blocked, it's important that scroll is locked. These things have nothing to do with a backdrop being rendered.

The main reason to block outside pointer events is to prevent sloppy outside presses from causing any interaction. The only reason to lock the scroll in conjunction then is because when scrolling away from the popup, you may be confused why you can't instantly click anything beneath it on first click, since the popup is no longer visible/relevant. This PR makes it so the overlay becomes inert when it isn't visible, so this concern is mostly gone (depending on the anchor's visibility).

The only justified reason to lock outside scroll is because you've obscured the content beneath the popup and can't see it properly. It makes sense to prevent scrolling in that case since you'll become disoriented after dismissing the popup. But if the backdrop is fully transparent, there's no justification to lock scroll when you can see everything beneath the popup - the fact that outside presses are prevented on first click isn't related to the ability to scroll around

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: menu This is the name of the generic UI component, not the React module! component: popover The React component. component: select This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants