Fix: ability to create highlight annotations on a Microsoft Surface (#…

…108)

* Fix: ability to create highlight annotations on a Microsoft Surface
* Fix: Ensure mouseleave events don't trigger on touch
* Fix: Hover behavior on surface

src/AnnotationDialog.js

@@ -448,10 +448,12 @@ class AnnotationDialog extends EventEmitter {
      * Mouseleave handler. Hides dialog if we aren't creating the first one.
      *
      * @protected
+     * @param {Event} event DOM event
      * @return {void}
      */
-    mouseleaveHandler() {
-        if (this.hasAnnotations) {
+    mouseleaveHandler(event) {
+        const isStillInDialog = util.isInDialog(event, event.toElement);
+        if (!isStillInDialog && this.hasAnnotations) {
             this.hide();
         }
     }

src/AnnotationThread.js

@@ -551,7 +551,7 @@ class AnnotationThread extends EventEmitter {
             return;
         }
 
-        const mouseInDialog = util.isInDialog(event, this.dialog.element);
+        const mouseInDialog = util.isInDialog(event, event.toElement);
 
         const firstAnnotation = util.getFirstAnnotation(this.annotations);
         if (firstAnnotation && !mouseInDialog) {

src/CommentBox.js

@@ -279,16 +279,16 @@ class CommentBox extends EventEmitter {
 
         // Add event listeners
         if (this.hasTouch) {
-            this.textAreaEl.addEventListener('focus', this.focus);
             containerEl.addEventListener('touchend', this.preventDefaultAndPropagation.bind(this));
             this.cancelEl.addEventListener('touchend', this.onCancel);
             this.postEl.addEventListener('touchend', this.onPost);
-        } else {
-            this.textAreaEl.addEventListener('focus', this.focus);
-            this.cancelEl.addEventListener('click', this.onCancel);
-            this.postEl.addEventListener('click', this.onPost);
         }
 
+        this.textAreaEl.addEventListener('focus', this.focus);
+        containerEl.addEventListener('click', this.preventDefaultAndPropagation.bind(this));
+        this.cancelEl.addEventListener('click', this.onCancel);
+        this.postEl.addEventListener('click', this.onPost);
+
         return containerEl;
     }
 }

src/__tests__/AnnotationDialog-test.js

@@ -583,26 +583,31 @@ describe('AnnotationDialog', () => {
     });
 
     describe('mouseleaveHandler()', () => {
-        it('should not do anything if there are no annotations in the dialog', () => {
+        beforeEach(() => {
             stubs.hide = sandbox.stub(dialog, 'hide');
+            stubs.annotation = new Annotation({
+                annotationID: 'someID',
+                text: 'blah',
+                user: {},
+                permissions: {}
+            });
+        });
 
-            dialog.mouseleaveHandler();
+        it('should not do anything if there are no annotations in the dialog', () => {
+            dialog.mouseleaveHandler({});
             expect(stubs.hide).to.not.be.called;
         });
 
-        it('should hide dialog if there are annotations in the dialog', () => {
-            stubs.hide = sandbox.stub(dialog, 'hide');
-
-            dialog.addAnnotation(
-                new Annotation({
-                    annotationID: 'someID',
-                    text: 'blah',
-                    user: {},
-                    permissions: {}
-                })
-            );
+        it('should not do anything if the mouse is still in the dialog', () => {
+            dialog.addAnnotation(stubs.annotation);
+            sandbox.stub(util, 'isInDialog').returns(true);
+            dialog.mouseleaveHandler({});
+            expect(stubs.hide).to.not.be.called;
+        });
 
-            dialog.mouseleaveHandler();
+        it('should hide dialog if there are annotations in the dialog', () => {
+            dialog.addAnnotation(stubs.annotation);
+            dialog.mouseleaveHandler({});
             expect(stubs.hide).to.be.called;
         });
     });

src/__tests__/CommentBox-test.js

@@ -288,7 +288,7 @@ describe('CommentBox', () => {
             expect(uiElement.addEventListener).to.be.calledWith('focus', commentBox.focus);
         });
 
-        it('should add an event listener on the textarea, cancel and post buttons if the user is on a touch-enabled mobile device', () => {
+        it('should add an event listener on the textarea, cancel and post buttons if the user is on a touch-enabled device', () => {
             const uiElement = {
                 addEventListener: sandbox.stub(),
                 removeEventListener: sandbox.stub()
@@ -304,6 +304,10 @@ describe('CommentBox', () => {
             expect(uiElement.addEventListener).to.be.calledWith('touchend', sinon.match.func);
             expect(uiElement.addEventListener).to.be.calledWith('touchend', sinon.match.func);
 
+            expect(uiElement.addEventListener).to.be.calledWith('click', commentBox.onCancel);
+            expect(uiElement.addEventListener).to.be.calledWith('click', commentBox.onPost);
+            expect(uiElement.addEventListener).to.be.calledWith('focus', commentBox.focus);
+
             commentBox.containerEl = null;
         });
     });

src/__tests__/util-test.js

@@ -36,7 +36,8 @@ import {
     insertTemplate,
     generateBtn,
     createCommentTextNode,
-    clearCanvas
+    clearCanvas,
+    isInDialog
 } from '../util';
 import {
     STATES,
@@ -217,6 +218,17 @@ describe('util', () => {
         });
     });
 
+    describe('isInDialog()', () => {
+        it('should return false if no dialog element exists', () => {
+            expect(isInDialog({ clientX: 8, clientY: 8 })).to.be.falsy;
+        });
+
+        it('should return true if the event is in the given dialog', () => {
+            const dialogEl = document.querySelector(SELECTOR_ANNOTATION_DIALOG);
+            expect(isInDialog({ clientX: 8, clientY: 8 }, dialogEl)).to.be.truthy;
+        });
+    });
+
     describe('insertTemplate()', () => {
         it('should insert template into node', () => {
             const node = document.createElement('div');

src/doc/CreateHighlightDialog.js

@@ -1,6 +1,7 @@
 import CreateAnnotationDialog from '../CreateAnnotationDialog';
 import { ICON_HIGHLIGHT, ICON_HIGHLIGHT_COMMENT } from '../icons/icons';
-import { generateBtn, repositionCaret } from '../util';
+import { generateBtn, repositionCaret, getPageInfo } from '../util';
+import { getDialogCoordsFromRange } from '../doc/docUtil';
 import {
     CREATE_EVENT,
     CLASS_ANNOTATION_CARET,
@@ -14,7 +15,8 @@ import {
     SELECTOR_ADD_HIGHLIGHT_COMMENT_BTN,
     CLASS_MOBILE_ANNOTATION_DIALOG,
     CLASS_ANNOTATION_DIALOG,
-    CLASS_BUTTON_PLAIN
+    CLASS_BUTTON_PLAIN,
+    PAGE_PADDING_TOP
 } from '../constants';
 
 const CLASS_CREATE_DIALOG = 'bp-create-annotation-dialog';
@@ -100,18 +102,55 @@ class CreateHighlightDialog extends CreateAnnotationDialog {
      * Show the dialog. Adds to the parent container if it isn't already there.
      *
      * @public
-     * @param {HTMLElement} [newParentEl] - The new parent container to nest this in.
+     * @param {HTMLElement} newParentEl The new parent container to nest this in.
+     * @param {HTMLElement} selection Current text selection
      * @return {void}
      */
-    show(newParentEl) {
-        super.show(newParentEl);
+    show(newParentEl, selection) {
+        if (!selection) {
+            return;
+        }
+
+        // Select page of first node selected
+        const pageInfo = getPageInfo(selection.anchorNode);
+        if (!pageInfo.pageEl) {
+            return;
+        }
+
+        const dialogParentEl = this.isMobile ? newParentEl : pageInfo.pageEl;
+        super.show(dialogParentEl);
+
+        if (!this.isMobile) {
+            this.setPosition(selection);
+        }
 
         // Add to parent if it hasn't been added already
         if (!this.parentEl.querySelector(`.${CLASS_CREATE_DIALOG}`)) {
             this.parentEl.appendChild(this.containerEl);
         }
     }
 
+    /** @inheritdoc */
+    setPosition(selection) {
+        if (!selection || !selection.rangeCount) {
+            return;
+        }
+
+        const lastRange = selection.getRangeAt(selection.rangeCount - 1);
+        const coords = getDialogCoordsFromRange(lastRange);
+
+        // Select page of first node selected
+        const pageInfo = getPageInfo(selection.anchorNode);
+        if (!pageInfo.pageEl) {
+            return;
+        }
+
+        const pageDimensions = pageInfo.pageEl.getBoundingClientRect();
+        const pageLeft = pageDimensions.left;
+        const pageTop = pageDimensions.top + PAGE_PADDING_TOP;
+        super.setPosition(coords.x - pageLeft, coords.y - pageTop);
+    }
+
     /**
      * [destructor]
      *

src/doc/DocAnnotator.js

@@ -23,7 +23,6 @@ import {
     CLASS_ANNOTATION_LAYER_HIGHLIGHT_COMMENT,
     CLASS_ANNOTATION_LAYER_DRAW,
     CLASS_ANNOTATION_PLAIN_HIGHLIGHT,
-    CLASS_HIDDEN,
     THREAD_EVENT,
     ANNOTATOR_EVENT,
     CONTROLLER_EVENT,
@@ -32,6 +31,7 @@ import {
 
 const MOUSEMOVE_THROTTLE_MS = 25;
 const HOVER_TIMEOUT_MS = 75;
+const SELECTION_TIMEOUT = 500;
 const MOUSE_MOVE_MIN_DISTANCE = 5;
 const CLASS_RANGY_HIGHLIGHT = 'rangy-highlight';
 
@@ -343,7 +343,7 @@ class DocAnnotator extends Annotator {
 
         this.clickThread = this.clickThread.bind(this);
 
-        if (this.isMobile && this.hasTouch) {
+        if (this.isMobile || this.hasTouch) {
             this.onSelectionChange = this.onSelectionChange.bind(this);
         }
 
@@ -417,7 +417,7 @@ class DocAnnotator extends Annotator {
             return;
         }
 
-        if (this.hasTouch && this.isMobile) {
+        if (this.hasTouch || this.isMobile) {
             document.addEventListener('selectionchange', this.onSelectionChange);
         } else {
             this.annotatedElement.addEventListener('dblclick', this.highlightMouseupHandler);
@@ -448,9 +448,8 @@ class DocAnnotator extends Annotator {
             controller.removeSelection();
         });
 
-        if (this.hasTouch && this.isMobile) {
+        if (this.hasTouch || this.isMobile) {
             document.removeEventListener('selectionchange', this.onSelectionChange);
-            this.annotatedElement.removeEventListener('touchstart', this.drawingSelectionHandler);
         } else {
             this.annotatedElement.removeEventListener('click', this.drawingSelectionHandler);
             this.annotatedElement.removeEventListener('dblclick', this.highlightMouseupHandler);
@@ -554,35 +553,42 @@ class DocAnnotator extends Annotator {
      * @return {void}
      */
     onSelectionChange(event) {
+        event.preventDefault();
+        event.stopPropagation();
+
+        if (this.selectionEndTimeout) {
+            clearTimeout(this.selectionEndTimeout);
+            this.selectionEndTimeout = null;
+        }
+
         // Do nothing if in a text area or mobile dialog or mobile create dialog is already open
-        const isHidden = this.mobileDialogEl && this.mobileDialogEl.classList.contains(CLASS_HIDDEN);
         const pointController = this.modeControllers[TYPES.point];
         const isCreatingPoint = !!(pointController && pointController.pendingThreadID);
-        if (isCreatingPoint || !isHidden || document.activeElement.nodeName.toLowerCase() === 'textarea') {
+        if (isCreatingPoint || document.activeElement.nodeName.toLowerCase() === 'textarea') {
             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 || (this.lastSelection && selection.anchorNode !== this.lastSelection.anchorNode)) {
+        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.lastSelection = null;
             this.lastHighlightEvent = null;
             this.createHighlightDialog.hide();
             this.highlighter.removeAllHighlights();
             return;
         }
 
-        const isCreateDialogVisible = this.createHighlightDialog && this.createHighlightDialog.isVisible;
-        if (!isCreateDialogVisible) {
-            this.createHighlightDialog.show(this.container);
-        }
+        this.selectionEndTimeout = setTimeout(() => {
+            if (this.createHighlightDialog) {
+                this.createHighlightDialog.show(this.container, selection);
+            }
+        }, SELECTION_TIMEOUT);
 
         const { page } = util.getPageInfo(event.target);
 
@@ -873,20 +879,8 @@ class DocAnnotator extends Annotator {
             return;
         }
 
-        const lastRange = selection.getRangeAt(selection.rangeCount - 1);
-        const { x, y } = docUtil.getDialogCoordsFromRange(lastRange);
-
-        const pageDimensions = pageEl.getBoundingClientRect();
-
-        const pageLeft = pageDimensions.left;
-        const pageTop = pageDimensions.top + PAGE_PADDING_TOP;
         const dialogParentEl = this.isMobile ? this.container : pageEl;
-
-        this.createHighlightDialog.show(dialogParentEl);
-
-        if (!this.isMobile) {
-            this.createHighlightDialog.setPosition(x - pageLeft, y - pageTop);
-        }
+        this.createHighlightDialog.show(dialogParentEl, selection);
 
         this.isCreatingHighlight = true;
         this.lastHighlightEvent = event;

src/doc/DocHighlightThread.js

@@ -190,7 +190,7 @@ class DocHighlightThread extends AnnotationThread {
      * the annotations dialog
      */
     isOnHighlight(event) {
-        return util.isInDialog(event, this.dialog.element) || this.isInHighlight(event);
+        return util.isInDialog(event) || this.isInHighlight(event);
     }
 
     /**
@@ -224,7 +224,7 @@ class DocHighlightThread extends AnnotationThread {
             return false;
 
             // If mouse is in dialog, change state to hover or active-hover
-        } else if (util.isInDialog(event, this.dialog.element)) {
+        } else if (util.isInDialog(event)) {
             // Keeps dialog open if comment is pending
             if (this.state === STATES.pending_active) {
                 return false;
@@ -527,7 +527,7 @@ class DocHighlightThread extends AnnotationThread {
                 context.fill();
 
                 // Update highlight icon hover to appropriate color
-                if (this.dialog.element) {
+                if (this.dialog && this.dialog.element) {
                     this.dialog.toggleHighlightIcon(fillStyle);
                 }
             }