radix-ui / primitives

Radix Primitives is an open-source UI component library for building high-quality, accessible design systems and web apps. Maintained by @workos.
https://radix-ui.com/primitives
MIT License
15.76k stars 817 forks source link

[Dialog] force mounting dialog content makes the dialog open by default #998

Closed tk-olari closed 2 years ago

tk-olari commented 2 years ago

Bug report

Force mounting DialogOverlay and DialogContent renders in the DOM the dialog and it is possible to control the visibility of the dialog with CSS but it also locks the scroll and traps the focus which makes the page unusable.

Current Behavior

Force mounting DialogOverlay and DialogContent in modal mode blocks the scrolling and traps focus.

Expected behavior

Force mounting DialogOverlay and DialogContent should work with scroll lock and focus trap the same way as forceMount=false. So basically the dialog should control focus trap and scroll lock regardless of forceMount option.

Reproducible example

https://codesandbox.io/s/2oryx?file=/App.js

In this example I made the height of the body 200vh for scrolling purpose. By default the forceMount option is true and you can observe that it is impossible to scroll or click the button. The dialog is hidden with CSS using [data-state='closed'] selector. If forceMount is set to false, everything is working as expected.

Suggested solution

I'm not sure if this is a bug or an intended behaviour. If it is intended, how the scroll lock and focus trap can be released programmatically?

Your environment

Software Name(s) Version
Radix Package(s) @radix-ui/react-dialog 0.1.0
React n/a 17.0.2
Browser any any
Assistive tech - -
Node n/a 14.16.0
npm/yarn any any
Operating System any any
jjenzz commented 2 years ago

I'm not sure if this is a bug or an intended behaviour. If it is intended, how the scroll lock and focus trap can be released programmatically?

We had a similar issue reported for this here https://github.com/radix-ui/primitives/issues/628. It might help shed some light on the thinking behind this prop. Although I'm curious, why are you wanting to hide the component with CSS instead?

@benoitgrelard I wonder if we should rename the prop to something like deferUnmount and explicitly document it as a prop to defer unmount for animation libs? Unless the use-case here is valid of course.

benoitgrelard commented 2 years ago

Yeah I don't think the use case is valid considering what the prop is for. I'm also unsure changing the name of the prop would change anything 🤔

jjenzz commented 2 years ago

I'm also unsure changing the name of the prop would change anything 🤔

The name forceMount implies it can be used to keep it mounted "for any reason". Hence, I assume, @tk-olari thought they could use it to keep things mounted and hide with CSS instead.

Our docs even imply this:

Used to force mounting when more control is needed. Useful when controlling animation with React animation libraries.

"When more control is needed" - for anything?

My thinking with the rename is that it would explicitly communicate that it is only meant for deferring the unmount to something else to handle, as in, it should still be unmounted by something and not long-lived.

tk-olari commented 2 years ago

Yeah so my use case was to keep some internal state as @jjenzz pointed in #628 but I'm using a third party (algolia) and it's kinda difficult with algolia connectors to lift the state and control it. The issue I encountered is that when dialog get unmounted the algolia filters connector cleans any applied filters (https://github.com/algolia/react-instantsearch/issues/892) and I was thinking that forceMount will keep the dialog alive as much as the web page is open and I will be able to control its visibility using css. But now I got that this prop is for animations only. Anyway I managed to solve(kinda) my initial issue with some hacky code, but it works.

So the issue I opened here was more like a question. Now that I got the answer, feel free to close it.

benoitgrelard commented 2 years ago

I'm also unsure changing the name of the prop would change anything 🤔

The name forceMount implies it can be used to keep it mounted "for any reason". Hence, I assume, @tk-olari thought they could use it to keep things mounted and hide with CSS instead.

Our docs even imply this:

Used to force mounting when more control is needed. Useful when controlling animation with React animation libraries.

"When more control is needed" - for anything?

My thinking with the rename is that it would explicitly communicate that it is only meant for deferring the unmount to something else to handle, as in, it should still be unmounted by something and not long-lived.

Yeah perhaps a name change could help, just not sure to what cause it's confusing, as it's not really deferring unmount, it's just always mounted and up to the user to now mount/unmount correctly.

So in my mind, forceMount communicates what happens better, it's forcing it mounted always. Perhaps just a docs description change?

tk-olari commented 2 years ago

Yeah, I think if it is possible to update the docs to reflect what indeed forceMount does and doesn't might be helpful for others to reduce confusion. Maybe in react ecosystem its a well know term about mounting/unmounting, but I was a long time on angular side and I think this is the actual issue here - terminology 😄

fgatti675 commented 1 year ago

I am sorry guys, I got confused with forceMount, I was expecting it to work like the original author suggests. I have read this thread, but I still fail to understand the purpose. I was expecting that with forceMount=true and open=false the components would sill be attached to the dom but not visible or interactive. This would be useful to be able to animate it, like the prop description suggests. Instead, it just remains opens and blocks all the interaction with the rest of the page. What am I missing, or how could I solve this? Thank you

jjenzz commented 1 year ago

@fgatti675 the forceMount prop is only really useful when delegating the unmount to a JavaScript animation library. if you are trying to animate using CSS then you can use css keyframes and the unmount will automatically wait for your animation to complete in all radix primitives.

i agree this forceMount prop is confusing though. it's one of my least favourite parts of the Radix APIs.

@benoitgrelard in regards to this:

So in my mind, forceMount communicates what happens better, it's forcing it mounted always.

the confusion stems from the fact that we have two props forceMount and open like @fgatti675 describes. if someone sets open={false} it seems the expectation is that it would behave closed (so no scroll lock) but would still exist in the DOM (mounted).

that's why i proposed the deferUnmount rename because it is not meant for forcing the DOM tree to stick around indefinitely, it's really just for deferring the unmount to animation libs (i don't know of any other useful use-case in its current form).

50bbx commented 7 months ago

We encountered this during development. Changing the prop as @jjenzz suggests, would only reduce confusion, but it would not address the scenario where you want the content of the dialog mounted (for any reason) but closed.

We would find this useful because we have asynchronous content in the modal and we would like to prefetch it before the user clicks on the open button of the modal instead of let the user wait with a loading state. The easiest way to do this would be pre-mounting the component that does the fetch call but it doesn't seem possible at the moment.

Is that correct?

paschalidi commented 4 months ago

+1 looking to understand how to have the Dialog closed but always mounted on the dom. Any workarounds for this?

Our use-case is same to @tk-olari, we use algolia. @tk-olari how did u solve this in the end?

Ojay commented 1 month ago

+1 for this, I have a carousel with some hefty images I need to preload within my Dialog. We can hack our way around the CSS but there should be a considered out-of-the-box approach for this feature (IMO).

OFranke commented 1 month ago

@paschalidi in the use case of Algolia I guess you have to have your custom refinements in the DOM because otherwise closing the Dialog would also kill the Algolia state from the url. One easy workaround you can do is:

<div>
  <Dialog>
    <CustomRefinements />  
  </Dialog>
  <div className="hidden">
    <CustomRefinements />
  </div>
</div>

Basically you render all your refinements two times and just hide one via CSS so Algolia will maintain the search state because the Refinements are not unmounted at any point.

oktav777 commented 1 month ago

For everyone dealing with Algolia, it has options to handle this case, see show and hide widgets

hope it helps

christian-reichart commented 3 weeks ago

Also +1 here, currently having an issue trying to display select options as an overlay instead of a popover. I don't understand the prop forceMount and why it's related to the open state. IMO this should be 2 completely different things.

I want the component mounted, but don't want it to display or scrolllock my view.