Chore: Cleaning up and consolidating annotation methods (#288)

- Combines point and draw annotation specific handler methods in Annotator.js - Cleans up how annotation mode handlers are dealt with in BaseViewer.js - Fixes point annotation button so that it turns blue when in annotation mode - Moving annotationmodeenter/exit strings to constants - Force this.highlightMouseMoveHandler to be reset on re-bind
box · Aug 11, 2017 · 6542b5d · 6542b5d
1 parent 21f490f
commit 6542b5d
Show file tree

Hide file tree

Showing 9 changed files with 518 additions and 568 deletions.
diff --git a/src/lib/annotations/Annotator.js b/src/lib/annotations/Annotator.js
diff --git a/src/lib/annotations/Annotator.scss b/src/lib/annotations/Annotator.scss
@@ -95,7 +95,7 @@ $avatar-color-9: #f22c44;
     }
 }
 
-.bp-btn-plain.bp-btn-annotate.bp-is-active svg {
+.bp-btn-plain.bp-btn-annotate-point.bp-is-active svg {
     fill: $box-blue;
 }
 
@@ -425,22 +425,18 @@ $avatar-color-9: #f22c44;
 //------------------------------------------------------------------------------
 // Annotation mode
 //------------------------------------------------------------------------------
-.bp-draw-annotation-mode .page,
-.bp-draw-annotation-mode .bp-annotation-layer-highlight,
-.bp-draw-annotation-mode .textLayer > div,
-.bp-draw-annotation-mode > img,
-.bp-point-annotation-mode .page,
-.bp-point-annotation-mode .bp-annotation-layer-highlight,
-.bp-point-annotation-mode .textLayer > div,
-.bp-point-annotation-mode > img {
+.bp-annotation-mode .page,
+.bp-annotation-mode .bp-annotation-layer-highlight,
+.bp-annotation-mode .textLayer > div,
+.bp-annotation-mode > img {
     cursor: crosshair;
 }
 
 // Needed to allow point annotations since PDF.js adds a funky mousedown handler
 // that helps with text selection - we need to disable this since it interacts
 // badly with point annotations on non-text divs in non-Chrome browsers
-.bp-point-annotation-mode .textLayer > div,
-.bp-point-annotation-mode .textLayer .endOfContent {
+.bp-annotation-mode .textLayer > div,
+.bp-annotation-mode .textLayer .endOfContent {
     pointer-events: none;
 }
 

diff --git a/src/lib/annotations/__tests__/Annotator-test.js b/src/lib/annotations/__tests__/Annotator-test.js
diff --git a/src/lib/annotations/annotationConstants.js b/src/lib/annotations/annotationConstants.js
@@ -6,8 +6,7 @@ export const CLASS_ANNOTATION_BUTTON_POST = 'post-annotation-btn';
 export const CLASS_ANNOTATION_DIALOG = 'bp-annotation-dialog';
 export const CLASS_ANNOTATION_HIGHLIGHT_DIALOG = 'bp-annotation-highlight-dialog';
 export const CLASS_ANNOTATION_POINT_MARKER = 'bp-point-annotation-marker';
-export const CLASS_ANNOTATION_POINT_MODE = 'bp-point-annotation-mode';
-export const CLASS_ANNOTATION_DRAW_MODE = 'bp-draw-annotation-mode';
+export const CLASS_ANNOTATION_MODE = 'bp-annotation-mode';
 export const CLASS_ANNOTATION_CARET = 'bp-annotation-caret';
 export const CLASS_ANNOTATION_TEXTAREA = 'annotation-textarea';
 export const CLASS_BUTTON_CONTAINER = 'button-container';
@@ -51,7 +50,7 @@ export const SELECTOR_ANNOTATION_BUTTON_POST = `.${CLASS_ANNOTATION_BUTTON_POST}
 export const SELECTOR_ANNOTATION_DIALOG = `.${CLASS_ANNOTATION_DIALOG}`;
 export const SELECTOR_ANNOTATION_HIGHLIGHT_DIALOG = `.${CLASS_ANNOTATION_HIGHLIGHT_DIALOG}`;
 export const SELECTOR_ANNOTATION_POINT_BUTTON = `.${CLASS_ANNOTATION_POINT_MARKER}`;
-export const SELECTOR_ANNOTATION_POINT_MODE = `.${CLASS_ANNOTATION_POINT_MODE}`;
+export const SELECTOR_ANNOTATION_POINT_MODE = `.${CLASS_ANNOTATION_MODE}`;
 export const SELECTOR_ANNOTATION_CARET = `.${CLASS_ANNOTATION_CARET}`;
 export const SELECTOR_ANNOTATION_TEXTAREA = `.${CLASS_ANNOTATION_TEXTAREA}`;
 export const SELECTOR_BUTTON_CONTAINER = `.${CLASS_BUTTON_CONTAINER}`;

diff --git a/src/lib/annotations/doc/DocAnnotator.js b/src/lib/annotations/doc/DocAnnotator.js
@@ -425,7 +425,7 @@ class DocAnnotator extends Annotator {
 
         this.annotatedElement.addEventListener('mouseup', this.highlightMouseupHandler);
 
-        if (this.annotationService.canAnnotate) {
+        if (this.canAnnotate) {
             this.annotatedElement.addEventListener('dblclick', this.highlightMouseupHandler);
             this.annotatedElement.addEventListener('mousedown', this.highlightMousedownHandler);
             this.annotatedElement.addEventListener('contextmenu', this.highlightMousedownHandler);
@@ -445,11 +445,12 @@ class DocAnnotator extends Annotator {
 
         this.annotatedElement.removeEventListener('mouseup', this.highlightMouseupHandler);
 
-        if (this.annotationService.canAnnotate) {
+        if (this.canAnnotate) {
             this.annotatedElement.removeEventListener('dblclick', this.highlightMouseupHandler);
             this.annotatedElement.removeEventListener('mousedown', this.highlightMousedownHandler);
             this.annotatedElement.removeEventListener('contextmenu', this.highlightMousedownHandler);
-            this.annotatedElement.removeEventListener('mousemove', this.getHighlightMouseMoveHandler());
+            this.annotatedElement.removeEventListener('mousemove', this.highlightMousemoveHandler);
+            this.highlightMousemoveHandler = null;
 
             if (this.highlightThrottleHandle) {
                 cancelAnimationFrame(this.highlightThrottleHandle);

diff --git a/src/lib/annotations/doc/__tests__/DocAnnotator-test.js b/src/lib/annotations/doc/__tests__/DocAnnotator-test.js
@@ -35,7 +35,11 @@ describe('lib/annotations/doc/DocAnnotator', () => {
             annotationService: {},
             fileVersionId: 1,
             isMobile: false,
-            options: {}
+            options: {},
+            previewUI: {
+                getAnnotateButton: () => {}
+            },
+            modeButtons: {}
         });
         annotator.annotatedElement = annotator.getAnnotatedEl(document);
         annotator.annotationService = {};
@@ -493,7 +497,7 @@ describe('lib/annotations/doc/DocAnnotator', () => {
         });
 
         it('shouldn\'t bind DOM listeners if user cannot annotate except mouseup', () => {
-            annotator.annotationService.canAnnotate = false;
+            annotator.canAnnotate = false;
 
             stubs.elMock.expects('addEventListener').withArgs('mouseup', sinon.match.func);
             stubs.elMock.expects('addEventListener').withArgs('dblclick', sinon.match.func).never();
@@ -504,7 +508,7 @@ describe('lib/annotations/doc/DocAnnotator', () => {
         });
 
         it('should bind DOM listeners if user can annotate', () => {
-            annotator.annotationService.canAnnotate = true;
+            annotator.canAnnotate = true;
 
             stubs.elMock.expects('addEventListener').withArgs('mouseup', sinon.match.func);
             stubs.elMock.expects('addEventListener').withArgs('dblclick', sinon.match.func);
@@ -521,10 +525,11 @@ describe('lib/annotations/doc/DocAnnotator', () => {
                 removeEventListener: () => {}
             };
             stubs.elMock = sandbox.mock(annotator.annotatedElement);
+            annotator.highlightMousemoveHandler = () => {};
         });
 
         it('should not unbind DOM listeners if user cannot annotate except mouseup', () => {
-            annotator.annotationService.canAnnotate = false;
+            annotator.canAnnotate = false;
 
             stubs.elMock.expects('removeEventListener').withArgs('mouseup', sinon.match.func);
             stubs.elMock.expects('removeEventListener').withArgs('mousedown', sinon.match.func).never();
@@ -535,7 +540,7 @@ describe('lib/annotations/doc/DocAnnotator', () => {
         });
 
         it('should unbind DOM listeners if user can annotate', () => {
-            annotator.annotationService.canAnnotate = true;
+            annotator.canAnnotate = true;
 
             stubs.elMock.expects('removeEventListener').withArgs('mouseup', sinon.match.func);
             stubs.elMock.expects('removeEventListener').withArgs('mousedown', sinon.match.func);
@@ -547,7 +552,7 @@ describe('lib/annotations/doc/DocAnnotator', () => {
 
         it('should stop and destroy the requestAnimationFrame handle created by getHighlightMousemoveHandler()', () => {
             const rafHandle = 12; // RAF handles are integers
-            annotator.annotationService.canAnnotate = true;
+            annotator.canAnnotate = true;
             annotator.highlightThrottleHandle = rafHandle;
             sandbox.stub(annotator, 'getHighlightMouseMoveHandler').returns(sandbox.stub());
 

diff --git a/src/lib/annotations/image/__tests__/ImageAnnotator-test.js b/src/lib/annotations/image/__tests__/ImageAnnotator-test.js
@@ -25,7 +25,8 @@ describe('lib/annotations/image/ImageAnnotator', () => {
             options: {},
             previewUI: {
                 getAnnotateButton: () => {}
-            }
+            },
+            modeButtons: {}
         });
         annotator.annotatedElement = annotator.getAnnotatedEl(document);
         annotator.annotationService = {};

diff --git a/src/lib/viewers/BaseViewer.js b/src/lib/viewers/BaseViewer.js
@@ -36,6 +36,8 @@ const ANNOTATION_TYPE_DRAW = 'draw';
 const ANNOTATION_TYPE_POINT = 'point';
 const LOAD_TIMEOUT_MS = 180000; // 3m
 const RESIZE_WAIT_TIME_IN_MILLIS = 300;
+const ANNOTATION_MODE_ENTER = 'annotationmodeenter';
+const ANNOTATION_MODE_EXIT = 'annotationmodeexit';
 const ANNOTATION_BUTTONS = {
     point: {
         title: __('annotation_point_toggle'),
@@ -167,21 +169,6 @@ class BaseViewer extends EventEmitter {
             });
         }
 
-        const { container } = this.options;
-        if (container) {
-            const pointAnnotateButtonEl = container.querySelector(SELECTOR_BOX_PREVIEW_BTN_ANNOTATE_POINT);
-            const drawAnnotateButtonEl = container.querySelector(SELECTOR_BOX_PREVIEW_BTN_ANNOTATE_DRAW);
-            if (pointAnnotateButtonEl) {
-                const handler = this.getAnnotationModeClickHandler('point');
-                pointAnnotateButtonEl.removeEventListener('click', handler);
-            }
-
-            if (drawAnnotateButtonEl) {
-                const handler = this.getAnnotationModeClickHandler('draw');
-                drawAnnotateButtonEl.removeEventListener('click', handler);
-            }
-        }
-
         fullscreen.removeAllListeners();
         document.defaultView.removeEventListener('resize', this.debouncedResizeHandler);
         this.removeAllListeners();
@@ -651,19 +638,13 @@ class BaseViewer extends EventEmitter {
             return;
         }
 
-        if (this.isAnnotatable()) {
+        if (this.areAnnotationsEnabled()) {
             const { file } = this.options;
-
-            // Users can currently only view annotations on mobile
             this.canAnnotate = checkPermission(file, PERMISSION_ANNOTATE);
+
             if (this.canAnnotate) {
-                // Show the annotate button for all enabled types for the
-                // current viewer
-                this.annotatorConf.TYPE.forEach((type) => {
-                    this.showModeAnnotateButton(type, ANNOTATION_BUTTONS);
-                });
+                this.initAnnotations();
             }
-            this.initAnnotations();
         }
     }
 
@@ -695,15 +676,6 @@ class BaseViewer extends EventEmitter {
         });
         this.annotator.init(this.scale);
 
-        // Disables controls during annotation mode
-        this.addListener('togglepointannotationmode', () => {
-            this.annotator.togglePointAnnotationHandler();
-        });
-
-        this.addListener('toggledrawannotationmode', () => {
-            this.annotator.toggleDrawAnnotationHandler();
-        });
-
         // Add a custom listener for events related to scaling/orientation changes
         this.addListener('scale', (data) => {
             this.annotator.emit('scaleAnnotations', data);
@@ -713,30 +685,6 @@ class BaseViewer extends EventEmitter {
         this.annotator.addListener('annotatorevent', this.handleAnnotatorNotifications);
     }
 
-    /**
-     * Returns whether or not viewer both supports annotations and has
-     * annotations enabled. If an optional type is passed in, we check if that
-     * type of annotation is allowed.
-     *
-     * @param {string} [type] - Type of annotation
-     * @return {boolean} Whether or not viewer is annotatable
-     */
-    isAnnotatable(type) {
-        if (!this.annotatorConf) {
-            return false;
-        }
-
-        const { TYPE: annotationTypes } = this.annotatorConf;
-        if (type && annotationTypes) {
-            if (!annotationTypes.some((annotationType) => type === annotationType)) {
-                return false;
-            }
-        }
-
-        // Return whether or not annotations are enabled for this viewer
-        return this.areAnnotationsEnabled();
-    }
-
     /**
      * Returns whether or not annotations are enabled for this viewer.
      *
@@ -753,47 +701,6 @@ class BaseViewer extends EventEmitter {
         return this.options.showAnnotations;
     }
 
-    /**
-     * Shows the annotate button for the specified mode
-     *
-     * @param {string} currentMode - Annotation mode
-     * @param {Object[]} modeButtons - Annotation modes which require buttons
-     * @return {void}
-     */
-    showModeAnnotateButton(currentMode, modeButtons) {
-        const mode = modeButtons[currentMode];
-        if (!mode || !this.isAnnotatable(currentMode)) {
-            return;
-        }
-
-        const { container } = this.options;
-        const annotateButtonEl = container.querySelector(mode.selector);
-        if (annotateButtonEl) {
-            annotateButtonEl.title = mode.title;
-            annotateButtonEl.classList.remove(CLASS_HIDDEN);
-
-            const handler = this.getAnnotationModeClickHandler(currentMode);
-            annotateButtonEl.addEventListener('click', handler);
-        }
-    }
-
-    /**
-     * Returns click handler for toggling annotation mode.
-     *
-     * @param {string} mode - Target annotation mode
-     * @return {Function|null} Click handler
-     */
-    getAnnotationModeClickHandler(mode) {
-        if (!mode || !this.isAnnotatable(mode)) {
-            return null;
-        }
-
-        const eventName = `toggle${mode}annotationmode`;
-        return () => {
-            this.emit(eventName);
-        };
-    }
-
     /**
      * Handle events emitted by the annotator
      *
@@ -806,7 +713,7 @@ class BaseViewer extends EventEmitter {
     handleAnnotatorNotifications(data) {
         /* istanbul ignore next */
         switch (data.event) {
-            case 'annotationmodeenter':
+            case ANNOTATION_MODE_ENTER:
                 this.disableViewerControls();
 
                 if (data.data === ANNOTATION_TYPE_POINT) {
@@ -815,7 +722,7 @@ class BaseViewer extends EventEmitter {
                     this.emit('notificationshow', __('notification_annotation_draw_mode'));
                 }
                 break;
-            case 'annotationmodeexit':
+            case ANNOTATION_MODE_EXIT:
                 this.enableViewerControls();
                 this.emit('notificationhide');
                 break;