Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit ea4f37a

Browse files
mghclaydiffrient
authored andcommittedFeb 26, 2017
[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.
1 parent f6768b7 commit ea4f37a

File tree

3 files changed

+86
-34
lines changed

3 files changed

+86
-34
lines changed
 

‎lib/components/Modal.js

+18
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,24 @@ var Modal = React.createClass({
8383
ariaAppHider.show(this.props.appElement);
8484
}
8585

86+
var state = this.portal.state;
87+
var now = Date.now();
88+
var closesAt = state.isOpen && this.props.closeTimeoutMS
89+
&& (state.closesAt
90+
|| now + this.props.closeTimeoutMS);
91+
92+
if (closesAt) {
93+
if (!state.beforeClose) {
94+
this.portal.closeWithTimeout();
95+
}
96+
97+
setTimeout(this.removePortal.bind(this), closesAt - now);
98+
} else {
99+
this.removePortal();
100+
}
101+
},
102+
103+
removePortal () {
86104
ReactDOM.unmountComponentAtNode(this.node);
87105
var parent = getParentElement(this.props.parentSelector);
88106
parent.removeChild(this.node);

‎lib/components/ModalPortal.js

+6-4
Original file line numberDiff line numberDiff line change
@@ -109,8 +109,9 @@ var ModalPortal = module.exports = React.createClass({
109109
},
110110

111111
closeWithTimeout: function() {
112-
this.setState({beforeClose: true}, function() {
113-
this.closeTimer = setTimeout(this.closeWithoutTimeout, this.props.closeTimeoutMS);
112+
var closesAt = Date.now() + this.props.closeTimeoutMS;
113+
this.setState({beforeClose: true, closesAt: closesAt}, function() {
114+
this.closeTimer = setTimeout(this.closeWithoutTimeout, this.state.closesAt - Date.now());
114115
}.bind(this));
115116
},
116117

@@ -119,7 +120,8 @@ var ModalPortal = module.exports = React.createClass({
119120
beforeClose: false,
120121
isOpen: false,
121122
afterOpen: false,
122-
}, function () { this.afterClose() }.bind(this))
123+
closesAt: null
124+
}, this.afterClose);
123125
},
124126

125127
handleKeyDown: function(event) {
@@ -158,7 +160,7 @@ var ModalPortal = module.exports = React.createClass({
158160
},
159161

160162
shouldBeClosed: function() {
161-
return !this.props.isOpen && !this.state.beforeClose;
163+
return !this.state.isOpen && !this.state.beforeClose;
162164
},
163165

164166
contentHasFocus: function() {

‎specs/Modal.spec.js

+62-30
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,13 @@ const Simulate = TestUtils.Simulate;
99
import sinon from 'sinon';
1010
import expect from 'expect';
1111

12-
describe('Modal', function () {
12+
describe('Modal', () => {
13+
afterEach('check if test cleaned up rendered modals', function () {
14+
var overlay = document.querySelectorAll('.ReactModal__Overlay');
15+
var content = document.querySelectorAll('.ReactModal__Content');
16+
expect(overlay.length).toBe(0);
17+
expect(content.length).toBe(0);
18+
});
1319

1420
it('scopes tab navigation to the modal');
1521
it('focuses the last focused element when tabbing in from browser chrome');
@@ -116,7 +122,7 @@ describe('Modal', function () {
116122
unmountModal();
117123
done();
118124
}
119-
}, null, function () {});
125+
}, null);
120126

121127
renderModal({
122128
isOpen: true,
@@ -174,6 +180,7 @@ describe('Modal', function () {
174180
preventDefault: function() { tabPrevented = true; }
175181
});
176182
expect(tabPrevented).toEqual(true);
183+
unmountModal();
177184
});
178185

179186
it('supports portalClassName', function () {
@@ -236,15 +243,16 @@ describe('Modal', function () {
236243
});
237244

238245
it('adds class to body when open', function() {
239-
renderModal({isOpen: false});
246+
renderModal({ isOpen: false });
240247
expect(document.body.className.indexOf('ReactModal__Body--open') !== -1).toEqual(false);
248+
unmountModal();
241249

242-
renderModal({isOpen: true});
243-
expect(document.body.className.indexOf('ReactModal__Body--open') !== -1).toEqual(true);
244-
245-
renderModal({isOpen: false});
246-
expect(document.body.className.indexOf('ReactModal__Body--open') !== -1).toEqual(false);
250+
renderModal({ isOpen: true });
251+
expect(document.body.className.indexOf('ReactModal__Body--open') !== -1).toEqual(true);
247252
unmountModal();
253+
254+
renderModal({ isOpen: false });
255+
expect(document.body.className.indexOf('ReactModal__Body--open') !== -1).toEqual(false);
248256
});
249257

250258
it('removes class from body when unmounted without closing', function() {
@@ -441,26 +449,50 @@ describe('Modal', function () {
441449
expect(event.constructor.name).toEqual('SyntheticEvent');
442450
});
443451

444-
//it('adds --before-close for animations', function() {
445-
//var node = document.createElement('div');
446-
447-
//var component = ReactDOM.render(React.createElement(Modal, {
448-
//isOpen: true,
449-
//ariaHideApp: false,
450-
//closeTimeoutMS: 50,
451-
//}), node);
452-
453-
//component = ReactDOM.render(React.createElement(Modal, {
454-
//isOpen: false,
455-
//ariaHideApp: false,
456-
//closeTimeoutMS: 50,
457-
//}), node);
458-
459-
// It can't find these nodes, I didn't spend much time on this
460-
//var overlay = document.querySelector('.ReactModal__Overlay');
461-
//var content = document.querySelector('.ReactModal__Content');
462-
//ok(overlay.className.match(/ReactModal__Overlay--before-close/));
463-
//ok(content.className.match(/ReactModal__Content--before-close/));
464-
//unmountModal();
465-
//});
452+
it('adds --before-close for animations', () => {
453+
const closeTimeoutMS = 50;
454+
const modal = renderModal({
455+
isOpen: true,
456+
closeTimeoutMS
457+
});
458+
459+
modal.portal.closeWithTimeout();
460+
461+
const overlay = TestUtils.findRenderedDOMComponentWithClass(modal.portal, 'ReactModal__Overlay');
462+
const content = TestUtils.findRenderedDOMComponentWithClass(modal.portal, 'ReactModal__Content');
463+
464+
expect(/ReactModal__Overlay--before-close/.test(overlay.className)).toBe(true);
465+
expect(/ReactModal__Content--before-close/.test(content.className)).toBe(true);
466+
467+
modal.portal.closeWithoutTimeout();
468+
unmountModal();
469+
});
470+
471+
it('keeps the modal in the DOM until closeTimeoutMS elapses', (done) => {
472+
const closeTimeoutMS = 50;
473+
474+
renderModal({
475+
isOpen: true,
476+
closeTimeoutMS
477+
});
478+
479+
unmountModal();
480+
481+
const checkDOM = (expectMounted) => {
482+
const overlay = document.querySelectorAll('.ReactModal__Overlay');
483+
const content = document.querySelectorAll('.ReactModal__Content');
484+
const numNodes = expectMounted ? 1 : 0;
485+
expect(overlay.length).toBe(numNodes);
486+
expect(content.length).toBe(numNodes);
487+
};
488+
489+
// content is still mounted after modal is gone
490+
checkDOM(true);
491+
492+
setTimeout(() => {
493+
// content is unmounted after specified timeout
494+
checkDOM(false);
495+
done();
496+
}, closeTimeoutMS);
497+
});
466498
});

0 commit comments

Comments
 (0)