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] force mounting dialog content makes the dialog open by default #998

Closed
tk-olari opened this issue Nov 23, 2021 · 14 comments
Closed
Labels
Resolution: Invalid This issue isn't valid

Comments

@tk-olari
Copy link

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
Copy link
Contributor

jjenzz commented Nov 23, 2021

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 #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
Copy link
Contributor

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
Copy link
Contributor

jjenzz commented Nov 24, 2021

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
Copy link
Author

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 (algolia/react-instantsearch#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
Copy link
Contributor

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
Copy link
Author

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 😄

@benoitgrelard benoitgrelard added the Resolution: Invalid This issue isn't valid label Apr 5, 2022
@fgatti675
Copy link

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
Copy link
Contributor

jjenzz commented Aug 5, 2023

@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
Copy link

50bbx commented Mar 13, 2024

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
Copy link

+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
Copy link

Ojay commented Sep 17, 2024

+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
Copy link

OFranke commented Sep 18, 2024

@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
Copy link

oktav777 commented Sep 19, 2024

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

  • preserveSharedStateOnUnmount for newer versions
  • virtual components that are rendered as part of react component tree but not as part of DOM

hope it helps

@christian-reichart
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Invalid This issue isn't valid
Projects
None yet
Development

No branches or pull requests

10 participants