modal-open not being removed from body after closing modal #1323

justingrant · 2018-12-18T00:13:21Z

components: Modal
reactstrap version #6.5.0
import method es
react version #6.6.3
bootstrap version #4.1.3

What is happening?

The modal-open CSS class is not being removed from <body>when I close a modal. Clicking on the header's "x" button (or any other means of closing the modal) causes the page to be unscrollable.

What should be happening?

The modal-open style should be removed from the page when all modals are closed.

Investigation

Tracing through the Modal code in Chrome devtools, what I saw was two Modal instances being constructed but only one Modal instance is actually being used. The first Modal instance is being constructed by React but is never used after the constructor runs. It's being thrown away. None of its lifecycle methods are run. The second Modal instance is being used normally and its lifecycle methods fire, but when it closes, Modal.openCount is still nonzero so the modal-open style is not removed from the <body>.

Looking at Modal.js code, I saw a problem which explained the behavior above. The Modal constructor contains a side effect: a call to init() which sets modal-open on the <body>. The constructor (and hence init()) is being called twice, but destroy() is only being called once.

I was able to resolve the problem by forking reactstrap and moving the call to init() from the end of the constructor to the beginning of componentDidMount:

class Modal extends React.Component {
  constructor(props) {
    super(props);

    this._element = null;
    this._originalBodyPadding = null;
    this.getFocusableChildren = this.getFocusableChildren.bind(this);
    this.handleBackdropClick = this.handleBackdropClick.bind(this);
    this.handleBackdropMouseDown = this.handleBackdropMouseDown.bind(this);
    this.handleEscape = this.handleEscape.bind(this);
    this.handleTab = this.handleTab.bind(this);
    this.onOpened = this.onOpened.bind(this);
    this.onClosed = this.onClosed.bind(this);

    this.state = {
      isOpen: props.isOpen,
    };

    // removed
    // if (this.props.isOpen) {
    //   this.init();
    // }
  }

