Skip to content

Commit

Permalink
[fixed] respect closeTimeoutMS during unmount
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@mgh
Martin Hunt authored and mgh committed Feb 18, 2017
1 parent ebec638 commit ac01dda
Show file tree
Hide file tree
Showing 3 changed files with 88 additions and 26 deletions.
18 changes: 18 additions & 0 deletions lib/components/Modal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
10 changes: 6 additions & 4 deletions lib/components/ModalPortal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
});
}

Expand All @@ -142,7 +143,8 @@ export default class ModalPortal extends Component {
beforeClose: false,
isOpen: false,
afterOpen: false,
}, () => this.afterClose());
closesAt: null
}, this.afterClose);
}

handleKeyDown = (event) => {
Expand Down Expand Up @@ -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 () {
Expand Down
86 changes: 64 additions & 22 deletions specs/Modal.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Expand Down Expand Up @@ -115,6 +122,7 @@ describe('Modal', () => {
const modal = renderModal({
isOpen: true,
onRequestClose () {
unmountModal();
unmountModal();
done();
}
Expand Down Expand Up @@ -175,6 +183,7 @@ describe('Modal', () => {
preventDefault () { tabPrevented = true; }
});
expect(tabPrevented).toEqual(true);
unmountModal();
});

it('supports portalClassName', () => {
Expand Down Expand Up @@ -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', () => {
Expand All @@ -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);
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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);
});
});

0 comments on commit ac01dda

Please sign in to comment.