Modal does not work with React 16

[jochenberger]

Probably the same issue as facebook/react#9808.
When the Modal is shown and tries to get focus (via componentDidUpdate -> onShow - focus), setModalNode will not have been called and getDialogElement will return undefined.

[jochenberger]

https://github.com/callemall/material-ui/pull/7218/files might be helpful here.

[jquense]

Fo v16 we'll want to switch to the new (YAH!) portal api

[billschaller]

Edit: I had a subpackage still referencing the older version of react-bootstrap. Latest is still broken with React 16 though :(

@jquense @jochenberger Current shipping version of react-bootstrap is still referencing react-overlay@0.7.0. That version is still using string refs, and that's not working in the Modal component for some reason with React 16.

Would you consider hotfixing this? Kind of a blocker :(

[jquense]

we can't hotfix RB to v0.8 because there are a few breaking changes, we need to do it on a major bump of RB

[billschaller]

Could you push an @next version to npm?

[jquense]

I'm not sure that would even help no version of RO uses the new portal api yet.

string refs should work still with the old API I would open an issue on the React repo since its a v16 bug

[billschaller]

Ah - Part of my app was on 30.10, and the string ref was still there.

The latest release is still broken in 16 though.

It looks like the child component of Portal passed in by Modal is not mounted by the time Modal.componentDidUpdate is called - this makes sense, as AFAIK that lifecycle callback doesn't fire until the commit phase. The proper thing to do is to have the child you pass to the Portal component kick off onShow instead of doing it from cDU or cDM in the Modal component itself.

[billschaller]

@jquense Sent you a chat request

[jquense]

@billschaller sorry just saw the request. You are better off pinging me in the react-bootstrap discord channel https://discord.gg/0ZcbPKXt5bXLs9XK i'm monasticpanic

[KagamiChan]

Is there any news on the landing of RO on RB? I'd like to help but I don't know what are the tasks to be done.

[jochenberger]

facebook/react#10675 is probably also relevant.

[jquense]

@KagamiChan please feel free to jump in here. The tasks are to upgrade to the new createPortal api while (if possible) also supporting v15. So some amount of feature detecting and branching is needed. Portal should be the only component that needs an upgrade.

[KagamiChan]

thank you for your reply, will take some time to learn about this

[jochenberger]

I just created #201 to make some progress on this. Any help is appreciated.

[jochenberger]

@jquense, I don't think that this is the solution (see #201). I think that @billschaller pointed out the issue in #188 (comment). I got some of the tests to run by wrapping the div into a component that calls Modal.show it its componentDidMount. But I wonder if there is a way to do what @billschaller suggested without the need for an additional wrapper component.

[grantila]

FYI React 16 is released, so this just became a blocker for people to be able to upgrade.

[billschaller]

This shim has worked fine for us - it bypasses some functionality but it got us unblocked.

const {Modal} = require('react-overlays');

const focus = () => {};

const cDU = Modal.prototype.componentDidUpdate;

Modal.prototype.componentDidUpdate = function(prevProps: any) {
  if (this.focus !== focus) {
    this.focus = focus;
  }
  cDU.call(this, prevProps);
};

[Calyhre]

😄 I'm also using this in the meantime :

import { Modal as ReactOverlayModal } from 'react-overlays';

class Modal extends ReactOverlayModal {
  focus = () => {};
}

export default Modal;

[jquense]

you can see that there are a few efforts to fix this. IF you want stuff to go faster jump in on the PR's and review/test them

[patsissons]

I dug into this a bit to see what the root problem was. The core problem is react-dom's commitAllLifeCycles invokes commitAttachRef after commitLifeCycles. This means that setModalNode (which assigns the local modalNode field) is invoked after componentDidMount (which calls onShow and by extension focus).

What this means is that (given that react-dom does not change its commitAllLifeCycles function), we remove the onShow from componentDidMount and move it to setModalNode. I have tested this and it most definitely works as expected.

  setModalNode = (ref) => {
    this.modalNode = ref;
    if (ref && this.props.show) {
      this.onShow();
    }
  }

as a temporary shim, this should work for most setups.

import { Modal } from 'react-overlays';
Modal.prototype.componentWillMount = function() {
  this.setModalNode = function(ref: any) {
    this.modalNode = ref;
    if (ref != null && this.props.show) {
      this.onShow();
    }
  }.bind(this);
};
Modal.prototype.componentDidMount = function() {
  this._isMounted = true;
};

[ptkoz]

I'd just like to note that @patsissons solution obviously does not work when you're doing something like this:

class MyModal extends React.PureComponent {
    constructor(props) {
         super(props)
         this.state = { visible: false };
    }

    componentDidMount() {
         this.setState({ visible: true });
    }

    render() {
        return <Modal {...this.props} show={this.state.visible}> ... </Modal>
    }
}

as onShow is then called from Modal's componentDidUpdate, not componentDidMount.

// edit: fixed code

[jeremy-colin]

Hey! Anything we can do to help?

[TryingToImprove]

@jeremy-colin There seems to be some WIP pull requests.

[mglrahul]

hi guys,

i tried to use the patches provided here but couldn't able to do that. can anyone help me out in this matter. i already post my question here.

looking for help. @patsissons @pamelus @davidworkman9 @connorjburton @Calyhre

thanks for help in advance.

[connorjburton]

@mglrahul you are probably best off waiting for the official patch to be honest

[mglrahul]

@connorjburton actually i urgently need to fix the issue in my project. so if i get any working patch solution than that's fine, we can fix the temporary with the official update later. but right now i need a fix. if you can give me some idea it will be great.

i already try to integrate the patches mentioned above in my project, but couldn't be able to make it workable.

[patsissons]

@mglrahul see my post above (#188 (comment)) it addresses how to get this up and running with minimal side effects. The shim in the post should patch the react-overlay module to work in react 16. otherwise just downgrade to react 15 where the lifecycle sequence is compatible with these modals.

[mglrahul]

hey @patsissons thanks for replying, i read #188 commit, but couldn't understand where to put the patch you provided. do i need to put it in component or js library. below is my component.

import { Button, Modal } from 'react-bootstrap';

export default class ModelFirst extends Component {
  constructor(props) {
    super(props);

    this.state = {
      showModal: false
    };

    this.open = this.open.bind(this);
    this.close = this.close.bind(this);
  }

  close() {
    this.setState({ showModal: false });
  }

  open() {
    console.log('open');
    this.setState({ showModal: true });
  }

  render() {
    return (
      <div>
        <p>Click to get the full Modal experience!</p>
        <Button
          bsStyle="primary"
          bsSize="large"
          onClick={this.open}
        >
          Launch demo modal
        </Button>

        <Modal show={this.state.showModal} onHide={this.close}>
          <Modal.Header closeButton>
            <Modal.Title>Modal heading</Modal.Title>
          </Modal.Header>
          <Modal.Body>
            <h4>Overflowing text to show scroll behavior</h4>
            <p>Cras mattis consectetur purus sit amet fermentum. Cras justo odio, dapibus ac facilisis in, egestas eget quam. Morbi leo risus, porta ac consectetur ac, vestibulum at eros.</p>
            <p>Praesent commodo cursus magna, vel scelerisque nisl consectetur et. Vivamus sagittis lacus vel augue laoreet rutrum faucibus dolor auctor.</p>
            <p>Aenean lacinia bibendum nulla sed consectetur. Praesent commodo cursus magna, vel scelerisque nisl consectetur et. Donec sed odio dui. Donec ullamcorper nulla non metus auctor fringilla.</p>
          </Modal.Body>
          <Modal.Footer>
            <Button onClick={this.close}>Close</Button>
          </Modal.Footer>
        </Modal>
      </div>
    );
  }
};

this is the question i asked on stackoverflow.
I appreciate your help, thanks in advance. :-)

[seeden]

If you are using webpack you need to use this fix like this:

import Modal from 'react-bootstrap/node_modules/react-overlays/lib/Modal';

Modal.prototype.componentWillMount = function componentWillMount() {
  this.focus = function focus() {};
};

[mglrahul]

@seeden i try to use your solution in the component but i get some error, below is the error i got

Modal.prototype.componentWillMount = function componentWillMount() {
     |        ^
  20 |     this.focus = () => {};
  21 |   };
  22 | 

i have put the code out of render in component.

[zdila]

@mglrahul put it out of component

[erise133]

@mglrahul You can do that kind of walk around in your entry point for js files. main.js or index.js. Just make sure you have imported Modal from react-overlays

[astanciu]

Is this patch the "official" response for react-bootstrap on react16 or is there WIP to get this fixed properly?

[jharris4]

@astanciu #208 is the WIP. Looks like it's getting close to being ready...

[mglrahul]

finally i able to make it work, thanks to all the kind people @szczepanbarszczowski @zdila @seeden @patsissons @connorjburton

without all your help it won't work

[jharris4]

version 0.7.3 seems to have fixed the issue.

Thanks for the work on this!

[geminiyellow]

still not working

TypeError: Cannot read property 'contains' of undefined
    at app-bundle-9f3367cd4c8da193f2df7875ec037f39ff0441e251ba61c728a43c8363174343.js:24030
    at Modal.focus (app-bundle-9f3367cd4c8da193f2df7875ec037f39ff0441e251ba61c728a43c8363174343.js:65592)
    at Modal.onShow (app-bundle-9f3367cd4c8da193f2df7875ec037f39ff0441e251ba61c728a43c8363174343.js:65521)
    at Modal.componentDidUpdate (app-bundle-9f3367cd4c8da193f2df7875ec037f39ff0441e251ba61c728a43c8363174343.js:65278)
    at commitLifeCycles (vendor-69769756228c5b97a31cdefe85d1499ffa9865cebae44c24682bb47a8949d3eb.js:20358)
    at c (vendor-69769756228c5b97a31cdefe85d1499ffa9865cebae44c24682bb47a8949d3eb.js:20369)
    at k (vendor-69769756228c5b97a31cdefe85d1499ffa9865cebae44c24682bb47a8949d3eb.js:20372)
    at p (vendor-69769756228c5b97a31cdefe85d1499ffa9865cebae44c24682bb47a8949d3eb.js:20373)
    at batchedUpdates (vendor-69769756228c5b97a31cdefe85d1499ffa9865cebae44c24682bb47a8949d3eb.js:20379)
    at qb (vendor-69769756228c5b97a31cdefe85d1499ffa9865cebae44c24682bb47a8949d3eb.js:20230)

[bernharduw]

@geminiyellow this has been fixed in 0.7.3, but not yet in the 0.8 branch (as of 0.8.2).

I worked around that issue by creating a custom Modal wrapper component which monkey-patches cDM and cDU as outlined above:

import Modal from 'react-overlays/lib/Modal';

const focus = () => {};

const cDU = Modal.prototype.componentDidUpdate;
const cDM = Modal.prototype.componentDidMount;

// react-overlays v0.8.2 doesn't work with React 16, so we have to monkey-patch it.
// TODO remove this once react-overlays is compatible with React 16.
Modal.prototype.componentDidUpdate = function componentDidUpdatePatched(prevProps: any) {
  if (this.focus !== focus) {
    this.focus = focus;
  }
  cDU.call(this, prevProps);
};

Modal.prototype.componentDidMount = function componentDidMountPatched() {
  if (this.focus !== focus) {
    this.focus = focus;
  }
  cDM.call(this);
};

export default Modal;

Once 0.7.3 was released, I just downgraded the react-overlays version from ^0.8.2 to ^0.7.3, so that patch isn't needed any more.

[geminiyellow]

@billschaller thank you 👍