Fix: Drawing threads now destroy their event handlers properly (#317)

box · Dec 18, 2018 · ba3795d · ba3795d
1 parent 7e3c01b
commit ba3795d
Show file tree

Hide file tree

Showing 6 changed files with 64 additions and 36 deletions.
diff --git a/src/AnnotationThread.js b/src/AnnotationThread.js
@@ -102,10 +102,9 @@ class AnnotationThread extends EventEmitter {
     destroy() {
         this.threadID = null;
         this.unmountPopover();
+        this.unbindDOMListeners();
 
         if (this.element) {
-            this.unbindDOMListeners();
-
             if (this.element.parentNode) {
                 this.element.parentNode.removeChild(this.element);
             }

diff --git a/src/controllers/DrawingModeController.js b/src/controllers/DrawingModeController.js
@@ -166,8 +166,6 @@ class DrawingModeController extends AnnotationModeController {
         this.pushElementHandler(this.redoButtonEl, 'click', this.redoDrawing);
 
         // Mobile & Desktop listeners are bound for touch-enabled laptop edge cases
-        // $FlowFixMe
-        this.drawingStartHandler = this.drawingStartHandler.bind(this);
         this.pushElementHandler(this.annotatedElement, ['mousedown', 'touchstart'], this.drawingStartHandler, true);
     }
 

diff --git a/src/doc/DocDrawingThread.js b/src/doc/DocDrawingThread.js
@@ -147,6 +147,15 @@ class DocDrawingThread extends DrawingThread {
         return !!(location && !!this.location && !!this.location.page && this.location.page !== location.page);
     }
 
+    /** @inheritdoc */
+    destroy() {
+        if (this.drawingContext && this.drawingContext.canvas && this.drawingContext.canvas.parentNode) {
+            this.drawingContext.canvas.parentNode.removeChild(this.drawingContext.canvas);
+        }
+
+        super.destroy();
+    }
+
     /**
      * Display the document drawing thread. Will set the drawing context if the scale has changed since the last show.
      *

diff --git a/src/doc/__tests__/DocDrawingThread-test.js b/src/doc/__tests__/DocDrawingThread-test.js
@@ -320,6 +320,7 @@ describe('doc/DocDrawingThread', () => {
 
         it('should return the pending drawing context when the state is pending', () => {
             thread.state = STATES.pending;
+            thread.destroy = jest.fn();
             thread.drawingContext = {
                 clearRect: jest.fn(),
                 canvas: {
@@ -381,4 +382,25 @@ describe('doc/DocDrawingThread', () => {
             expect(value).toStrictEqual([5, 5, 45, 40]);
         });
     });
+
+    describe('destroy()', () => {
+        it('should remove the drawing canvas from the DOM if present', () => {
+            const removeFn = jest.fn();
+
+            thread.drawingContext = {
+                clearRect: jest.fn(),
+                canvas: {
+                    height: 100,
+                    parentNode: {
+                        removeChild: removeFn
+                    },
+                    width: 100
+                }
+            };
+
+            thread.destroy();
+
+            expect(removeFn).toHaveBeenCalled();
+        });
+    });
 });
diff --git a/src/drawing/DrawingThread.js b/src/drawing/DrawingThread.js
@@ -15,6 +15,12 @@ class DrawingThread extends AnnotationThread {
     /** @property {number} - Drawing state */
     drawingFlag = DRAW_STATES.idle;
 
+    /** @property {Function} Handler for drawing start events */
+    userMoveHandler;
+
+    /** @property {Function} Handler for drawing stop events */
+    userStopHandler;
+
     /** @property {DrawingContainer} - The path container supporting undo and redo */
     pathContainer = new DrawingContainer();
 
@@ -96,7 +102,6 @@ class DrawingThread extends AnnotationThread {
             window.cancelAnimationFrame(this.lastAnimationRequestId);
         }
 
-        this.unmountPopover();
         this.reset();
         super.destroy();
 
@@ -124,43 +129,36 @@ class DrawingThread extends AnnotationThread {
      * @return {void}
      */
     bindDrawingListeners(locationFunction) {
+        this.userMoveHandler = eventToLocationHandler(locationFunction, this.handleMove);
+        this.userStopHandler = eventToLocationHandler(locationFunction, this.handleStop);
+
         if (this.hasTouch) {
-            this.annotatedElement.addEventListener(
-                'touchmove',
-                eventToLocationHandler(locationFunction, this.handleMove)
-            );
-            this.annotatedElement.addEventListener(
-                'touchcancel',
-                eventToLocationHandler(locationFunction, this.handleStop)
-            );
-            this.annotatedElement.addEventListener(
-                'touchend',
-                eventToLocationHandler(locationFunction, this.handleStop)
-            );
+            this.annotatedElement.addEventListener('touchmove', this.userMoveHandler);
+            this.annotatedElement.addEventListener('touchcancel', this.userStopHandler);
+            this.annotatedElement.addEventListener('touchend', this.userStopHandler);
         } else {
-            this.annotatedElement.addEventListener(
-                'mousemove',
-                eventToLocationHandler(locationFunction, this.handleMove)
-            );
-            this.annotatedElement.addEventListener(
-                'mouseup',
-                eventToLocationHandler(locationFunction, this.handleStop)
-            );
+            this.annotatedElement.addEventListener('mousemove', this.userMoveHandler);
+            this.annotatedElement.addEventListener('mouseup', this.userStopHandler);
         }
     }
 
+    /** @inheritdoc */
+    unbindDOMListeners() {
+        this.unbindDrawingListeners();
+        super.unbindDOMListeners();
+    }
+
     /**
      * Unbinds DOM event listeners for drawing new threads.
      *
      * @return {void}
      */
     unbindDrawingListeners() {
-        this.annotatedElement.removeEventListener('mousemove', eventToLocationHandler);
-        this.annotatedElement.removeEventListener('mouseup', eventToLocationHandler);
-
-        this.annotatedElement.removeEventListener('touchmove', eventToLocationHandler);
-        this.annotatedElement.removeEventListener('touchcancel', eventToLocationHandler);
-        this.annotatedElement.removeEventListener('touchend', eventToLocationHandler);
+        this.annotatedElement.removeEventListener('mousemove', this.userMoveHandler);
+        this.annotatedElement.removeEventListener('mouseup', this.userStopHandler);
+        this.annotatedElement.removeEventListener('touchmove', this.userMoveHandler);
+        this.annotatedElement.removeEventListener('touchcancel', this.userStopHandler);
+        this.annotatedElement.removeEventListener('touchend', this.userStopHandler);
     }
 
     /**

diff --git a/src/drawing/__tests__/DrawingThread-test.js b/src/drawing/__tests__/DrawingThread-test.js
@@ -119,15 +119,17 @@ describe('drawing/DrawingThread', () => {
             thread.annotatedElement = {
                 removeEventListener: jest.fn()
             };
+            thread.userMoveHandler = jest.fn();
+            thread.userStopHandler = jest.fn();
         });
 
         it('should add only mouse listeners for desktop devices', () => {
             thread.unbindDrawingListeners();
-            expect(thread.annotatedElement.removeEventListener).toBeCalledWith('mousemove', expect.any(Function));
-            expect(thread.annotatedElement.removeEventListener).toBeCalledWith('mouseup', expect.any(Function));
-            expect(thread.annotatedElement.removeEventListener).toBeCalledWith('touchmove', expect.any(Function));
-            expect(thread.annotatedElement.removeEventListener).toBeCalledWith('touchcancel', expect.any(Function));
-            expect(thread.annotatedElement.removeEventListener).toBeCalledWith('touchend', expect.any(Function));
+            expect(thread.annotatedElement.removeEventListener).toBeCalledWith('mousemove', thread.userMoveHandler);
+            expect(thread.annotatedElement.removeEventListener).toBeCalledWith('mouseup', thread.userStopHandler);
+            expect(thread.annotatedElement.removeEventListener).toBeCalledWith('touchmove', thread.userMoveHandler);
+            expect(thread.annotatedElement.removeEventListener).toBeCalledWith('touchcancel', thread.userStopHandler);
+            expect(thread.annotatedElement.removeEventListener).toBeCalledWith('touchend', thread.userStopHandler);
         });
     });