[Slider] fix memory leaks

mui · Aug 16, 2018 · ad9af5b · ad9af5b
1 parent 96d2c38
commit ad9af5b
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 23 deletions.
diff --git 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
@@ -48,15 +48,43 @@ describe('<Slider />', () => {
     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(<Slider onChange={handleChange} onDragEnd={handleDragEnd} value={0} />);
+
+      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(<Slider vertical value={0} />);