  componentDidMount() {
    // added
    if (this.props.isOpen) {
      this.init();
    }

    if (this.props.onEnter) {
      this.props.onEnter();
    }

    if (this.state.isOpen && this.props.autoFocus) {
      this.setFocus();
    }

    this._isMounted = true;
  }

React advises against side effects in constructors. I suspect the behavior above may be one reason for this recommendation. Apparently, React can construct instances and throw them away without running any lifecycle methods!

I'm happy to prepare a PR to move Modal initialization into componentDidMount(). But there may be other reasons that I'm not aware of to use this non-recommended side effect in the constructor.

Does anyone know the history of why init() is being called in the constructor instead of in componentDidMount where side effects and DOM manipulations are recommended?

Steps to reproduce issue

Repro steps are difficult at the moment because the problem shows up in the middle of a fairly complicated project. To display a modal, my app pushes data into an array that's stored in React Context (the new 16.x kind, not the deprecated old context). Another part of the app will notice the change to Context and will transform each element in the array into one or more modals. When a modal is closed, the onClosed event handler will remove the data from the array in Context which will cause the modal to be unmounted.

I can pull this logic out of my app to provide a simpler repro, but doing this will take some time so (per discussion above) I wanted to understand more about the reasons for initializing in the constructor before going through the trouble of creating a repro. Hope this is OK.

Error message in console

(none-- it's UI behavior, not an error, that's the problem)

Code

See above-- this behavior is deeply nested inside my app, so it's non-trivial to pull it out. I can do it if needed, though.

The text was updated successfully, but these errors were encountered:

nibblesnbits · 2019-05-06T03:02:22Z

+1, I just ran into this today. Any update?

bigorangemachine · 2019-05-10T14:48:30Z

@nibblesnbits,

Yes I came across this issue.

Had to downgrade. The issue is that init() is being called in the life cycle event while the modal is taking itself down.

It might be that its async with the animation so maybe try turning the fade off. Or in your close function just change Modal.openCount = 0; yourself.

Basically the modal count is always 1 (never 0 and thus never closes)

nibblesnbits · 2019-05-10T15:17:40Z

I see. Maybe I'll have time to do a PR this weekend for that. Here's hoping.

desfero · 2019-05-14T06:15:10Z

With react strict mode turned on class is almost never removed in development mode as strict mode may invoke constructor twice.

justingrant · 2019-05-22T20:49:08Z

BTW, it looks like this issue should be fixed by PR #1495 which was just merged.

justingrant · 2019-07-12T21:58:50Z

FYI, v8.0.1 was just released. It contains #1495 which should solve this problem. Closing.

Vishal1419 · 2019-09-20T15:07:56Z

I am using reactstrap with typescript. I am using v8.0.1, still I am facing the issue.

evolvan · 2019-09-25T10:04:13Z

I checked with updated version 8.0.1. The issue still there. It is something due to very quick update in its isOpen prop. I am using Modal in my Loader component. My Loaded component show/hide quickly.

timiles · 2019-10-18T09:55:30Z

This issue is still happening for me too, when my component's constructor initialises this.state = { isOpen: true };, and this state property is passed along into <Modal isOpen={this.state.isOpen}>...

I fixed it by initialising my own component's state to being false in the constructor, then setting it to true in componentDidMount. Then it seems the Modal knows it's added the class to the body, and therefore remembers to remove it again.

lucasfalcaoo · 2019-12-17T20:34:08Z

I'm still having this issue on v. ^8.2.0, does somebody have a workaround? For now I'm going to remove the modal-open class by force using: document.body.classList.remove('modal-open');

z1m1n · 2020-01-21T15:49:54Z

Still having this issue too

z1m1n · 2020-01-21T16:02:21Z

@justingrant please reopen

TheSharpieOne · 2020-01-21T16:33:05Z

@z1m1n This issue has been resolved. If you are experiencing a similar issue, please open a new issues and reference this issue. Please also provide a working minimal code sample which shows the issue (you can use https://stackblitz.com/edit/reactstrap-v8?file=Example.js as a place to start)

Also note that this was more properly fixed in the latest release (8.4.0). Please ensure you are using the latest version. Checkout this commit and commit comment: 310b061

saysmaster · 2020-02-02T22:17:44Z

8.4.1 the issue is still here. I don't know how to reproduce. It happened after I recently upgraded. The project is big and several months old.

rodlukas · 2020-02-23T16:12:44Z

@saysmaster I also had this problem with project ~2 years old, beginning with reactstrap v8.3.0 the classname modal-open wasn't removed after modal submit. Version 8.2.0 didn't have this problem. I found the problem and solve it, maybe it will also help to you.

After hours of debugging and reviewing the reactstrap code and changes introduced between 8.2.0 and 8.4.1 I found the cause - it's because of changes introduced with 310b061. However, from my understanding, it's not a bug in the reactstrap but it's the consequence of wrong usage of the Modal component.

Description of what was hapenning in my code:

Situation: There's a component with Modal, this Modal has inside other component with some form. When the form is submitted, onSubmit in the form component calls two functions from the parent:
1. modal close (ie. setModal that sets state of isOpen in parent to false),
2. refresh of some ancestor component (that has this modal component as children).
Thanks to setModal, we set the prop isOpen to false on Modal component, but in the meantime the refresh from the parent component causes the Modal to unmount from DOM. Then the Modal is not able to successfully end the operation for close (reactstrap Modal function onClosed), instead of this the componentWillUnmount method of modal starts on unmount. This method is now not able to completely remove the modal (mainly modal-open attribute on body) because it would remove this attribute only if this.props.isOpen === true, but this.props.isOpen is set thanks to setModal to false.
In the versions <= 8.2.0 my code worked because the componentWillUnmount method was based on the state: this.state.isOpen === true in modal method componentWillUnmount was considered to be true thanks to the fact that propagation of props changes to the state (in method onClosed) was interrupted by method componentWillUnmount that saves this situation because this.state.isOpen === true is true.
Impact of this behaviour can also be observed on the modal close animation - when onClosed does not correctly end, modal is unmounted, forced to close and there is no animation on close.

TLDR description:

Modal is forced to unmount, modal-open is not removed. In previous versions the modal method componentWillUnmount corrected this behaviour (probably unintentionally). Versions >=8.3.0 introduced one important bugfix 310b061 that removed this unintentional helper that corrected the behaviour and let the bug in the code appear.

Solution:

The form in the modal had a prop with function for refresh of the ancestor components that was called after the form submit. This function caused the modal to unmount before the modal-open could be removed. This function can't be called from the children component, it should passed to modal prop onClosed, the modal will then call it after everything is done (modal is fully closed) and prepared for the calling. OnSubmit of the inner form will only call the function that sets isOpen prop on modal to false.
Thanks to this change the modal is now able to close completely (without unmount interrupting the closing process). After that, the onClosed function can be processed, the modal can be unmounted and the refresh from some ancestor component can occur.

TheSharpieOne · 2020-02-25T14:47:51Z

This bug existed before 310b061 was created so it's not possible for 310b061 to introduce it. I believe there are multiple factors in play. Both cases are race conditions which makes them difficult to reproduce and difficult to determine there was more than one scenario.

The difference between state and props is that state is async, leading to issues when the modal unmounts before the state has been updated to reflect the value of the prop which is what 310b061 aimed to fix. With props, the cleanup code run synchronously which means the cleanup happens before other code runs which would unmount the component.

But based on your finding, I believe the issue lies with how onClosed is supposed to be trigger when props.isOpen is false. In that case, CSS transition group is set to trigger it on onExited, but it doesn't seem to be happening when CSS transition group is unmounted.

Currently, here are the possibilities during unmount:

STATE	PROP	Reason	Has body class	`close` Triggered?
`false`	`false`	not open, not opening	No	No
`true`	`false`	open, but closing next render	Yes	No
`false`	`true`	closed, but opening next render	Yes	Yes
`true`	`true`	open (not closing)	Yes	Yes

Based on that truth table, maybe we can just check if either state.isOpen or prop.isOpen is true in order to trigger close during unmount?

rodlukas · 2020-02-25T15:44:28Z

@TheSharpieOne nice overview, looks good to me. The check if state.isOpen or prop.isOpen is true should solve all these problems with modal-open attribute completely, including the problem I was writing about and also won't break the fixed one (#1626) :)

#1323

SteiNNx · 2020-03-03T17:55:22Z

Fixed my problem for moment...

import React from 'react';
import ReactDOM from 'react-dom';
import { Provider } from 'react-redux';
import { Modal } from "reactstrap";
import 'normalize.css/normalize.css';
import 'alertifyjs/build/css/alertify.min.css';
import 'alertifyjs/build/css/themes/bootstrap.min.css';

import store from '@/redux/store';
import fetchInitialData from '@/redux/fetchInitialData';
import Root from './screens/Root';
import '@/components/PdfViewer/pdfJsPageChange';
/** import  Playground from '@/playground'; */


// store.subscribe(() => console.log(store.getState()));
fetchInitialData();

/** Hotfix css-modal */
Modal.prototype.componentWillUnmount = function() {
  if (this.props.onExit) {
    this.props.onExit();
  }

  if (this._element) {
    this.destroy();
    if (this.props.isOpen || this.state.isOpen) {
      this.close();
    }
  }

  this._isMounted = false;
};
/** Hotfix css-modal */

ReactDOM.render(
  <Provider store={store}>
    <Root />
  </Provider>
  , document.getElementById('ROOT')
);

AKhoo · 2020-05-16T18:41:40Z

This was still an issue for me when I implemented a loading modal in a Layout component shared across all pages. My Modal's isOpen prop was set to read an isLoading prop stored in Redux state. modal-open was not being removed from body, even on reactstrap 8.4.1.

Solution: I added an effect in my modal to forcefully remove the modal-open class on body.

const LoadingModal = (props) => {
  const { isLoading } = props;

  useEffect(() => {
    if (!isLoading) {
      document.querySelector('body').classList.remove('modal-open');
    };
  }, [isLoading])

  return (
    <Modal isOpen={isLoading}>
        Loading....
    </Modal>
  );
}

rodlukas · 2020-05-16T19:03:03Z

@AKhoo the fix is merged in master but not released yet

@TheSharpieOne is there any ETA for the release?

gidomanders · 2020-06-12T10:51:04Z

Is there any ETA for the release? I expected a patch release already since the last release was months ago...

The modal-open class remains on the body tag even after the modal is closed. This is an issue of the reactstrap library (reactstrap/reactstrap#1323) which is already fixed buy not released yet. Added a temporary work-around to remove the modal-open class when modal is closed. Closes #426

ATLUSio · 2021-09-30T16:38:09Z

I was able to resolve this issue with the following CSS:

body:not(.modal-open) {
  overflow-y: scroll !important;
}

My issue was the class being removed but the CSS somehow staying. So, when the modal is not open I'm forcing it to allow scrolling, otherwise the modal's css will apply.

justingrant mentioned this issue May 22, 2019

fix(Modal): handle init modal in SSR #1495

Merged

12 tasks

justingrant mentioned this issue Jul 10, 2019

ETA of next release? #1568

Closed

justingrant closed this as completed Jul 12, 2019

TheSharpieOne added a commit that referenced this issue Feb 25, 2020

fix(Modal): fix modal close immediately unmounted

c8def29

#1323

gidomanders mentioned this issue Jun 12, 2020

Modal not closing properly 42BV/ui#426

Closed

gidomanders mentioned this issue Jun 22, 2020

fix: modal-open class remains on body after modal closed 42BV/ui#437

Merged

TheSharpieOne mentioned this issue Jun 23, 2020

chore(release): adding 8.5.0 #1854

Merged

notcruz mentioned this issue Mar 8, 2023

Development/lab 7 all-rit/ALL#216

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

modal-open not being removed from body after closing modal #1323

modal-open not being removed from body after closing modal #1323

justingrant commented Dec 18, 2018 •

edited

nibblesnbits commented May 6, 2019

bigorangemachine commented May 10, 2019 •

edited

nibblesnbits commented May 10, 2019

desfero commented May 14, 2019

justingrant commented May 22, 2019

justingrant commented Jul 12, 2019

Vishal1419 commented Sep 20, 2019

evolvan commented Sep 25, 2019

timiles commented Oct 18, 2019

lucasfalcaoo commented Dec 17, 2019

z1m1n commented Jan 21, 2020

z1m1n commented Jan 21, 2020

TheSharpieOne commented Jan 21, 2020 •

edited

saysmaster commented Feb 2, 2020

rodlukas commented Feb 23, 2020 •

edited

TheSharpieOne commented Feb 25, 2020 •

edited

rodlukas commented Feb 25, 2020 •

edited

SteiNNx commented Mar 3, 2020

AKhoo commented May 16, 2020

rodlukas commented May 16, 2020

gidomanders commented Jun 12, 2020

ATLUSio commented Sep 30, 2021

modal-open not being removed from body after closing modal #1323

modal-open not being removed from body after closing modal #1323

Comments

justingrant commented Dec 18, 2018 • edited

What is happening?

What should be happening?

Investigation

Steps to reproduce issue

Error message in console

Code

nibblesnbits commented May 6, 2019

bigorangemachine commented May 10, 2019 • edited

nibblesnbits commented May 10, 2019

desfero commented May 14, 2019

justingrant commented May 22, 2019

justingrant commented Jul 12, 2019

Vishal1419 commented Sep 20, 2019

evolvan commented Sep 25, 2019

timiles commented Oct 18, 2019

lucasfalcaoo commented Dec 17, 2019

z1m1n commented Jan 21, 2020

z1m1n commented Jan 21, 2020

TheSharpieOne commented Jan 21, 2020 • edited

saysmaster commented Feb 2, 2020

rodlukas commented Feb 23, 2020 • edited

Description of what was hapenning in my code:

TLDR description:

Solution:

TheSharpieOne commented Feb 25, 2020 • edited

rodlukas commented Feb 25, 2020 • edited

SteiNNx commented Mar 3, 2020

AKhoo commented May 16, 2020

rodlukas commented May 16, 2020

gidomanders commented Jun 12, 2020

ATLUSio commented Sep 30, 2021

justingrant commented Dec 18, 2018 •

edited

bigorangemachine commented May 10, 2019 •

edited

TheSharpieOne commented Jan 21, 2020 •

edited

rodlukas commented Feb 23, 2020 •

edited

TheSharpieOne commented Feb 25, 2020 •

edited

rodlukas commented Feb 25, 2020 •

edited