[fixed] respect closeTimeoutMS during unmount

Fixes issue reactjs#17: "When the modal is unmounted, it will abruptly close, not waiting for any animations to finish." The bug was caused by the Modal component unmounting the portal immediately in `componentWillUnmount` regardless of whether the portal is currently closing or would animate to close. Now when a Modal has a non-zero `closeTimeoutMS`, the Modal inspects the portal state before closing. If the portal is open or closing, but not closed, it waits to unmount the portal. If the portal is open and not already closing, the Modal calls closeWithTimeout() to trigger the close. Adds test to ensure portal DOM persists after Modal is unmounted when the Modal has a `closeTimeoutMS` set. Updates existing tests to properly unmount test Modal instances.
loganscharen · Feb 26, 2017 · ea4f37a · ea4f37a
1 parent f6768b7
commit ea4f37a
Show file tree

Hide file tree

Showing 3 changed files with 86 additions and 34 deletions.
diff --git a/lib/components/Modal.js b/lib/components/Modal.js
@@ -83,6 +83,24 @@ var Modal = React.createClass({
       ariaAppHider.show(this.props.appElement);
     }
 
+    var state = this.portal.state;
+    var now = Date.now();
+    var closesAt = state.isOpen && this.props.closeTimeoutMS
+      && (state.closesAt
+        || now + this.props.closeTimeoutMS);
+
+    if (closesAt) {
+      if (!state.beforeClose) {
+        this.portal.closeWithTimeout();
+      }
+
+      setTimeout(this.removePortal.bind(this), closesAt - now);
+    } else {
+      this.removePortal();
+    }
+  },
+
+  removePortal () {
     ReactDOM.unmountComponentAtNode(this.node);
     var parent = getParentElement(this.props.parentSelector);
     parent.removeChild(this.node);

diff --git a/lib/components/ModalPortal.js b/lib/components/ModalPortal.js
@@ -109,8 +109,9 @@ var ModalPortal = module.exports = React.createClass({
   },
 
   closeWithTimeout: function() {
-    this.setState({beforeClose: true}, function() {
-      this.closeTimer = setTimeout(this.closeWithoutTimeout, this.props.closeTimeoutMS);
+    var closesAt = Date.now() + this.props.closeTimeoutMS;
+    this.setState({beforeClose: true, closesAt: closesAt}, function() {
+      this.closeTimer = setTimeout(this.closeWithoutTimeout, this.state.closesAt - Date.now());
     }.bind(this));
   },
 
@@ -119,7 +120,8 @@ var ModalPortal = module.exports = React.createClass({
       beforeClose: false,
       isOpen: false,
       afterOpen: false,
-    }, function () { this.afterClose() }.bind(this))
+      closesAt: null
+    }, this.afterClose);
   },
 
   handleKeyDown: function(event) {
@@ -158,7 +160,7 @@ var ModalPortal = module.exports = React.createClass({
   },
 
   shouldBeClosed: function() {
-    return !this.props.isOpen && !this.state.beforeClose;
+    return !this.state.isOpen && !this.state.beforeClose;
   },
 
   contentHasFocus: function() {

diff --git a/specs/Modal.spec.js b/specs/Modal.spec.js
@@ -9,7 +9,13 @@ const Simulate = TestUtils.Simulate;
 import sinon from 'sinon';
 import expect from 'expect';
 
-describe('Modal', function () {
+describe('Modal', () => {
+  afterEach('check if test cleaned up rendered modals', function () {
+    var overlay = document.querySelectorAll('.ReactModal__Overlay');
+    var content = document.querySelectorAll('.ReactModal__Content');
+    expect(overlay.length).toBe(0);
+    expect(content.length).toBe(0);
+  });
 
   it('scopes tab navigation to the modal');
   it('focuses the last focused element when tabbing in from browser chrome');
@@ -116,7 +122,7 @@ describe('Modal', function () {
         unmountModal();
         done();
       }
-    }, null, function () {});
+    }, null);
 
     renderModal({
       isOpen: true,
@@ -174,6 +180,7 @@ describe('Modal', function () {
         preventDefault: function() { tabPrevented = true; }
     });
     expect(tabPrevented).toEqual(true);
+    unmountModal();
   });
 
   it('supports portalClassName', function () {
@@ -236,15 +243,16 @@ describe('Modal', function () {
   });
 
   it('adds class to body when open', function() {
-    renderModal({isOpen: false});
+    renderModal({ isOpen: false });
     expect(document.body.className.indexOf('ReactModal__Body--open') !== -1).toEqual(false);
+    unmountModal();
 
-    renderModal({isOpen: true});
-    expect(document.body.className.indexOf('ReactModal__Body--open')  !== -1).toEqual(true);
-
-    renderModal({isOpen: false});
-    expect(document.body.className.indexOf('ReactModal__Body--open')  !== -1).toEqual(false);
+    renderModal({ isOpen: true });
+    expect(document.body.className.indexOf('ReactModal__Body--open') !== -1).toEqual(true);
     unmountModal();
+
+    renderModal({ isOpen: false });
+    expect(document.body.className.indexOf('ReactModal__Body--open') !== -1).toEqual(false);
   });
 
   it('removes class from body when unmounted without closing', function() {
@@ -441,26 +449,50 @@ describe('Modal', function () {
     expect(event.constructor.name).toEqual('SyntheticEvent');
   });
 
-  //it('adds --before-close for animations', function() {
-    //var node = document.createElement('div');
-
-    //var component = ReactDOM.render(React.createElement(Modal, {
-      //isOpen: true,
-      //ariaHideApp: false,
-      //closeTimeoutMS: 50,
-    //}), node);
-
-    //component = ReactDOM.render(React.createElement(Modal, {
-      //isOpen: false,
-      //ariaHideApp: false,
-      //closeTimeoutMS: 50,
-    //}), node);
-
-    // It can't find these nodes, I didn't spend much time on this
-    //var overlay = document.querySelector('.ReactModal__Overlay');
-    //var content = document.querySelector('.ReactModal__Content');
-    //ok(overlay.className.match(/ReactModal__Overlay--before-close/));
-    //ok(content.className.match(/ReactModal__Content--before-close/));
-    //unmountModal();
-  //});
+  it('adds --before-close for animations', () => {
+    const closeTimeoutMS = 50;
+    const modal = renderModal({
+      isOpen: true,
+      closeTimeoutMS
+    });
+
+    modal.portal.closeWithTimeout();
+
+    const overlay = TestUtils.findRenderedDOMComponentWithClass(modal.portal, 'ReactModal__Overlay');
+    const content = TestUtils.findRenderedDOMComponentWithClass(modal.portal, 'ReactModal__Content');
+
+    expect(/ReactModal__Overlay--before-close/.test(overlay.className)).toBe(true);
+    expect(/ReactModal__Content--before-close/.test(content.className)).toBe(true);
+
+    modal.portal.closeWithoutTimeout();
+    unmountModal();
+  });
+
+  it('keeps the modal in the DOM until closeTimeoutMS elapses', (done) => {
+    const closeTimeoutMS = 50;
+
+    renderModal({
+      isOpen: true,
+      closeTimeoutMS
+    });
+
+    unmountModal();
+
+    const checkDOM = (expectMounted) => {
+      const overlay = document.querySelectorAll('.ReactModal__Overlay');
+      const content = document.querySelectorAll('.ReactModal__Content');
+      const numNodes = expectMounted ? 1 : 0;
+      expect(overlay.length).toBe(numNodes);
+      expect(content.length).toBe(numNodes);
+    };
+
+    // content is still mounted after modal is gone
+    checkDOM(true);
+
+    setTimeout(() => {
+      // content is unmounted after specified timeout
+      checkDOM(false);
+      done();
+    }, closeTimeoutMS);
+  });
 });