Fix: Ensure onSelectionChange isn't triggered while creating highligh…

…ts (#281)

- Fix Rangy highlight CSS
- Ensure onSelectionChange isn't triggered while creating highlights

src/Annotator.scss

@@ -168,8 +168,7 @@
 
 // Quad point positioning - the helper divs are positioned relative to the Rangy-created element
 .bp-doc .rangy-highlight {
-    background-color: lighten($highlight-yellow, 10%);
-    opacity: .25;
+    background-color: $highlight-yellow;
     position: relative;
 }
 

src/components/ActionControls/ActionControls.scss

@@ -35,6 +35,9 @@
         }
 
         .ba-annotation-input-container {
+            bottom: 0;
+            padding: 15px;
+            position: absolute;
             width: 100%;
         }
 

src/doc/CreateHighlightDialog.js

@@ -265,13 +265,10 @@ class CreateHighlightDialog extends EventEmitter {
     onCommentClick = () => {
         this.emit(CREATE_EVENT.comment);
         this.renderAnnotationPopover(TYPES.highlight_comment);
-
-        // $FlowFixMe
-        document.getSelection().removeAllRanges();
     };
 
     /**
-     * Cancels adding a comment to the highlgiht annotation by rendering a plain highlight popover
+     * Cancels adding a comment to the highlight annotation by rendering a plain highlight popover
      *
      * @return {void}
      */

src/doc/DocAnnotator.js

@@ -247,6 +247,12 @@ class DocAnnotator extends Annotator {
      * @return {void}
      */
     renderPage(pageNum: number) {
+        // $FlowFixMe
+        document.getSelection().removeAllRanges();
+        if (this.highlighter) {
+            this.highlighter.removeAllHighlights();
+        }
+
         // Scale existing canvases on re-render
         this.scaleAnnotationCanvases(pageNum);
         super.renderPage(pageNum);
@@ -412,7 +418,6 @@ class DocAnnotator extends Annotator {
         ) {
             mouseEvent.stopPropagation();
             mouseEvent.preventDefault();
-            this.highlighter.removeAllHighlights();
             this.resetHighlightSelection(mouseEvent);
             return;
         }
@@ -442,6 +447,7 @@ class DocAnnotator extends Annotator {
 
         // $FlowFixMe
         this.createHighlightDialog.unmountPopover();
+        this.isCreatingHighlight = false;
     }
 
     /**
@@ -455,6 +461,9 @@ class DocAnnotator extends Annotator {
 
         // $FlowFixMe
         document.getSelection().removeAllRanges();
+        if (this.highlighter) {
+            this.highlighter.removeAllHighlights();
+        }
     }
 
     /**
@@ -532,37 +541,38 @@ class DocAnnotator extends Annotator {
             this.selectionEndTimeout = null;
         }
 
+        // Bail if mid highlight and tapping on the screen
+        const selection = window.getSelection();
+        const isClickOutsideCreateDialog = this.isCreatingHighlight && util.isInDialog(event);
+        if (!docUtil.isValidSelection(selection) || isClickOutsideCreateDialog) {
+            this.lastHighlightEvent = null;
+            this.resetHighlightSelection(event);
+            return;
+        }
+
         // Do nothing if in a text area or mobile dialog or mobile create dialog is already open
         const pointController = this.modeControllers[TYPES.point];
         const isCreatingPoint = !!(pointController && pointController.pendingThreadID);
         const isPopoverActive = !!util.findClosestElWithClass(document.activeElement, CLASS_ANNOTATION_POPOVER);
-        if (isCreatingPoint || isPopoverActive) {
+        if (this.isCreatingHighlight || isCreatingPoint || isPopoverActive) {
             return;
         }
 
-        const selection = window.getSelection();
-
         // If we're creating a new selection, make sure to clear out to avoid
         // incorrect text being selected
-        if (!this.lastSelection || !selection || !docUtil.hasSelectionChanged(selection, this.lastSelection)) {
-            this.highlighter.removeAllHighlights();
-        }
-
-        // Bail if mid highlight and tapping on the screen
-        if (!docUtil.isValidSelection(selection)) {
-            this.lastHighlightEvent = null;
-
-            // $FlowFixMe
-            this.createHighlightDialog.unmountPopover();
+        if (
+            this.highlighter &&
+            (!this.lastSelection || !selection || !docUtil.hasSelectionChanged(selection, this.lastSelection))
+        ) {
             this.highlighter.removeAllHighlights();
-            return;
         }
 
         this.selectionEndTimeout = setTimeout(() => {
             if (this.createHighlightDialog && !this.createHighlightDialog.isVisible) {
                 this.createHighlightDialog.show(this.lastSelection);
             }
         }, SELECTION_TIMEOUT);
+        this.isCreatingHighlight = true;
 
         const { page } = util.getPageInfo(event.target);
 

src/doc/DocHighlightThread.js

@@ -115,22 +115,19 @@ class DocHighlightThread extends AnnotationThread {
         return super.save(type, text);
     }
 
-    /**
-     * Deletes an annotation.
-     *
-     * @param {Object} annotation annotation to delete
-     * @param {boolean} [useServer] Whether or not to delete on server, default true
-     * @return {void}
-     */
-    delete(annotation: Object, useServer: boolean = true): Promise<any> {
-        const promise = super.delete(annotation, useServer);
+    /** @inheritdoc */
+    deleteSuccessHandler = () => {
+        // Broadcast annotation deletion event
+        this.emit(THREAD_EVENT.delete);
 
         if (this.threadID) {
             this.renderAnnotationPopover();
+        } else {
+            // $FlowFixMe
+            const { page } = this.location;
+            this.emit(THREAD_EVENT.render, { page });
         }
-
-        return promise;
-    }
+    };
 
     /**
      * Mousedown handler for thread. Deletes this thread if it is still pending.

src/doc/__tests__/CreateHighlightDialog-test.js

@@ -158,9 +158,6 @@ describe('doc/CreateHighlightDialog', () => {
 
     describe('onCommentClick()', () => {
         it('should create a plain highlight and render the popover', () => {
-            document.getSelection = jest.fn().mockReturnValue({
-                removeAllRanges: jest.fn()
-            });
             dialog.onCommentClick({ preventDefault: jest.fn(), stopPropagation: jest.fn() });
             expect(dialog.emit).toBeCalledWith(CREATE_EVENT.comment);
             expect(dialog.renderAnnotationPopover).toBeCalled();

src/doc/__tests__/DocAnnotator-test.js

@@ -411,6 +411,12 @@ describe('doc/DocAnnotator', () => {
 
     describe('renderPage()', () => {
         beforeEach(() => {
+            document.getSelection = jest.fn().mockReturnValue({
+                removeAllRanges: jest.fn()
+            });
+            annotator.highlighter = {
+                removeAllHighlights: jest.fn()
+            };
             annotator.scaleAnnotationCanvases = jest.fn();
             annotator.modeControllers = {
                 type: {
@@ -785,10 +791,12 @@ describe('doc/DocAnnotator', () => {
             annotator.lastSelection = {};
 
             annotator.highlighter = { removeAllHighlights: jest.fn() };
+            annotator.resetHighlightSelection = jest.fn();
 
             window.getSelection = jest.fn();
             util.getPageInfo = jest.fn().mockReturnValue({ page: 1 });
             util.findClosestElWithClass = jest.fn().mockReturnValue(null);
+            annotator.isCreatingHighlight = false;
         });
 
         it('should reset the selectionEndTimeout', () => {
@@ -798,10 +806,20 @@ describe('doc/DocAnnotator', () => {
             expect(annotator.selectionEndTimeout).toBeNull();
         });
 
-        it('should do nothing if focus is on a text input element', () => {
-            util.findClosestElWithClass = jest.fn().mockReturnValue({});
+        it('should clear selection if the user is currently creating a highlight annotation', () => {
+            annotator.isCreatingHighlight = true;
+            util.isInDialog = jest.fn().mockReturnValue(true);
+            annotator.onSelectionChange(event);
+            expect(annotator.resetHighlightSelection).toBeCalled();
+        });
+
+        it('should clear out highlights and exit "annotation creation" mode if an invalid selection', () => {
+            annotator.lastHighlightEvent = event;
+            docUtil.isValidSelection = jest.fn().mockReturnValue(false);
+
             annotator.onSelectionChange(event);
-            expect(window.getSelection).not.toBeCalled();
+            expect(annotator.lastHighlightEvent).toBeNull();
+            expect(annotator.resetHighlightSelection).toBeCalled();
         });
 
         it('should do nothing the the user is currently creating a point annotation', () => {
@@ -810,7 +828,7 @@ describe('doc/DocAnnotator', () => {
             };
             controller.pendingThreadID = 'something';
             annotator.onSelectionChange(event);
-            expect(window.getSelection).not.toBeCalled();
+            expect(annotator.isCreatingHighlight).toBeFalsy();
         });
 
         it('should clear selection if the highlight has not changed', () => {
@@ -825,20 +843,6 @@ describe('doc/DocAnnotator', () => {
             expect(annotator.highlighter.removeAllHighlights).toBeCalled();
         });
 
-        it('should clear out highlights and exit "annotation creation" mode if an invalid selection', () => {
-            const selection = {
-                toString: () => '' // Causes invalid selection
-            };
-            window.getSelection = jest.fn().mockReturnValue(selection);
-            annotator.lastHighlightEvent = event;
-            docUtil.hasSelectionChanged = jest.fn().mockReturnValue(true);
-            docUtil.isValidSelection = jest.fn().mockReturnValue(false);
-
-            annotator.onSelectionChange(event);
-            expect(annotator.lastHighlightEvent).toBeNull();
-            expect(annotator.highlighter.removeAllHighlights).toBeCalled();
-        });
-
         it('should show the createHighlightDialog', () => {
             const selection = {
                 rangeCount: 10,
@@ -852,6 +856,7 @@ describe('doc/DocAnnotator', () => {
 
             annotator.onSelectionChange(event);
             expect(annotator.selectionEndTimeout).not.toBeNull();
+            expect(annotator.isCreatingHighlight).toBeTruthy();
         });
 
         it('should set all of the highlight annotations on the page to "inactive" state', () => {
@@ -947,6 +952,9 @@ describe('doc/DocAnnotator', () => {
             annotator.modeControllers = {
                 highlight: controller
             };
+            annotator.highlighter = {
+                removeAllHighlights: jest.fn()
+            };
             annotator.activeThread = undefined;
             annotator.plainHighlightEnabled = false;
             annotator.commentHighlightEnabled = false;