From f225c133b39e365e349a9061b40acc0831f7cbe3 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Wed, 23 Jan 2019 15:55:04 -0800 Subject: [PATCH 1/6] Fix: Highlight selection on Surface - Do not reset highlight selection on 'selectionchange' event on touch enabled devices when the user is creating a highlight annotation --- src/doc/DocAnnotator.js | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/src/doc/DocAnnotator.js b/src/doc/DocAnnotator.js index 90271a4d3..a688d65e5 100644 --- a/src/doc/DocAnnotator.js +++ b/src/doc/DocAnnotator.js @@ -462,8 +462,12 @@ class DocAnnotator extends Annotator { resetHighlightSelection(event: ?Event) { this.hideCreateDialog(event); - // $FlowFixMe - window.getSelection().removeAllRanges(); + const selection = window.getSelection(); + if (selection.rangeCount > 0) { + // $FlowFixMe + selection.removeAllRanges(); + } + if (this.highlighter) { this.highlighter.removeAllHighlights(); } @@ -557,13 +561,15 @@ class DocAnnotator extends Annotator { this.selectionEndTimeout = null; } - this.resetHighlightOnOutsideClick(event); + if (!this.isCreatingHighlight) { + this.resetHighlightOnOutsideClick(event); + } // 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 (this.isCreatingHighlight || isCreatingPoint || isPopoverActive) { + if (isCreatingPoint || isPopoverActive) { return; } @@ -577,7 +583,7 @@ class DocAnnotator extends Annotator { } this.selectionEndTimeout = setTimeout(() => { - if (this.createHighlightDialog && !this.createHighlightDialog.isVisible) { + if (this.createHighlightDialog) { this.createHighlightDialog.show(this.lastSelection); } }, SELECTION_TIMEOUT); @@ -594,12 +600,8 @@ class DocAnnotator extends Annotator { this.modeControllers[TYPES.highlight_comment].applyActionToThreads((thread) => thread.reset(), page); } - let mouseEvent = event; // $FlowFixMe - if (this.hasTouch && event.targetTouches) { - mouseEvent = event.targetTouches[0]; - } - this.lastHighlightEvent = mouseEvent; + this.lastHighlightEvent = this.hasTouch && event.targetTouches ? event.targetTouches[0] : event; this.lastSelection = selection; } @@ -774,15 +776,17 @@ class DocAnnotator extends Annotator { * Bail if mid highlight and click is outside highlight/selection * * @param {Event} event - Mouse event - * @return {void} + * @return {boolean} - Whether or not event was consumed */ - resetHighlightOnOutsideClick(event: Event) { + resetHighlightOnOutsideClick(event: Event): boolean { const selection = window.getSelection(); const isClickOutsideCreateDialog = this.isCreatingHighlight && !util.isInDialog(event); if (!docUtil.isValidSelection(selection) && isClickOutsideCreateDialog) { this.lastHighlightEvent = null; this.resetHighlightSelection(event); + return true; } + return false; } /** From c79a98ad50ff783159117e255fd2c1e60f7d0a60 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Wed, 23 Jan 2019 17:14:52 -0800 Subject: [PATCH 2/6] Chore: Remove unecessary changes --- src/doc/DocAnnotator.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/doc/DocAnnotator.js b/src/doc/DocAnnotator.js index a688d65e5..de117d7f3 100644 --- a/src/doc/DocAnnotator.js +++ b/src/doc/DocAnnotator.js @@ -462,12 +462,8 @@ class DocAnnotator extends Annotator { resetHighlightSelection(event: ?Event) { this.hideCreateDialog(event); - const selection = window.getSelection(); - if (selection.rangeCount > 0) { - // $FlowFixMe - selection.removeAllRanges(); - } - + // $FlowFixMe + window.getSelection().removeAllRanges(); if (this.highlighter) { this.highlighter.removeAllHighlights(); } From ca465718b364db7e675a1aca7f1cba628f62ef2a Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Wed, 23 Jan 2019 17:42:53 -0800 Subject: [PATCH 3/6] Update: Tests --- src/doc/__tests__/DocAnnotator-test.js | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/src/doc/__tests__/DocAnnotator-test.js b/src/doc/__tests__/DocAnnotator-test.js index 2170fb8ca..94cf71d8e 100644 --- a/src/doc/__tests__/DocAnnotator-test.js +++ b/src/doc/__tests__/DocAnnotator-test.js @@ -846,11 +846,12 @@ describe('doc/DocAnnotator', () => { expect(annotator.selectionEndTimeout).not.toEqual(123); }); - it('should clear selection if the user is currently creating a highlight annotation', () => { - annotator.isCreatingHighlight = true; + it('should clear selection if the user is NOT currently creating a highlight annotation', () => { + annotator.isCreatingHighlight = false; util.isInDialog = jest.fn().mockReturnValue(true); annotator.onSelectionChange(event); expect(annotator.resetHighlightOnOutsideClick).toBeCalled(); + expect(util.findClosestElWithClass).toBeCalled(); }); it('should clear out highlights and exit "annotation creation" mode if an invalid selection', () => { @@ -899,6 +900,25 @@ describe('doc/DocAnnotator', () => { expect(annotator.isCreatingHighlight).toBeTruthy(); }); + it('should reposition the existing createHighlightDialog', () => { + const selection = { + rangeCount: 10, + isCollapsed: false, + anchorNode: { + parentNode: 'Node' + }, + toString: () => 'asdf' + }; + window.getSelection = jest.fn().mockReturnValue(selection); + docUtil.hasSelectionChanged = jest.fn().mockReturnValue(true); + annotator.lastHighlightEvent = event; + annotator.createHighlightDialog.isVisible = true; + + 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', () => { const selection = { rangeCount: 10, From 63b8a58d477c67c0fa6e2e056e9ccfb3a93aa646 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Thu, 24 Jan 2019 13:55:04 -0800 Subject: [PATCH 4/6] Chore: Cleanup --- src/doc/DocAnnotator.js | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/doc/DocAnnotator.js b/src/doc/DocAnnotator.js index de117d7f3..7859fa044 100644 --- a/src/doc/DocAnnotator.js +++ b/src/doc/DocAnnotator.js @@ -1,5 +1,6 @@ // @flow import rangy from 'rangy'; +import get from 'lodash/get'; /* eslint-disable no-unused-vars */ // Workaround for rangy npm issue: https://github.com/timdown/rangy/lib/issues/342 import rangyClassApplier from 'rangy/lib/rangy-classapplier'; @@ -596,8 +597,7 @@ class DocAnnotator extends Annotator { this.modeControllers[TYPES.highlight_comment].applyActionToThreads((thread) => thread.reset(), page); } - // $FlowFixMe - this.lastHighlightEvent = this.hasTouch && event.targetTouches ? event.targetTouches[0] : event; + this.lastHighlightEvent = this.hasTouch ? get(event, 'targetTouches[0]', event) : event; this.lastSelection = selection; } @@ -772,17 +772,15 @@ class DocAnnotator extends Annotator { * Bail if mid highlight and click is outside highlight/selection * * @param {Event} event - Mouse event - * @return {boolean} - Whether or not event was consumed + * @return {void} */ - resetHighlightOnOutsideClick(event: Event): boolean { + resetHighlightOnOutsideClick(event: Event) { const selection = window.getSelection(); const isClickOutsideCreateDialog = this.isCreatingHighlight && !util.isInDialog(event); if (!docUtil.isValidSelection(selection) && isClickOutsideCreateDialog) { this.lastHighlightEvent = null; this.resetHighlightSelection(event); - return true; } - return false; } /** From 9671d7f0ca3ab4af093a69bcb7bbcc4792381d65 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Thu, 24 Jan 2019 16:25:51 -0800 Subject: [PATCH 5/6] Chore: Remove this.hasTouch check in onSelectionChange() --- src/doc/DocAnnotator.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/doc/DocAnnotator.js b/src/doc/DocAnnotator.js index 7859fa044..ecaaf245f 100644 --- a/src/doc/DocAnnotator.js +++ b/src/doc/DocAnnotator.js @@ -597,7 +597,7 @@ class DocAnnotator extends Annotator { this.modeControllers[TYPES.highlight_comment].applyActionToThreads((thread) => thread.reset(), page); } - this.lastHighlightEvent = this.hasTouch ? get(event, 'targetTouches[0]', event) : event; + this.lastHighlightEvent = get(event, 'targetTouches[0]', event); this.lastSelection = selection; } From d86ec2c3dce31150d3b77ca89b82cceef7e39744 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Thu, 24 Jan 2019 17:51:52 -0800 Subject: [PATCH 6/6] Chore: Don't reset on clickHandler after onSelectionChange() is called --- src/doc/DocAnnotator.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/doc/DocAnnotator.js b/src/doc/DocAnnotator.js index ecaaf245f..06f0ce893 100644 --- a/src/doc/DocAnnotator.js +++ b/src/doc/DocAnnotator.js @@ -416,6 +416,7 @@ class DocAnnotator extends Annotator { if ( // $FlowFixMe this.hasMouseMoved(this.lastHighlightEvent, event) && + !this.isCreatingHighlight && this.createHighlightDialog && this.createHighlightDialog.isVisible && !this.createHighlightDialog.isInHighlight(mouseEvent)