Fix: Moved guards so drawing works when highlights disabled (#409)

box · Sep 26, 2017 · f516a4f · f516a4f
1 parent ef60e95
commit f516a4f
Show file tree

Hide file tree

Showing 2 changed files with 49 additions and 7 deletions.
diff --git a/src/lib/annotations/doc/DocAnnotator.js b/src/lib/annotations/doc/DocAnnotator.js
@@ -530,19 +530,27 @@ class DocAnnotator extends Annotator {
         this.annotatedElement.addEventListener('mouseup', this.highlightMouseupHandler);
 
         // Prevent all forms of highlight annotations if annotating (or plain AND comment highlights) is disabled
-        if (!this.permissions.canAnnotate || (!this.plainHighlightEnabled && !this.commentHighlightEnabled)) {
+        if (!this.permissions.canAnnotate) {
             return;
         }
 
         if (this.hasTouch && this.isMobile) {
-            document.addEventListener('selectionchange', this.onSelectionChange);
             this.annotatedElement.addEventListener('touchstart', this.drawingSelectionHandler);
+
+            // Highlight listeners
+            if (this.plainHighlightEnabled || this.commentHighlightEnabled) {
+                document.addEventListener('selectionchange', this.onSelectionChange);
+            }
         } else {
             this.annotatedElement.addEventListener('click', this.drawingSelectionHandler);
-            this.annotatedElement.addEventListener('dblclick', this.highlightMouseupHandler);
-            this.annotatedElement.addEventListener('mousedown', this.highlightMousedownHandler);
-            this.annotatedElement.addEventListener('contextmenu', this.highlightMousedownHandler);
-            this.annotatedElement.addEventListener('mousemove', this.getHighlightMouseMoveHandler());
+
+            // Highlight listeners
+            if (this.plainHighlightEnabled || this.commentHighlightEnabled) {
+                this.annotatedElement.addEventListener('dblclick', this.highlightMouseupHandler);
+                this.annotatedElement.addEventListener('mousedown', this.highlightMousedownHandler);
+                this.annotatedElement.addEventListener('contextmenu', this.highlightMousedownHandler);
+                this.annotatedElement.addEventListener('mousemove', this.getHighlightMouseMoveHandler());
+            }
         }
     }
 

diff --git a/src/lib/annotations/doc/__tests__/DocAnnotator-test.js b/src/lib/annotations/doc/__tests__/DocAnnotator-test.js
@@ -660,7 +660,7 @@ describe('lib/annotations/doc/DocAnnotator', () => {
             annotator.bindDOMListeners();
         });
 
-        it('should bind selectionchange event, on the document, if on mobile and can annotate', () => {
+        it('should bind selectionchange and touchstart event, on the document, if on mobile and can annotate', () => {
             annotator.permissions.canAnnotate = true;
             annotator.isMobile = true;
             annotator.hasTouch = true;
@@ -673,6 +673,40 @@ describe('lib/annotations/doc/DocAnnotator', () => {
             expect(docListen).to.be.calledWith('selectionchange', sinon.match.func);
             expect(annotatedElementListen).to.be.calledWith('touchstart', sinon.match.func);
         });
+
+        it('should not bind selection change event if both annotation types are disabled, and touch enabled, mobile enabled', () => {
+            annotator.permissions.canAnnotate = true;
+            annotator.isMobile = true;
+            annotator.hasTouch = true;
+            annotator.plainHighlightEnabled = false;
+            annotator.commentHighlightEnabled = false;
+
+            const docListen = sandbox.spy(document, 'addEventListener');
+            const annotatedElementListen = sandbox.spy(annotator.annotatedElement, 'addEventListener');
+
+            annotator.bindDOMListeners();
+
+            expect(docListen).to.not.be.calledWith('selectionchange', sinon.match.func);
+            expect(annotatedElementListen).to.be.calledWith('touchstart', sinon.match.func);
+        });
+
+        it('should not bind selection change event if both annotation types are disabled, and touch disabled, mobile disabled', () => {
+            annotator.permissions.canAnnotate = true;
+            annotator.isMobile = false;
+            annotator.hasTouch = false;
+            annotator.plainHighlightEnabled = false;
+            annotator.commentHighlightEnabled = false;
+
+            stubs.elMock.expects('addEventListener').withArgs('mouseup', sinon.match.func);
+            stubs.elMock.expects('addEventListener').withArgs('click', sinon.match.func);
+            stubs.elMock.expects('addEventListener').never().withArgs('dblclick', sinon.match.func);
+            stubs.elMock.expects('addEventListener').never().withArgs('mousedown', sinon.match.func);
+            stubs.elMock.expects('addEventListener').never().withArgs('contextmenu', sinon.match.func);
+            stubs.elMock.expects('addEventListener').never().withArgs('mousemove', sinon.match.func);
+
+            annotator.bindDOMListeners();
+        });
+
     });
 
     describe('unbindDOMListeners()', () => {