Reactstrap: modal-open not being removed from body after closing modal

Created on 18 Dec 2018 · 21Comments · Source: reactstrap/reactstrap

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.

Source

justingrant

👍6

Most helpful comment

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.

saysmaster on 2 Feb 2020

👍6

All 21 comments

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

nibblesnbits on 6 May 2019

@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)

bigorangemachine on 10 May 2019

👍1

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

nibblesnbits on 10 May 2019

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

desfero on 14 May 2019

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

justingrant on 22 May 2019

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

justingrant on 12 Jul 2019

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

Vishal1419 on 20 Sep 2019

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.

evolvan on 25 Sep 2019

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.

timiles on 18 Oct 2019

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');

lucasfalcaoo on 17 Dec 2019

👍2 ❤1

Still having this issue too

z1m1n on 21 Jan 2020

@justingrant please reopen

z1m1n on 21 Jan 2020

@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: 310b06192c597

TheSharpieOne on 21 Jan 2020

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.

saysmaster on 2 Feb 2020

👍6

@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 https://github.com/reactstrap/reactstrap/commit/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 https://github.com/reactstrap/reactstrap/commit/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.

rodlukas on 23 Feb 2020

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?

TheSharpieOne on 25 Feb 2020

@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 (https://github.com/reactstrap/reactstrap/issues/1626) :)

rodlukas on 25 Feb 2020

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')
);

SteiNNx on 3 Mar 2020

👍1

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 (
Loading....

);
}
````