Fix: 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,10 @@ class DocAnnotator extends Annotator {
      * @return {void}
      */
     renderPage(pageNum: number) {
+        // $FlowFixMe
+        document.getSelection().removeAllRanges();
+        this.highlighter.removeAllHighlights();
+
         // Scale existing canvases on re-render
         this.scaleAnnotationCanvases(pageNum);
         super.renderPage(pageNum);
@@ -442,6 +446,7 @@ class DocAnnotator extends Annotator {
 
         // $FlowFixMe
         this.createHighlightDialog.unmountPopover();
+        this.isCreatingHighlight = false;
     }
 
     /**
@@ -532,11 +537,18 @@ class DocAnnotator extends Annotator {
             this.selectionEndTimeout = null;
         }
 
+        const isClickOutsideCreateDialog = this.isCreatingHighlight && util.isInDialog(event);
+        if (isClickOutsideCreateDialog) {
+            this.hideCreateDialog(event);
+            this.highlighter.removeAllHighlights();
+            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;
         }
 
@@ -563,6 +575,7 @@ class DocAnnotator extends Annotator {
                 this.createHighlightDialog.show(this.lastSelection);
             }
         }, SELECTION_TIMEOUT);
+        this.isCreatingHighlight = true;
 
         const { page } = util.getPageInfo(event.target);
 
@@ -785,6 +798,7 @@ class DocAnnotator extends Annotator {
             return true;
         }
 
+        this.highlighter.removeAllHighlights();
         this.resetHighlightSelection(event);
         return false;
     }

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: {
@@ -813,6 +819,12 @@ describe('doc/DocAnnotator', () => {
             expect(window.getSelection).not.toBeCalled();
         });
 
+        it('should do nothing the the user is currently creating a highlight annotation', () => {
+            annotator.isCreatingHighlight = true;
+            annotator.onSelectionChange(event);
+            expect(window.getSelection).not.toBeCalled();
+        });
+
         it('should clear selection if the highlight has not changed', () => {
             const selection = {
                 anchorNode: 'derp',
@@ -852,6 +864,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 +960,9 @@ describe('doc/DocAnnotator', () => {
             annotator.modeControllers = {
                 highlight: controller
             };
+            annotator.highlighter = {
+                removeAllHighlights: jest.fn()
+            };
             annotator.activeThread = undefined;
             annotator.plainHighlightEnabled = false;
             annotator.commentHighlightEnabled = false;