From e92b1c6165715c1869a7dbbaa37254b35930119d Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Wed, 15 Aug 2018 11:52:16 +0200 Subject: [PATCH] [Slider] fix memory leaks --- packages/material-ui-lab/src/Slider/Slider.js | 32 +++++++---------- .../material-ui-lab/src/Slider/Slider.test.js | 34 +++++++++++++++++-- 2 files changed, 43 insertions(+), 23 deletions(-) diff --git a/packages/material-ui-lab/src/Slider/Slider.js b/packages/material-ui-lab/src/Slider/Slider.js index 0f7a35edf0722c..6371a2f38bdc3b 100644 --- a/packages/material-ui-lab/src/Slider/Slider.js +++ b/packages/material-ui-lab/src/Slider/Slider.js @@ -134,15 +134,6 @@ export const styles = theme => { }; }; -function addEventListener(node, event, handler, capture) { - node.addEventListener(event, handler, capture); - return { - remove: function remove() { - node.removeEventListener(event, handler, capture); - }, - }; -} - function percentToValue(percent, min, max) { return ((max - min) * percent) / 100 + min; } @@ -198,6 +189,8 @@ if (process.env.NODE_ENV !== 'production' && !React.createContext) { class Slider extends React.Component { state = { currentState: 'initial' }; + jumpAnimationTimeoutId = -1; + componentDidMount() { if (this.containerRef) { this.containerRef.addEventListener('touchstart', preventPageScrolling, { passive: false }); @@ -206,6 +199,9 @@ class Slider extends React.Component { componentWillUnmount() { this.containerRef.removeEventListener('touchstart', preventPageScrolling, { passive: false }); + document.body.removeEventListener('mousemove', this.handleMouseMove); + document.body.removeEventListener('mouseup', this.handleMouseUp); + clearTimeout(this.jumpAnimationTimeoutId); } static getDerivedStateFromProps(nextProps, prevState) { @@ -281,7 +277,7 @@ class Slider extends React.Component { event.preventDefault(); this.setState({ currentState: 'activated' }); - this.globalMouseUpListener = addEventListener(document, 'touchend', this.handleMouseUp); + document.body.addEventListener('touchend', this.handleMouseUp); if (typeof this.props.onDragStart === 'function') { this.props.onDragStart(event); @@ -292,8 +288,8 @@ class Slider extends React.Component { event.preventDefault(); this.setState({ currentState: 'activated' }); - this.globalMouseUpListener = addEventListener(document, 'mouseup', this.handleMouseUp); - this.globalMouseMoveListener = addEventListener(document, 'mousemove', this.handleMouseMove); + document.body.addEventListener('mousemove', this.handleMouseMove); + document.body.addEventListener('mouseup', this.handleMouseUp); if (typeof this.props.onDragStart === 'function') { this.props.onDragStart(event); @@ -303,13 +299,8 @@ class Slider extends React.Component { handleMouseUp = event => { this.setState({ currentState: 'normal' }); - if (this.globalMouseUpListener) { - this.globalMouseUpListener.remove(); - } - - if (this.globalMouseMoveListener) { - this.globalMouseMoveListener.remove(); - } + document.body.removeEventListener('mousemove', this.handleMouseMove); + document.body.removeEventListener('mouseup', this.handleMouseUp); if (typeof this.props.onDragEnd === 'function') { this.props.onDragEnd(event); @@ -373,7 +364,8 @@ class Slider extends React.Component { playJumpAnimation() { this.setState({ currentState: 'jumped' }, () => { - setTimeout(() => { + clearTimeout(this.jumpAnimationTimeoutId); + this.jumpAnimationTimeoutId = setTimeout(() => { this.setState({ currentState: 'normal' }); }, this.props.theme.transitions.duration.complex); }); diff --git a/packages/material-ui-lab/src/Slider/Slider.test.js b/packages/material-ui-lab/src/Slider/Slider.test.js index c016703f4dc2e2..78f6e1b5d3ca1c 100644 --- a/packages/material-ui-lab/src/Slider/Slider.test.js +++ b/packages/material-ui-lab/src/Slider/Slider.test.js @@ -48,15 +48,43 @@ describe('', () => { wrapper.simulate('click'); wrapper.simulate('mousedown'); // document.simulate('mouseup') - const mouseupEvent = document.createEvent('HTMLEvents'); - mouseupEvent.initEvent('mouseup', false, true); - document.dispatchEvent(mouseupEvent); + document.body.dispatchEvent(new window.MouseEvent('mouseup')); assert.strictEqual(handleChange.callCount, 1, 'should have called the handleChange cb'); assert.strictEqual(handleDragStart.callCount, 1, 'should have called the handleDragStart cb'); assert.strictEqual(handleDragEnd.callCount, 1, 'should have called the handleDragEnd cb'); }); + describe('unmount', () => { + it('should not have global event listeners registered after unmount', () => { + const handleChange = spy(); + const handleDragEnd = spy(); + + const wrapper = mount(); + + const callGlobalListeners = () => { + document.body.dispatchEvent(new window.MouseEvent('mousemove')); + document.body.dispatchEvent(new window.MouseEvent('mouseup')); + }; + + wrapper.simulate('mousedown'); + callGlobalListeners(); + // pre condition: the dispatched event actually did something when mounted + assert.strictEqual(handleChange.callCount, 1, 'should have called the handleChange cb'); + assert.strictEqual(handleDragEnd.callCount, 1, 'should have called the handleDragEnd cb'); + + wrapper.unmount(); + + // After unmounting global listeners should not be registered aynmore since that would + // break component encapsulation. If they are still mounted either react will throw warnings + // or other component logic throws. + // post condition: the dispatched events dont cause errors/warnings + callGlobalListeners(); + assert.strictEqual(handleChange.callCount, 1, 'should not have called handleChange again'); + assert.strictEqual(handleDragEnd.callCount, 1, 'should not have called handleDragEnd again'); + }); + }); + describe('prop: vertical', () => { it('should render with the default and vertical classes', () => { const wrapper = shallow();