From 6a667f1c680a9cd244eaed32a2808fcfcc8bdce2 Mon Sep 17 00:00:00 2001 From: Jeremy Press Date: Mon, 23 Oct 2017 15:02:46 -0700 Subject: [PATCH] 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 --- src/lib/annotations/Annotator.js | 5 ++++ src/lib/annotations/Annotator.scss | 18 +++++++++++++-- .../annotations/__tests__/Annotator-test.js | 21 +++++++++++++++++ src/lib/annotations/annotationConstants.js | 1 + src/lib/annotations/doc/DocAnnotator.js | 3 ++- .../__tests__/CreateHighlightDialog-test.html | 1 + .../__tests__/CreateHighlightDialog-test.js | 13 +++++++++-- .../doc/__tests__/DocAnnotator-test.js | 23 +++++++++++++++++++ .../drawing/DrawingModeController.js | 3 +++ .../__tests__/DrawingModeController-test.js | 12 ++++++++++ 10 files changed, 95 insertions(+), 5 deletions(-) create mode 100644 src/lib/annotations/doc/__tests__/CreateHighlightDialog-test.html diff --git a/src/lib/annotations/Annotator.js b/src/lib/annotations/Annotator.js index 74bc043ae..6d3d0efa6 100644 --- 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 index 12ed7fce5..1baaefe9e 100644 --- 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 index 431aa60ab..82f567e24 100644 --- 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 index 4d59361b0..4c93c286a 100644 --- 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 index 5af21c8ff..e4476cce8 100644 --- 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 new file mode 100644 index 000000000..f6eed9609 --- /dev/null +++ b/src/lib/annotations/doc/__tests__/CreateHighlightDialog-test.html @@ -0,0 +1 @@ +
diff --git a/src/lib/annotations/doc/__tests__/CreateHighlightDialog-test.js b/src/lib/annotations/doc/__tests__/CreateHighlightDialog-test.js index 1897944f3..3821f75ba 100644 --- 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 index a4229b91a..ef352bd7a 100644 --- 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 index 3f65a5b22..f62e50a29 100644 --- 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 index d33a295db..95f0192a9 100644 --- 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()', () => {