Fix: Fix draw selection and related bugs (#440)

* Fix: Fix draw selection and related bugs * Fixes selection on Firefox/Safari * Fixes interaction between text selection and drawings * Replaces text cursor with crosshair * Chore: Moving selection clearing out of CreateHighlightDialog
box · Oct 23, 2017 · 6a667f1 · 6a667f1
1 parent b11f2dd
commit 6a667f1
Show file tree

Hide file tree

Showing 10 changed files with 95 additions and 5 deletions.
diff --git a/src/lib/annotations/Annotator.js b/src/lib/annotations/Annotator.js
@@ -265,6 +265,11 @@ class Annotator extends EventEmitter {
 
         this.destroyPendingThreads();
 
+        if (this.createHighlightDialog.isVisible) {
+            document.getSelection().removeAllRanges();
+            this.createHighlightDialog.hide();
+        }
+
         // No specific mode available for annotation type
         if (!(mode in this.modeButtons)) {
             return;

diff --git a/src/lib/annotations/Annotator.scss b/src/lib/annotations/Annotator.scss
@@ -407,8 +407,6 @@ $tablet: 'max-width: 768px';
 //------------------------------------------------------------------------------
 // CSS for highlights
 //------------------------------------------------------------------------------
-.bp-annotation-layer-draw,
-.bp-annotation-layer-draw-in-progress,
 .bp-annotation-layer-highlight {
     cursor: text;
     left: 0;
@@ -550,6 +548,22 @@ $tablet: 'max-width: 768px';
     }
 }
 
+.bp-annotation-draw .textLayer {
+    pointer-events: none;
+
+    > div {
+        pointer-events: auto;
+    }
+}
+
+.bp-annotation-layer-draw,
+.bp-annotation-layer-draw-in-progress {
+    left: 0;
+    mix-blend-mode: multiply;
+    position: absolute;
+    top: 15px; // Match 15px padding top on page
+}
+
 //------------------------------------------------------------------------------
 // Annotation mode
 //------------------------------------------------------------------------------

diff --git a/src/lib/annotations/__tests__/Annotator-test.js b/src/lib/annotations/__tests__/Annotator-test.js
@@ -361,6 +361,11 @@ describe('lib/annotations/Annotator', () => {
                     point: { selector: 'point_btn' },
                     draw: { selector: 'draw_btn' }
                 };
+
+                annotator.createHighlightDialog = {
+                    isVisible: false,
+                    hide: sandbox.stub()
+                }
             });
 
             afterEach(() => {
@@ -379,6 +384,22 @@ describe('lib/annotations/Annotator', () => {
                 expect(stubs.exitModes).to.not.be.called;
             });
 
+            it('should hide the highlight dialog and remove selection if it is visible', () => {
+                const getSelectionStub = sandbox.stub(document, 'getSelection').returns({
+                    removeAllRanges: sandbox.stub()
+                });
+
+                annotator.toggleAnnotationHandler(TYPES.highlight);
+                expect(annotator.createHighlightDialog.hide).to.not.be.called;
+                expect(getSelectionStub).to.not.be.called;
+
+                annotator.createHighlightDialog.isVisible = true;
+
+                annotator.toggleAnnotationHandler(TYPES.highlight);
+                expect(annotator.createHighlightDialog.hide).to.be.called;
+                expect(getSelectionStub).to.be.called;
+            });
+
             it('should turn annotation mode on if it is off', () => {
                 stubs.annotationMode.returns(false);
 

diff --git a/src/lib/annotations/annotationConstants.js b/src/lib/annotations/annotationConstants.js
@@ -27,6 +27,7 @@ export const CLASS_TEXTAREA = 'bp-textarea';
 export const CLASS_HIGHLIGHT_BTNS = 'bp-annotation-highlight-btns';
 export const CLASS_ADD_HIGHLIGHT_BTN = 'bp-add-highlight-btn';
 export const CLASS_ADD_HIGHLIGHT_COMMENT_BTN = 'bp-highlight-comment-btn';
+export const CLASS_ANNOTATION_DRAW = 'bp-annotation-draw';
 export const CLASS_ANNOTATION_LAYER_HIGHLIGHT = 'bp-annotation-layer-highlight';
 export const CLASS_ANNOTATION_LAYER_DRAW = 'bp-annotation-layer-draw';
 export const CLASS_ANNOTATION_LAYER_DRAW_IN_PROGRESS = 'bp-annotation-layer-draw-in-progress';

diff --git a/src/lib/annotations/doc/DocAnnotator.js b/src/lib/annotations/doc/DocAnnotator.js
@@ -838,8 +838,9 @@ class DocAnnotator extends Annotator {
             this.highlighter.removeAllHighlights();
         }
 
-        if (this.createHighlightDialog) {
+        if (this.createHighlightDialog.isVisible) {
             this.createHighlightDialog.hide();
+            document.getSelection().removeAllRanges();
         }
 
         this.isCreatingHighlight = false;

diff --git a/src/lib/annotations/doc/__tests__/CreateHighlightDialog-test.html b/src/lib/annotations/doc/__tests__/CreateHighlightDialog-test.html
@@ -0,0 +1 @@
+<div class="bp-create-highlight-dialog-container"></div>
diff --git a/src/lib/annotations/doc/__tests__/CreateHighlightDialog-test.js b/src/lib/annotations/doc/__tests__/CreateHighlightDialog-test.js
@@ -20,8 +20,14 @@ describe('lib/annotations/doc/CreateHighlightDialog', () => {
         highlightComment: 'highlight comment'
     };
 
+    before(() => {
+        fixture.setBase('src/lib');
+    });
+
     beforeEach(() => {
-        parentEl = document.createElement('div');
+        fixture.load('annotations/doc/__tests__/CreateHighlightDialog-test.html');
+
+        const parentEl = document.querySelector('.bp-create-highlight-dialog-container');
         dialog = new CreateHighlightDialog(parentEl, { localized });
     });
 
@@ -94,7 +100,10 @@ describe('lib/annotations/doc/CreateHighlightDialog', () => {
         });
 
         it('should move the UI element to the parent element if it does not already contain it', () => {
-            const append = sandbox.stub(parentEl, 'appendChild');
+            const newParent = document.createElement('span');
+            dialog.setParentEl(newParent);
+            const append = sandbox.stub(newParent, 'appendChild');
+
             dialog.show();
             expect(append).to.be.calledWith(dialog.containerEl);
         });

diff --git a/src/lib/annotations/doc/__tests__/DocAnnotator-test.js b/src/lib/annotations/doc/__tests__/DocAnnotator-test.js
@@ -1078,6 +1078,29 @@ describe('lib/annotations/doc/DocAnnotator', () => {
             annotator.highlightMouseupHandler({ x: 0, y: 0 });
             expect(annotator.highlighter.removeAllHighlights).to.be.called;
         });
+
+        it('should hide the highlight dialog and clear selection if it is visible', () => {
+            annotator.createHighlightDialog = {
+                isVisible: false,
+                hide: sandbox.stub(),
+                removeListener: sandbox.stub(),
+                destroy: sandbox.stub()
+            }
+
+            const getSelectionStub = sandbox.stub(document, 'getSelection').returns({
+                removeAllRanges: sandbox.stub()
+            });
+
+            annotator.highlightMouseupHandler({ x: 0, y: 0 });
+            expect(annotator.createHighlightDialog.hide).to.not.be.called;
+            expect(getSelectionStub).to.not.be.called;
+
+            annotator.createHighlightDialog.isVisible = true;
+
+            annotator.highlightMouseupHandler({ x: 0, y: 0 });
+            expect(annotator.createHighlightDialog.hide).to.be.called;
+            expect(getSelectionStub).to.be.called;
+        });
     });
 
     describe('onSelectionChange()', () => {

diff --git a/src/lib/annotations/drawing/DrawingModeController.js b/src/lib/annotations/drawing/DrawingModeController.js
@@ -3,6 +3,7 @@ import AnnotationModeController from '../AnnotationModeController';
 import annotationsShell from './../annotationsShell.html';
 import * as annotatorUtil from '../annotatorUtil';
 import {
+    CLASS_ANNOTATION_DRAW,
     TYPES,
     STATES,
     SELECTOR_ANNOTATION_BUTTON_DRAW_CANCEL,
@@ -53,6 +54,8 @@ class DrawingModeController extends AnnotationModeController {
         this.postButtonEl = annotator.getAnnotateButton(SELECTOR_ANNOTATION_BUTTON_DRAW_POST);
         this.undoButtonEl = annotator.getAnnotateButton(SELECTOR_ANNOTATION_BUTTON_DRAW_UNDO);
         this.redoButtonEl = annotator.getAnnotateButton(SELECTOR_ANNOTATION_BUTTON_DRAW_REDO);
+
+        this.annotator.annotatedElement.classList.add(CLASS_ANNOTATION_DRAW);
     }
 
     /**

diff --git a/src/lib/annotations/drawing/__tests__/DrawingModeController-test.js b/src/lib/annotations/drawing/__tests__/DrawingModeController-test.js
@@ -1,6 +1,7 @@
 import AnnotationModeController from '../../AnnotationModeController';
 import DrawingModeController from '../DrawingModeController';
 import * as annotatorUtil from '../../annotatorUtil';
+import { CLASS_ANNOTATION_DRAW} from '../../annotationConstants';
 
 let drawingModeController;
 let stubs;
@@ -23,6 +24,11 @@ describe('lib/annotations/drawing/DrawingModeController', () => {
             getAnnotateButton: sandbox.stub(),
             options: {
                 header: 'none'
+            },
+            annotatedElement: {
+                classList: {
+                    add: sandbox.stub()
+                }
             }
         };
         it('should use the annotator to get button elements', () => {
@@ -53,6 +59,12 @@ describe('lib/annotations/drawing/DrawingModeController', () => {
             drawingModeController.registerAnnotator(annotator);
             expect(setupHeaderStub).to.be.called;
         });
+
+        it('should add the draw class to the annotated element', () => {
+            annotator.options.header = 'none';
+            drawingModeController.registerAnnotator(annotator);
+            expect(annotator.annotatedElement.classList.add).to.be.calledWith(CLASS_ANNOTATION_DRAW);
+        });
     });
 
     describe('registerThread()', () => {
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		<div class="bp-create-highlight-dialog-container"></div>