From 34927f1f6bcac7d84fe8aedd4fd0628813f86448 Mon Sep 17 00:00:00 2001 From: Martin Hunt Date: Wed, 12 Oct 2016 21:27:08 -0400 Subject: [PATCH] [fixed] respect closeTimeoutMS during unmount Fixes issue #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. --- lib/components/Modal.js | 18 ++++++++ lib/components/ModalPortal.js | 10 ++-- specs/Modal.spec.js | 86 ++++++++++++++++++++++++++--------- 3 files changed, 88 insertions(+), 26 deletions(-) diff --git a/lib/components/Modal.js b/lib/components/Modal.js index 6ed9eaaa..2089f81e 100644 --- a/lib/components/Modal.js +++ b/lib/components/Modal.js @@ -105,6 +105,24 @@ export default class Modal extends Component { ariaAppHider.show(this.props.appElement); } + const state = this.portal.state; + const now = Date.now(); + const 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); const parent = getParentElement(this.props.parentSelector); parent.removeChild(this.node); diff --git a/lib/components/ModalPortal.js b/lib/components/ModalPortal.js index 246d07db..4d6aa2eb 100644 --- a/lib/components/ModalPortal.js +++ b/lib/components/ModalPortal.js @@ -132,8 +132,9 @@ export default class ModalPortal extends Component { } closeWithTimeout () { - this.setState({ beforeClose: true }, () => { - this.closeTimer = setTimeout(this.closeWithoutTimeout, this.props.closeTimeoutMS); + const closesAt = Date.now() + this.props.closeTimeoutMS; + this.setState({ beforeClose: true, closesAt }, () => { + this.closeTimer = setTimeout(this.closeWithoutTimeout, this.state.closesAt - Date.now()); }); } @@ -142,7 +143,8 @@ export default class ModalPortal extends Component { beforeClose: false, isOpen: false, afterOpen: false, - }, () => this.afterClose()); + closesAt: null + }, this.afterClose); } handleKeyDown = (event) => { @@ -182,7 +184,7 @@ export default class ModalPortal extends Component { } shouldBeClosed () { - return !this.props.isOpen && !this.state.beforeClose; + return !this.state.isOpen && !this.state.beforeClose; } contentHasFocus () { diff --git a/specs/Modal.spec.js b/specs/Modal.spec.js index 85479909..0427f408 100644 --- a/specs/Modal.spec.js +++ b/specs/Modal.spec.js @@ -18,6 +18,13 @@ const Simulate = TestUtils.Simulate; describe('Modal', () => { + afterEach('check if test cleaned up rendered modals', () => { + const overlay = document.querySelectorAll('.ReactModal__Overlay'); + const 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'); @@ -115,6 +122,7 @@ describe('Modal', () => { const modal = renderModal({ isOpen: true, onRequestClose () { + unmountModal(); unmountModal(); done(); } @@ -175,6 +183,7 @@ describe('Modal', () => { preventDefault () { tabPrevented = true; } }); expect(tabPrevented).toEqual(true); + unmountModal(); }); it('supports portalClassName', () => { @@ -204,26 +213,31 @@ describe('Modal', () => { it('overrides the default styles when a custom overlayClassName is used', () => { const modal = renderModal({ isOpen: true, overlayClassName: 'myOverlayClass' }); expect(modal.portal.overlay.style.backgroundColor).toEqual(''); + unmountModal(); }); it('supports adding style to the modal contents', () => { const modal = renderModal({ isOpen: true, style: { content: { width: '20px' } } }); expect(modal.portal.content.style.width).toEqual('20px'); + unmountModal(); }); it('supports overriding style on the modal contents', () => { const modal = renderModal({ isOpen: true, style: { content: { position: 'static' } } }); expect(modal.portal.content.style.position).toEqual('static'); + unmountModal(); }); it('supports adding style on the modal overlay', () => { const modal = renderModal({ isOpen: true, style: { overlay: { width: '75px' } } }); expect(modal.portal.overlay.style.width).toEqual('75px'); + unmountModal(); }); it('supports overriding style on the modal overlay', () => { const modal = renderModal({ isOpen: true, style: { overlay: { position: 'static' } } }); expect(modal.portal.overlay.style.position).toEqual('static'); + unmountModal(); }); it('supports overriding the default styles', () => { @@ -234,14 +248,17 @@ describe('Modal', () => { const modal = renderModal({ isOpen: true }); expect(modal.portal.content.style.position).toEqual(newStyle); Modal.defaultStyles.content.position = previousStyle; + unmountModal(); }); it('adds class to body when open', () => { 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); + unmountModal(); renderModal({ isOpen: false }); expect(document.body.className.indexOf('ReactModal__Body--open') !== -1).toEqual(false); @@ -323,6 +340,7 @@ describe('Modal', () => { expect(event).toBeTruthy(); expect(event.constructor).toBeTruthy(); expect(event.key).toEqual('FakeKeyToTestLater'); + unmountModal(); }); describe('should close on overlay click', () => { @@ -455,26 +473,50 @@ describe('Modal', () => { }); }); - // 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); + }); });