Fix: Prevent unnecessary draw code from executing (#421)

box · Oct 4, 2017 · d6cdcf9 · d6cdcf9
1 parent 26813a4
commit d6cdcf9
Show file tree

Hide file tree

Showing 4 changed files with 31 additions and 12 deletions.
diff --git a/src/lib/annotations/BoxAnnotations.js b/src/lib/annotations/BoxAnnotations.js
@@ -86,7 +86,7 @@ class BoxAnnotations {
     }
 
     /**
-     * Chooses a annotator based on viewer.
+     * Chooses an annotator based on viewer.
      *
      * @param {Object} viewerName - Current preview viewer name
      * @param {Object} permissions - File permissions
@@ -97,6 +97,11 @@ class BoxAnnotations {
     determineAnnotator(viewerName, permissions, viewerConfig = {}, disabledAnnotators = []) {
         let modifiedAnnotator = null;
 
+        // Remove draw annotations if they aren't explicitly enabled.
+        if (!viewerConfig.drawEnabled) {
+            this.annotators[0].TYPE = this.annotators[0].TYPE.filter((type) => type !== TYPES.draw);
+        }
+
         const hasAnnotationPermissions = canLoadAnnotations(permissions);
         const annotator = this.getAnnotatorsForViewer(viewerName, disabledAnnotators);
         if (!hasAnnotationPermissions || !annotator || viewerConfig.enabled === false) {
@@ -112,11 +117,6 @@ class BoxAnnotations {
             });
         }
 
-        // Remove draw annotations if they aren't explicitly enabled.
-        if (!viewerConfig.drawEnabled) {
-            modifiedAnnotator.TYPE = modifiedAnnotator.TYPE.filter((type) => type !== TYPES.draw);
-        }
-
         return modifiedAnnotator;
     }
 }

diff --git a/src/lib/annotations/__tests__/BoxAnnotations-test.js b/src/lib/annotations/__tests__/BoxAnnotations-test.js
@@ -136,20 +136,23 @@ describe('lib/annotators/BoxAnnotations', () => {
         it('should remove draw annotations unless explicitly enabled', () => {
             let config = {
                 enabled: true,
+                drawEnabled: true,
                 disabledTypes: ['point']
             };
             const docAnnotator = {
                 NAME: 'Document',
                 VIEWER: ['Document'],
                 TYPE: ['point', 'highlight', 'draw']
             };
+
             loader.annotators = [docAnnotator];
-            let annotator = loader.determineAnnotator('Document', {}, config);
-            expect(annotator.TYPE.includes('draw')).to.be.false;
 
-            config.drawEnabled = true;
             annotator = loader.determineAnnotator('Document', {}, config);
             expect(annotator.TYPE.includes('draw')).to.be.true;
+
+            config.drawEnabled = undefined;
+            let annotator = loader.determineAnnotator('Document', {}, config);
+            expect(annotator.TYPE.includes('draw')).to.be.false;
         });
     });
 

diff --git a/src/lib/annotations/doc/DocAnnotator.js b/src/lib/annotations/doc/DocAnnotator.js
@@ -91,6 +91,9 @@ class DocAnnotator extends Annotator {
     /** @property {boolean} - True if regular highlights are allowed to be read/written */
     plainHighlightEnabled;
 
+    /** @property {boolean} - True if draw annotations are allowed to be read/written */
+    drawEnabled;
+
     /** @property {boolean} - True if comment highlights are allowed to be read/written */
     commentHighlightEnabled;
 
@@ -111,6 +114,12 @@ class DocAnnotator extends Annotator {
 
         this.plainHighlightEnabled = this.isModeAnnotatable(TYPES.highlight);
         this.commentHighlightEnabled = this.isModeAnnotatable(TYPES.highlight_comment);
+        this.drawEnabled = this.isModeAnnotatable(TYPES.draw);
+
+        // Don't bind to draw specific handlers if we cannot draw
+        if (this.drawEnabled) {
+            this.drawingSelectionHandler = this.drawingSelectionHandler.bind(this);
+        }
 
         // Don't bind to highlight specific handlers if we cannot highlight
         if (!this.plainHighlightEnabled && !this.commentHighlightEnabled) {
@@ -119,7 +128,6 @@ class DocAnnotator extends Annotator {
 
         // Explicit scoping
         this.highlightCreateHandler = this.highlightCreateHandler.bind(this);
-        this.drawingSelectionHandler = this.drawingSelectionHandler.bind(this);
         this.showFirstDialogFilter = showFirstDialogFilter.bind(this);
 
         this.createHighlightDialog = new CreateHighlightDialog(this.container, {
@@ -535,14 +543,18 @@ class DocAnnotator extends Annotator {
         }
 
         if (this.hasTouch && this.isMobile) {
-            this.annotatedElement.addEventListener('touchstart', this.drawingSelectionHandler);
+            if (this.drawEnabled) {
+                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);
+            if (this.drawEnabled) {
+                this.annotatedElement.addEventListener('click', this.drawingSelectionHandler);
+            }
 
             // Highlight listeners
             if (this.plainHighlightEnabled || this.commentHighlightEnabled) {

diff --git a/src/lib/annotations/doc/__tests__/DocAnnotator-test.js b/src/lib/annotations/doc/__tests__/DocAnnotator-test.js
@@ -650,6 +650,7 @@ describe('lib/annotations/doc/DocAnnotator', () => {
 
         it('should bind DOM listeners if user can annotate', () => {
             annotator.permissions.canAnnotate = true;
+            annotator.drawEnabled = true;
 
             stubs.elMock.expects('addEventListener').withArgs('mouseup', sinon.match.func);
             stubs.elMock.expects('addEventListener').withArgs('dblclick', sinon.match.func);
@@ -664,6 +665,7 @@ describe('lib/annotations/doc/DocAnnotator', () => {
             annotator.permissions.canAnnotate = true;
             annotator.isMobile = true;
             annotator.hasTouch = true;
+            annotator.drawEnabled = true;
 
             const docListen = sandbox.spy(document, 'addEventListener');
             const annotatedElementListen = sandbox.spy(annotator.annotatedElement, 'addEventListener');
@@ -680,6 +682,7 @@ describe('lib/annotations/doc/DocAnnotator', () => {
             annotator.hasTouch = true;
             annotator.plainHighlightEnabled = false;
             annotator.commentHighlightEnabled = false;
+            annotator.drawEnabled = true;
 
             const docListen = sandbox.spy(document, 'addEventListener');
             const annotatedElementListen = sandbox.spy(annotator.annotatedElement, 'addEventListener');
@@ -696,6 +699,7 @@ describe('lib/annotations/doc/DocAnnotator', () => {
             annotator.hasTouch = false;
             annotator.plainHighlightEnabled = false;
             annotator.commentHighlightEnabled = false;
+            annotator.drawEnabled = true;
 
             stubs.elMock.expects('addEventListener').withArgs('mouseup', sinon.match.func);
             stubs.elMock.expects('addEventListener').withArgs('click', sinon.match.func);