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 backdrop is persisted when initialized as open but then immediately closed #12831

Closed
2 tasks done
kamranayub opened this issue Sep 10, 2018 · 14 comments · Fixed by #16694
Closed
2 tasks done

Dialog backdrop is persisted when initialized as open but then immediately closed #12831

kamranayub opened this issue Sep 10, 2018 · 14 comments · Fixed by #16694
Labels
bug 🐛 Something doesn't work component: dialog This is the name of the generic UI component, not the React module!

Comments

@kamranayub
Copy link
Contributor

This should not be a duplicate of the transition issue as I'm not using a Transition, this is a default Dialog component (reproduced below using the Component API Demo page).

Essentially I think flipping the open prop too quickly results in the backdrop being persisted. I know ideally I should get it to initialize with the right value, but the prop is driven off of Redux and initially I won't have the updated state until some other components that handle stuff are run (this is auth related, so validating tokens and stuff all happens outside this component).

  • This is a v1.x issue.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

When dialog is mounted initially with open true and then immediately reset back to false, backdrop should not be shown or block UI.

Current Behavior

When Dialog is initially mounted as open={true} but then immediately set to false (in my case, due to getDerivedStateFromProps that flips it immediately due to business logic), the UI backdrop is still being persisted and blocks the UI.

Steps to Reproduce

I changed the demo file to start with open set to true and then on mount, set it to false to emulate the situation I'm in.

Link: https://codesandbox.io/s/14w19w1px4

  1. Start app
  2. Try to click somewhere like the button

Context

Your Environment

Tech Version
Material-UI v3.0.2
React 16.3.2
Browser Chrome 68
TypeScript Yes (no in demo)
etc.
@mbrn
Copy link
Contributor

mbrn commented Sep 10, 2018

After you set open to false it does not set exited property of modal to true. So hidden class does not applied and dialog covers all area. Am I right? @kamranayub

I think that handleExit could not be called by a reason that i don't know after getDerivedStateFromProps.
I could be related with getHasTransition method.

Now i have a bigger problem: My daughter wants play with me:). So maybe later i will debug it.

@oliviertassinari: Please verify us , are we in right way?

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 10, 2018

As far as I could look into the issue, it's not related to the backdrop. You can remove it and still experience the problem. The onExited hook of react-transition-group isn't called.

  • Setting disablePortal on the Portal solves the problem
  • Using a div over a Portal solves the problem
  • Using NoSsr over the Portal maintains the issue.

So, we have it, it's a rendering order issue. We need the transition to render once with open true to get the onExited called when open is set to false.

I will leave you with that 😴

@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Sep 10, 2018
@nathanmarks
Copy link
Member

@mbrn are you working on this?

@mbrn
Copy link
Contributor

mbrn commented Sep 11, 2018

@nathanmarks i will.

@kamranayub
Copy link
Contributor Author

kamranayub commented Sep 11, 2018 via email

@mbrn
Copy link
Contributor

mbrn commented Sep 11, 2018

@oliviertassinari You are right. When i change disablePortal props of Modal to true, the problem is being solved. Should i send disablePortal:true to Modal from Dialog or make Modal's default disablePortal value true?

What do you offer? @nathanmarks @oliviertassinari

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 11, 2018

Should i send disablePortal:true to Modal from Dialog or make Modal's default disablePortal value true?

Please don't. It was just to identify what's going on. If few people are affected by this issue, @kamranayub and others can stick to the disablePortal workaround.
But if we can find a simple fix, not something that requires 50 LOCs, then yes, why not fixing it :). We could explore the onEnter hook or thedialogRef existence.

@mbrn
Copy link
Contributor

mbrn commented Sep 11, 2018

ok @oliviertassinari. @kamranayub you can set disablePortal={true} to solve your problem.

@kamranayub
Copy link
Contributor Author

kamranayub commented Sep 11, 2018 via email

@kamranayub
Copy link
Contributor Author

I actually managed to remove the need for a dialog in my use case so I'm no longer affected by this. I can close and if someone else finds a need for a fix, can re-open perhaps.

@nathanmarks
Copy link
Member

Re-opening this since it's a legitimate bug

@kamranayub
Copy link
Contributor Author

Sure, no problem. disablePortal={true} does indeed work.

@oliviertassinari oliviertassinari added the component: dialog This is the name of the generic UI component, not the React module! label Mar 13, 2019
ValentinH added a commit to ValentinH/material-ui that referenced this issue Jul 23, 2019
This should address mui#12831.

After following the comments in the above issue, I indeed noticed that `onExited` was not called. However, I've also noticed that `onEntered` was not called either. This is because the transition is not even rendered.

To solve this, I have added a flag that tells if the transition was entered. If this is not the case, we can remove the modal from the DOM when open is false.
oliviertassinari pushed a commit to ValentinH/material-ui that referenced this issue Jul 24, 2019
This should address mui#12831.

After following the comments in the above issue, I indeed noticed that `onExited` was not called. However, I've also noticed that `onEntered` was not called either. This is because the transition is not even rendered.

To solve this, I have added a flag that tells if the transition was entered. If this is not the case, we can remove the modal from the DOM when open is false.
oliviertassinari pushed a commit that referenced this issue Jul 25, 2019
* Modal: prevent backdrop to stay open when 'open' is flickering

This should address #12831.

After following the comments in the above issue, I indeed noticed that `onExited` was not called. However, I've also noticed that `onEntered` was not called either. This is because the transition is not even rendered.

To solve this, I have added a flag that tells if the transition was entered. If this is not the case, we can remove the modal from the DOM when open is false.

* [Modal] Fix exited state
@megphillips91

This comment has been minimized.

@jacknkandy
Copy link

I have just encountered this issue after upgrading from v5.10.0 to v5.11.10.

My issue is similar to @kamranayub, I am not using transitions, but have a situation where the dialog open state is immediately changing from true to false after the app launches.

I agree with @megphillips91 that this issue should perhaps be reopened. Although, I was able to solve my issue using the suggested disablePortal={true} fix, this is not described in the MUI documentation for Dialog components, and the reason this issue is happening is not clear/obvious.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: dialog This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants