From 6a64f6d6b56ba38d909a0e50a90fc1797f328696 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Mon, 7 Jan 2019 20:39:43 -0800 Subject: [PATCH 1/8] Fix: Double clicking to create highlight annotation - May also fix text selection issues on IE/Edge --- src/doc/DocAnnotator.js | 53 +++++++++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 20 deletions(-) diff --git a/src/doc/DocAnnotator.js b/src/doc/DocAnnotator.js index d589db7b6..dc7a8e278 100644 --- a/src/doc/DocAnnotator.js +++ b/src/doc/DocAnnotator.js @@ -410,10 +410,8 @@ class DocAnnotator extends Annotator { // Hide the create dialog if click was not in the popover if ( - !this.isCreatingHighlight && - // $FlowFixMe + this.createHighlightDialog && this.createHighlightDialog.isVisible && - // $FlowFixMe !this.createHighlightDialog.isInHighlight(mouseEvent) ) { mouseEvent.stopPropagation(); @@ -464,6 +462,8 @@ class DocAnnotator extends Annotator { if (this.highlighter) { this.highlighter.removeAllHighlights(); } + + this.mouseDownEvent = null; } /** @@ -598,7 +598,6 @@ class DocAnnotator extends Annotator { } let mouseEvent = event; - // $FlowFixMe if (this.hasTouch && event.targetTouches) { mouseEvent = event.targetTouches[0]; @@ -652,6 +651,7 @@ class DocAnnotator extends Annotator { * @return {void} */ highlightMousedownHandler = (event: Event) => { + const prevMouseDownEvent = this.mouseDownEvent; this.mouseDownEvent = event; // $FlowFixMe @@ -665,7 +665,9 @@ class DocAnnotator extends Annotator { } this.isCreatingHighlight = true; - this.resetHighlightSelection(this.mouseDownEvent); + if (this.hasMouseMoved(prevMouseDownEvent, event)) { + this.resetHighlightSelection(this.mouseEvent); + } if (this.plainHighlightEnabled) { this.modeControllers[TYPES.highlight].destroyPendingThreads(); @@ -696,6 +698,21 @@ class DocAnnotator extends Annotator { return isPending || this.createHighlightDialog.isVisible; } + /** + * Determines whether the mouse position has changed between the two provided mouse events + * @param {Event} prevEvent - Previous mouse event + * @param {Event} event - Current mouse event + * @return {boolean} Whether or not mouse has moved + */ + hasMouseMoved(prevEvent: Event, event: Event) { + if (!prevEvent || !event) { + return false; + } + + // $FlowFixMe + return prevEvent.clientX !== event.clientX || prevEvent.clientY !== event.clientY; + } + /** * Mouseup handler. Switches between creating a highlight and delegating * to highlight click handlers depending on whether mouse moved since @@ -705,8 +722,7 @@ class DocAnnotator extends Annotator { * @return {void} */ highlightMouseupHandler = (event: Event) => { - this.isCreatingHighlight = false; - + // this.isCreatingHighlight = false; if (util.isInAnnotationOrMarker(event, this.container)) { return; } @@ -716,25 +732,20 @@ class DocAnnotator extends Annotator { } let mouseUpEvent = event; - // $FlowFixMe if (this.hasTouch && event.targetTouches) { mouseUpEvent = event.targetTouches[0]; } - const { clientX, clientY } = this.mouseDownEvent; - const hasMouseMoved = - // $FlowFixMe - (clientX && clientX !== mouseUpEvent.clientX) || (clientY && clientY !== mouseUpEvent.clientY); - // Creating highlights is disabled on mobile for now since the // event we would listen to, selectionchange, fires continuously and // is unreliable. If the mouse moved or we double clicked text, // we trigger the create handler instead of the click handler - if ((this.createHighlightDialog && hasMouseMoved) || event.type === 'dblclick') { + if ( + (this.createHighlightDialog && this.hasMouseMoved(this.mouseDownEvent, mouseUpEvent)) || + event.type === 'dblclick' + ) { this.highlightCreateHandler(event); - } else { - this.resetHighlightSelection(event); } }; @@ -779,8 +790,12 @@ class DocAnnotator extends Annotator { return false; } - // $FlowFixMe - if (this.createHighlightDialog.isVisible) { + // Does nothing if the use is creating a highlight or has double clicked text + if ( + (this.createHighlightDialog && this.createHighlightDialog.isVisible) || + // $FlowFixMe + event.detail === 2 + ) { return true; } @@ -799,8 +814,6 @@ class DocAnnotator extends Annotator { commentThreads = this.modeControllers[TYPES.highlight_comment].getIntersectingThreads(event, location); } - this.hideAnnotations(event); - const intersectingThreads = [].concat(plainThreads, commentThreads); intersectingThreads.forEach((thread) => this.clickThread(event, thread)); From a49446df3604f62a249644d1f96a04f53a8759d1 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Wed, 9 Jan 2019 12:51:11 -0800 Subject: [PATCH 2/8] Chore: Cleanup --- src/doc/DocAnnotator.js | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/src/doc/DocAnnotator.js b/src/doc/DocAnnotator.js index dc7a8e278..4e7fde9f0 100644 --- a/src/doc/DocAnnotator.js +++ b/src/doc/DocAnnotator.js @@ -392,10 +392,10 @@ class DocAnnotator extends Annotator { /** * Handles click events when not in an annotation mode * - * @param {Event} event - Mouse event + * @param {MouseEvent} event - Mouse event * @return {void} */ - clickHandler = (event: Event) => { + clickHandler = (event: MouseEvent) => { let mouseEvent = event; // $FlowFixMe @@ -647,10 +647,10 @@ class DocAnnotator extends Annotator { * Mousedown handler on annotated element. Also delegates to mousedown * handler for each thread. * - * @param {Event} event DOM event + * @param {MouseEvent} event DOM event * @return {void} */ - highlightMousedownHandler = (event: Event) => { + highlightMousedownHandler = (event: MouseEvent) => { const prevMouseDownEvent = this.mouseDownEvent; this.mouseDownEvent = event; @@ -700,16 +700,15 @@ class DocAnnotator extends Annotator { /** * Determines whether the mouse position has changed between the two provided mouse events - * @param {Event} prevEvent - Previous mouse event - * @param {Event} event - Current mouse event + * @param {MouseEvent} prevEvent - Previous mouse event + * @param {MouseEvent} event - Current mouse event * @return {boolean} Whether or not mouse has moved */ - hasMouseMoved(prevEvent: Event, event: Event) { + hasMouseMoved(prevEvent: MouseEvent, event: MouseEvent) { if (!prevEvent || !event) { return false; } - // $FlowFixMe return prevEvent.clientX !== event.clientX || prevEvent.clientY !== event.clientY; } @@ -718,11 +717,10 @@ class DocAnnotator extends Annotator { * to highlight click handlers depending on whether mouse moved since * mousedown. * - * @param {Event} event DOM event + * @param {MouseEvent} event DOM event * @return {void} */ - highlightMouseupHandler = (event: Event) => { - // this.isCreatingHighlight = false; + highlightMouseupHandler = (event: MouseEvent) => { if (util.isInAnnotationOrMarker(event, this.container)) { return; } @@ -782,20 +780,16 @@ class DocAnnotator extends Annotator { * Highlight click handler. Delegates click event to click handlers for * threads on the page. * - * @param {Event} event DOM event - * @return {void} + * @param {MouseEvent} event DOM event + * @return {boolean} Whether or not mouse event was consumed */ - highlightClickHandler(event: Event) { + highlightClickHandler(event: MouseEvent) { if (!this.plainHighlightEnabled && !this.commentHighlightEnabled) { return false; } // Does nothing if the use is creating a highlight or has double clicked text - if ( - (this.createHighlightDialog && this.createHighlightDialog.isVisible) || - // $FlowFixMe - event.detail === 2 - ) { + if ((this.createHighlightDialog && this.createHighlightDialog.isVisible) || event.detail === 2) { return true; } From 5ea0bb6cf3d3c885fd7c7b80cf47bab86fd39171 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Fri, 11 Jan 2019 16:02:46 -0800 Subject: [PATCH 3/8] Fix: Prevent focus change from removing highlight selection --- src/AnnotationThread.js | 7 +++++- src/_common.scss | 7 ------ .../AnnotationPopover/AnnotationPopover.js | 6 ++--- .../AnnotationPopover/AnnotationPopover.scss | 5 ++-- src/components/AnnotationPopover/Overlay.js | 25 ------------------- src/doc/DocAnnotator.js | 24 ++++++++++-------- 6 files changed, 25 insertions(+), 49 deletions(-) delete mode 100644 src/components/AnnotationPopover/Overlay.js diff --git a/src/AnnotationThread.js b/src/AnnotationThread.js index a5e6a6802..c71ed0d25 100644 --- a/src/AnnotationThread.js +++ b/src/AnnotationThread.js @@ -7,6 +7,7 @@ import AnnotationAPI from './api/AnnotationAPI'; import * as util from './util'; import { ICON_PLACED_ANNOTATION } from './icons/icons'; import { + SELECTOR_ANNOTATION_POPOVER, CLASS_ANNOTATION_POINT_MARKER, CLASS_FLIPPED_POPOVER, DATA_TYPE_ANNOTATION_INDICATOR, @@ -179,6 +180,7 @@ class AnnotationThread extends EventEmitter { const isPending = this.state === STATES.pending; const pageEl = this.getPopoverParent(); + const popoverLayer = util.getPopoverLayer(pageEl); this.popoverComponent = render( , - util.getPopoverLayer(pageEl) + popoverLayer ); + + const popoverEl = popoverLayer.querySelector(SELECTOR_ANNOTATION_POPOVER); + popoverEl.scrollIntoView(); } /** diff --git a/src/_common.scss b/src/_common.scss index 44ec2ee9c..9a2ad2962 100644 --- a/src/_common.scss +++ b/src/_common.scss @@ -14,8 +14,6 @@ src: local('Lato Bold'), local('Lato-Bold'), url('https://cdn01.boxcdn.net/fonts/1.0.2/lato/Lato-Bold.woff2') format('woff2'), url('https://cdn01.boxcdn.net/fonts/1.0.2/lato/Lato-Bold.woff') format('woff'); } -@import '~box-react-ui/lib/styles/common/overlay'; - .ba { @include reset; @@ -96,11 +94,6 @@ @import '~box-react-ui/lib/styles/common/buttons'; } -.flyout-overlay { - @import '~box-react-ui/lib/styles/common/links'; - @import '~box-react-ui/lib/styles/common/buttons'; -} - /************************************** * Accessibility **************************************/ diff --git a/src/components/AnnotationPopover/AnnotationPopover.js b/src/components/AnnotationPopover/AnnotationPopover.js index 0068b94f4..0ab916f35 100644 --- a/src/components/AnnotationPopover/AnnotationPopover.js +++ b/src/components/AnnotationPopover/AnnotationPopover.js @@ -6,7 +6,6 @@ import PlainButton from 'box-react-ui/lib/components/plain-button'; import IconClose from 'box-react-ui/lib/icons/general/IconClose'; import Internationalize from '../Internationalize'; -import Overlay from './Overlay'; import CommentList from '../CommentList'; import { TYPES, CLASS_ANNOTATION_POPOVER, CLASS_ANNOTATION_CARET } from '../../constants'; @@ -19,6 +18,7 @@ const CLASS_ANIMATE_POPOVER = 'ba-animate-popover'; const CLASS_CREATE_POPOVER = 'ba-create-popover'; const CLASS_MOBILE_HEADER = 'ba-mobile-header'; const CLASS_MOBILE_CLOSE_BTN = 'ba-mobile-close-btn'; +const CLASS_POPOVER_OVERLAY = 'ba-popover-overlay'; type Props = { isMobile: boolean, @@ -99,7 +99,7 @@ class AnnotationPopover extends React.PureComponent { ) : ( )} - +
{hasComments ? ( ) : ( @@ -121,7 +121,7 @@ class AnnotationPopover extends React.PureComponent { onCommentClick={onCommentClick} /> )} - +
); diff --git a/src/components/AnnotationPopover/AnnotationPopover.scss b/src/components/AnnotationPopover/AnnotationPopover.scss index 5e7a20f62..bc307db92 100644 --- a/src/components/AnnotationPopover/AnnotationPopover.scss +++ b/src/components/AnnotationPopover/AnnotationPopover.scss @@ -40,7 +40,8 @@ } } - .overlay { + .ba-popover-overlay { + background: white; border: 1px solid $seesee; border-radius: 4px; display: block; @@ -50,7 +51,7 @@ white-space: normal; } - .ba-inline-popover .overlay { + .ba-inline-popover .ba-popover-overlay { align-items: center; display: inline-flex; } diff --git a/src/components/AnnotationPopover/Overlay.js b/src/components/AnnotationPopover/Overlay.js deleted file mode 100644 index 0bea9a533..000000000 --- a/src/components/AnnotationPopover/Overlay.js +++ /dev/null @@ -1,25 +0,0 @@ -// @flow -import * as React from 'react'; - -import OverlayComponent from 'box-react-ui/lib/components/flyout/Overlay'; - -const CLASS_POPOVER_OVERLAY = 'ba-popover-overlay'; - -type Props = { - shouldDefaultFocus: boolean, - children: React.Node -}; - -const Overlay = ({ shouldDefaultFocus, children }: Props) => { - if (shouldDefaultFocus) { - return ( - - {children} - - ); - } - - return
{children}
; -}; - -export default Overlay; diff --git a/src/doc/DocAnnotator.js b/src/doc/DocAnnotator.js index 4e7fde9f0..2506992c3 100644 --- a/src/doc/DocAnnotator.js +++ b/src/doc/DocAnnotator.js @@ -26,6 +26,7 @@ import { CLASS_ANNOTATION_POPOVER } from '../constants'; +const DOUBLE_CLICK_COUNT = 2; const SELECTION_TIMEOUT = 500; const CLASS_RANGY_HIGHLIGHT = 'rangy-highlight'; @@ -245,7 +246,7 @@ class DocAnnotator extends Annotator { /** @inheritdoc */ resetAnnotationUI(pageNum?: number) { // $FlowFixMe - document.getSelection().removeAllRanges(); + window.getSelection().removeAllRanges(); if (this.highlighter) { this.highlighter.removeAllHighlights(); } @@ -350,7 +351,6 @@ class DocAnnotator extends Annotator { document.addEventListener('selectionchange', this.onSelectionChange); } else { this.annotatedElement.addEventListener('mouseup', this.highlightMouseupHandler); - this.annotatedElement.addEventListener('dblclick', this.highlightMouseupHandler); this.annotatedElement.addEventListener('mousedown', this.highlightMousedownHandler); this.annotatedElement.addEventListener('contextmenu', this.highlightMousedownHandler); } @@ -383,7 +383,6 @@ class DocAnnotator extends Annotator { document.removeEventListener('selectionchange', this.onSelectionChange); } else { this.annotatedElement.removeEventListener('mouseup', this.highlightMouseupHandler); - this.annotatedElement.removeEventListener('dblclick', this.highlightMouseupHandler); this.annotatedElement.removeEventListener('mousedown', this.highlightMousedownHandler); this.annotatedElement.removeEventListener('contextmenu', this.highlightMousedownHandler); } @@ -410,6 +409,7 @@ class DocAnnotator extends Annotator { // Hide the create dialog if click was not in the popover if ( + this.hasMouseMoved(this.lastHighlightEvent, event) && this.createHighlightDialog && this.createHighlightDialog.isVisible && !this.createHighlightDialog.isInHighlight(mouseEvent) @@ -458,7 +458,7 @@ class DocAnnotator extends Annotator { this.hideCreateDialog(event); // $FlowFixMe - document.getSelection().removeAllRanges(); + window.getSelection().removeAllRanges(); if (this.highlighter) { this.highlighter.removeAllHighlights(); } @@ -739,10 +739,7 @@ class DocAnnotator extends Annotator { // event we would listen to, selectionchange, fires continuously and // is unreliable. If the mouse moved or we double clicked text, // we trigger the create handler instead of the click handler - if ( - (this.createHighlightDialog && this.hasMouseMoved(this.mouseDownEvent, mouseUpEvent)) || - event.type === 'dblclick' - ) { + if (this.createHighlightDialog && this.hasMouseMoved(this.mouseDownEvent, mouseUpEvent)) { this.highlightCreateHandler(event); } }; @@ -788,8 +785,14 @@ class DocAnnotator extends Annotator { return false; } - // Does nothing if the use is creating a highlight or has double clicked text - if ((this.createHighlightDialog && this.createHighlightDialog.isVisible) || event.detail === 2) { + // Does nothing if the use is creating a highlight + if (this.createHighlightDialog && this.createHighlightDialog.isVisible) { + return true; + } + + // Create a highlight if the user has double clicked + if (event.detail === DOUBLE_CLICK_COUNT) { + this.highlightCreateHandler(event); return true; } @@ -817,7 +820,6 @@ class DocAnnotator extends Annotator { return true; } - this.resetHighlightSelection(event); return false; } From 15767ddf95be9d04f2ca7b19c3b04687e3c56918 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Wed, 16 Jan 2019 13:20:07 -0800 Subject: [PATCH 4/8] Fix: Bail if click is outside selection/highlight --- src/doc/DocAnnotator.js | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/src/doc/DocAnnotator.js b/src/doc/DocAnnotator.js index 2506992c3..b0f63f7a6 100644 --- a/src/doc/DocAnnotator.js +++ b/src/doc/DocAnnotator.js @@ -462,8 +462,6 @@ class DocAnnotator extends Annotator { if (this.highlighter) { this.highlighter.removeAllHighlights(); } - - this.mouseDownEvent = null; } /** @@ -554,13 +552,7 @@ class DocAnnotator extends Annotator { this.selectionEndTimeout = null; } - // Bail if mid highlight and tapping on the screen - const isClickOutsideCreateDialog = this.isCreatingHighlight && util.isInDialog(event); - if (!docUtil.isValidSelection(selection) || isClickOutsideCreateDialog) { - this.lastHighlightEvent = null; - this.resetHighlightSelection(event); - return; - } + this.resetHighlightOnTap(event); // Do nothing if in a text area or mobile dialog or mobile create dialog is already open const pointController = this.modeControllers[TYPES.point]; @@ -773,6 +765,22 @@ class DocAnnotator extends Annotator { this.lastHighlightEvent = event; }; + /** + * Bail if mid highlight and click is outside highlight/selection + * + * @param {MouseEvent} event - Mouse event + * @return {void} + */ + + resetHighlightOnTap(event: MouseEvent) { + const selection = window.getSelection(); + const isClickOutsideCreateDialog = this.isCreatingHighlight && !util.isInDialog(event); + if (!docUtil.isValidSelection(selection) && isClickOutsideCreateDialog) { + this.lastHighlightEvent = null; + this.resetHighlightSelection(event); + } + } + /** * Highlight click handler. Delegates click event to click handlers for * threads on the page. @@ -785,8 +793,9 @@ class DocAnnotator extends Annotator { return false; } - // Does nothing if the use is creating a highlight + // Does nothing if the user is creating a highlight if (this.createHighlightDialog && this.createHighlightDialog.isVisible) { + this.resetHighlightOnTap(); return true; } From 82a22ae101186e5cc5ca0dfa7272beb9abe7d385 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Wed, 16 Jan 2019 13:21:17 -0800 Subject: [PATCH 5/8] Fix: Tests --- .../__tests__/Overlay-test.js | 18 ----------------- .../__snapshots__/Overlay-test.js.snap | 20 ------------------- src/doc/DocAnnotator.js | 12 +++++------ 3 files changed, 6 insertions(+), 44 deletions(-) delete mode 100644 src/components/AnnotationPopover/__tests__/Overlay-test.js delete mode 100644 src/components/AnnotationPopover/__tests__/__snapshots__/Overlay-test.js.snap diff --git a/src/components/AnnotationPopover/__tests__/Overlay-test.js b/src/components/AnnotationPopover/__tests__/Overlay-test.js deleted file mode 100644 index aa2cb5076..000000000 --- a/src/components/AnnotationPopover/__tests__/Overlay-test.js +++ /dev/null @@ -1,18 +0,0 @@ -import * as React from 'react'; -import { shallow } from 'enzyme'; - -import Overlay from '../Overlay'; - -describe('components/Overlay', () => { - test('should correctly render a Focus Trap if not on mobile', () => { - const wrapper = shallow().dive(); - expect(wrapper).toMatchSnapshot(); - expect(wrapper.find('FocusTrap').length).toEqual(1); - }); - - test('should correctly render a div without a Focus Trap if on mobile', () => { - const wrapper = shallow(); - expect(wrapper).toMatchSnapshot(); - expect(wrapper.find('FocusTrap').length).toEqual(0); - }); -}); diff --git a/src/components/AnnotationPopover/__tests__/__snapshots__/Overlay-test.js.snap b/src/components/AnnotationPopover/__tests__/__snapshots__/Overlay-test.js.snap deleted file mode 100644 index 421c7334c..000000000 --- a/src/components/AnnotationPopover/__tests__/__snapshots__/Overlay-test.js.snap +++ /dev/null @@ -1,20 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`components/Overlay should correctly render a Focus Trap if not on mobile 1`] = ` - -
- -`; - -exports[`components/Overlay should correctly render a div without a Focus Trap if on mobile 1`] = ` -
-`; diff --git a/src/doc/DocAnnotator.js b/src/doc/DocAnnotator.js index b0f63f7a6..46e96848b 100644 --- a/src/doc/DocAnnotator.js +++ b/src/doc/DocAnnotator.js @@ -409,6 +409,7 @@ class DocAnnotator extends Annotator { // Hide the create dialog if click was not in the popover if ( + // $FlowFixMe this.hasMouseMoved(this.lastHighlightEvent, event) && this.createHighlightDialog && this.createHighlightDialog.isVisible && @@ -552,7 +553,7 @@ class DocAnnotator extends Annotator { this.selectionEndTimeout = null; } - this.resetHighlightOnTap(event); + 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]; @@ -696,7 +697,7 @@ class DocAnnotator extends Annotator { * @param {MouseEvent} event - Current mouse event * @return {boolean} Whether or not mouse has moved */ - hasMouseMoved(prevEvent: MouseEvent, event: MouseEvent) { + hasMouseMoved(prevEvent: ?MouseEvent, event: ?MouseEvent) { if (!prevEvent || !event) { return false; } @@ -768,11 +769,10 @@ class DocAnnotator extends Annotator { /** * Bail if mid highlight and click is outside highlight/selection * - * @param {MouseEvent} event - Mouse event + * @param {Event} event - Mouse event * @return {void} */ - - resetHighlightOnTap(event: MouseEvent) { + resetHighlightOnOutsideClick(event: Event) { const selection = window.getSelection(); const isClickOutsideCreateDialog = this.isCreatingHighlight && !util.isInDialog(event); if (!docUtil.isValidSelection(selection) && isClickOutsideCreateDialog) { @@ -795,7 +795,7 @@ class DocAnnotator extends Annotator { // Does nothing if the user is creating a highlight if (this.createHighlightDialog && this.createHighlightDialog.isVisible) { - this.resetHighlightOnTap(); + this.resetHighlightOnOutsideClick(event); return true; } From 548278f444b7c5755e375161918b6c7f2632a05e Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Wed, 16 Jan 2019 16:47:47 -0800 Subject: [PATCH 6/8] Fix: Double click to highlight on Edge --- src/AnnotationThread.js | 4 +--- src/doc/DocAnnotator.js | 7 ++++++- src/doc/DocDrawingThread.js | 7 +++++++ src/doc/DocHighlightThread.js | 2 +- 4 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/AnnotationThread.js b/src/AnnotationThread.js index a0bd235fc..b274bcec2 100644 --- a/src/AnnotationThread.js +++ b/src/AnnotationThread.js @@ -7,7 +7,6 @@ import AnnotationAPI from './api/AnnotationAPI'; import * as util from './util'; import { ICON_PLACED_ANNOTATION } from './icons/icons'; import { - SELECTOR_ANNOTATION_POPOVER, CLASS_ANNOTATION_POINT_MARKER, CLASS_FLIPPED_POPOVER, DATA_TYPE_ANNOTATION_INDICATOR, @@ -204,8 +203,7 @@ class AnnotationThread extends EventEmitter { popoverLayer ); - const popoverEl = popoverLayer.querySelector(SELECTOR_ANNOTATION_POPOVER); - popoverEl.scrollIntoView(); + this.scrollIntoView(); } /** diff --git a/src/doc/DocAnnotator.js b/src/doc/DocAnnotator.js index 9398936ed..47b9feadc 100644 --- a/src/doc/DocAnnotator.js +++ b/src/doc/DocAnnotator.js @@ -836,13 +836,18 @@ class DocAnnotator extends Annotator { return false; } + /** @inheritdoc */ + toggleAnnotationMode(mode: AnnotationType) { + this.resetAnnotationUI(); + super.toggleAnnotationMode(mode); + } + /** @inheritdoc */ hideAnnotations(event: ?Event) { if (event && util.isInDialog(event, this.container)) { return; } - this.resetHighlightSelection(event); super.hideAnnotations(); } diff --git a/src/doc/DocDrawingThread.js b/src/doc/DocDrawingThread.js index 7badbe272..8ce8ec125 100644 --- a/src/doc/DocDrawingThread.js +++ b/src/doc/DocDrawingThread.js @@ -181,6 +181,13 @@ class DocDrawingThread extends DrawingThread { this.draw(context, false); } + /** + * Do nothing for drawing annotations + * + * @return {void} + */ + scrollIntoView() {} + /** @inheritdoc */ hide() { this.clearBoundary(); diff --git a/src/doc/DocHighlightThread.js b/src/doc/DocHighlightThread.js index bc587b535..c01d56b35 100644 --- a/src/doc/DocHighlightThread.js +++ b/src/doc/DocHighlightThread.js @@ -275,7 +275,7 @@ class DocHighlightThread extends AnnotationThread { const [yPos] = docUtil.getLowerRightCornerOfLastQuadPoint(this.location.quadPoints); // Adjust scroll to highlight position - this.adjustScroll(this.annotatedElement.scrollTop + yPos); + this.centerAnnotation(this.annotatedElement.scrollTop + yPos); } /** From 23ff1285b5c95ba4dbcf11e954dd29f54730fe31 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Thu, 17 Jan 2019 13:46:38 -0800 Subject: [PATCH 7/8] Fix: Tests --- src/__tests__/AnnotationThread-test.js | 18 +++ .../__tests__/AnnotationPopover-test.js | 14 +- .../AnnotationPopover-test.js.snap | 143 +++--------------- src/doc/DocAnnotator.js | 9 -- src/doc/__tests__/DocAnnotator-test.js | 133 ++++++++-------- 5 files changed, 104 insertions(+), 213 deletions(-) diff --git a/src/__tests__/AnnotationThread-test.js b/src/__tests__/AnnotationThread-test.js index e78dae169..28daab378 100644 --- a/src/__tests__/AnnotationThread-test.js +++ b/src/__tests__/AnnotationThread-test.js @@ -1,4 +1,5 @@ /* eslint-disable no-unused-expressions */ +import * as ReactDOM from 'react-dom'; import AnnotationThread from '../AnnotationThread'; import * as util from '../util'; import { @@ -117,6 +118,23 @@ describe('AnnotationThread', () => { }); }); + describe('renderAnnotationPopover()', () => { + beforeEach(() => { + thread.getPopoverParent = jest.fn().mockReturnValue(rootElement); + util.getPopoverLayer = jest.fn().mockReturnValue(rootElement); + util.shouldDisplayMobileUI = jest.fn().mockReturnValue(false); + thread.scrollIntoView = jest.fn(); + ReactDOM.render = jest.fn(); + thread.position = jest.fn(); + }); + + it('should render and display the popover for this annotation', () => { + thread.renderAnnotationPopover(); + expect(thread.popoverComponent).not.toBeUndefined(); + expect(thread.scrollIntoView).toBeCalled(); + }); + }); + describe('hide()', () => { it('should hide the thread element', () => { thread.unmountPopover = jest.fn(); diff --git a/src/components/AnnotationPopover/__tests__/AnnotationPopover-test.js b/src/components/AnnotationPopover/__tests__/AnnotationPopover-test.js index 5b6aa52ff..fe7101505 100644 --- a/src/components/AnnotationPopover/__tests__/AnnotationPopover-test.js +++ b/src/components/AnnotationPopover/__tests__/AnnotationPopover-test.js @@ -73,24 +73,14 @@ describe('components/AnnotationPopover', () => { expect(wrapper.find('.ba-inline-popover').length).toEqual(1); }); - test('should correctly render a BRUI Overlay if not on mobile', () => { - const wrapper = render({ - canAnnotate: true, - comments, - isMobile: false - }); - expect(wrapper).toMatchSnapshot(); - expect(wrapper.find('Overlay').prop('shouldDefaultFocus')).toBeTruthy(); - }); - - test('should correctly render a div without a Focus Trap if on mobile', () => { + test('should correctly render a div without a Focus Trap', () => { const wrapper = render({ canAnnotate: true, comments, isMobile: true }); expect(wrapper).toMatchSnapshot(); - expect(wrapper.find('Overlay').prop('shouldDefaultFocus')).toBeFalsy(); + expect(wrapper.find('Overlay').exists()).toBeFalsy(); }); test('should correctly render a popover with comments and reply textarea', () => { diff --git a/src/components/AnnotationPopover/__tests__/__snapshots__/AnnotationPopover-test.js.snap b/src/components/AnnotationPopover/__tests__/__snapshots__/AnnotationPopover-test.js.snap index b58a8ef0e..ee816424b 100644 --- a/src/components/AnnotationPopover/__tests__/__snapshots__/AnnotationPopover-test.js.snap +++ b/src/components/AnnotationPopover/__tests__/__snapshots__/AnnotationPopover-test.js.snap @@ -1,111 +1,6 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`components/AnnotationPopover should correctly render a BRUI Overlay if not on mobile 1`] = ` - -
- - - - - -
-
-`; - -exports[`components/AnnotationPopover should correctly render a div without a Focus Trap if on mobile 1`] = ` +exports[`components/AnnotationPopover should correctly render a div without a Focus Trap 1`] = `
- - +
`; @@ -245,8 +140,8 @@ exports[`components/AnnotationPopover should correctly render a pending annotati - - +
`; @@ -310,8 +205,8 @@ exports[`components/AnnotationPopover should correctly render a popover with com - - + `; @@ -415,8 +310,8 @@ exports[`components/AnnotationPopover should correctly render a view-only popove - - + `; @@ -474,8 +369,8 @@ exports[`components/AnnotationPopover should correctly render an annotation with - - + `; @@ -541,14 +436,14 @@ exports[`components/AnnotationPopover should render a view-only annotation with - - + `; diff --git a/src/doc/DocAnnotator.js b/src/doc/DocAnnotator.js index 47b9feadc..90271a4d3 100644 --- a/src/doc/DocAnnotator.js +++ b/src/doc/DocAnnotator.js @@ -842,15 +842,6 @@ class DocAnnotator extends Annotator { super.toggleAnnotationMode(mode); } - /** @inheritdoc */ - hideAnnotations(event: ?Event) { - if (event && util.isInDialog(event, this.container)) { - return; - } - - super.hideAnnotations(); - } - /** * Delegates click event to click handlers for threads on the page. * diff --git a/src/doc/__tests__/DocAnnotator-test.js b/src/doc/__tests__/DocAnnotator-test.js index ecdefadb5..2170fb8ca 100644 --- a/src/doc/__tests__/DocAnnotator-test.js +++ b/src/doc/__tests__/DocAnnotator-test.js @@ -411,7 +411,7 @@ describe('doc/DocAnnotator', () => { describe('resetAnnotationUI()', () => { beforeEach(() => { - document.getSelection = jest.fn().mockReturnValue({ + window.getSelection = jest.fn().mockReturnValue({ removeAllRanges: jest.fn() }); annotator.highlighter = { @@ -531,10 +531,6 @@ describe('doc/DocAnnotator', () => { annotator.highlightMouseupHandler ); expect(annotator.annotatedElement.addEventListener).toBeCalledWith('wheel', annotator.hideCreateDialog); - expect(annotator.annotatedElement.addEventListener).toBeCalledWith( - 'dblclick', - annotator.highlightMouseupHandler - ); expect(annotator.annotatedElement.addEventListener).toBeCalledWith( 'mousedown', annotator.highlightMousedownHandler @@ -601,10 +597,6 @@ describe('doc/DocAnnotator', () => { 'mouseup', annotator.highlightMouseupHandler ); - expect(annotator.annotatedElement.addEventListener).not.toBeCalledWith( - 'dblclick', - annotator.highlightMouseupHandler - ); expect(annotator.annotatedElement.addEventListener).not.toBeCalledWith( 'mousedown', annotator.highlightMousedownHandler @@ -648,10 +640,6 @@ describe('doc/DocAnnotator', () => { 'touchend', annotator.hideCreateDialog ); - expect(annotator.annotatedElement.removeEventListener).toBeCalledWith( - 'dblclick', - annotator.highlightMouseupHandler - ); expect(annotator.annotatedElement.removeEventListener).toBeCalledWith( 'mousedown', annotator.highlightMousedownHandler @@ -697,7 +685,7 @@ describe('doc/DocAnnotator', () => { const selection = { removeAllRanges: jest.fn() }; - document.getSelection = jest.fn().mockReturnValue(selection); + window.getSelection = jest.fn().mockReturnValue(selection); annotator.hideCreateDialog = jest.fn(); annotator.resetHighlightSelection({}); @@ -735,6 +723,7 @@ describe('doc/DocAnnotator', () => { }; annotator.resetHighlightSelection = jest.fn(); util.isInAnnotationOrMarker = jest.fn().mockReturnValue(false); + annotator.hasMouseMoved = jest.fn().mockReturnValue(true); }); it('should do nothing if event occured inside an annotation or marker', () => { @@ -750,6 +739,12 @@ describe('doc/DocAnnotator', () => { expect(controller.destroyPendingThreads).not.toBeCalled(); }); + it('should not reset highlights if the user is double clicking', () => { + annotator.hasMouseMoved = jest.fn().mockReturnValue(false); + annotator.highlightMousedownHandler({ clientX: 1, clientY: 1 }); + expect(annotator.resetHighlightSelection).not.toBeCalled(); + }); + it('should get highlights on page and call their onMouse down method', () => { annotator.plainHighlightEnabled = true; annotator.highlightMousedownHandler({ clientX: 1, clientY: 1 }); @@ -758,29 +753,42 @@ describe('doc/DocAnnotator', () => { }); }); + describe('hasMouseMoved()', () => { + let prevEvent; + let currEvent; + + beforeEach(() => { + prevEvent = { clientX: 1, clientY: 1 }; + currEvent = { clientX: 1, clientY: 1 }; + }); + + it('should return false if either a previous OR current event is non-existent', () => { + expect(annotator.hasMouseMoved(prevEvent, null)).toBeFalsy(); + expect(annotator.hasMouseMoved(null, currEvent)).toBeFalsy(); + }); + + it('should return true if the mouse has moved', () => { + expect(annotator.hasMouseMoved(prevEvent, currEvent)).toBeFalsy(); + + currEvent.clientY = 2; + expect(annotator.hasMouseMoved(prevEvent, currEvent)).toBeTruthy(); + }); + }); + describe('highlightMouseupHandler()', () => { beforeEach(() => { annotator.highlightCreateHandler = jest.fn(); - annotator.resetHighlightSelection = jest.fn(); annotator.mouseDownEvent = { clientX: 100, clientY: 100 }; }); it('should call highlightCreateHandler if on desktop and the mouse moved', () => { annotator.highlightMouseupHandler({ x: 0, y: 0 }); expect(annotator.highlightCreateHandler).toBeCalled(); - expect(annotator.resetHighlightSelection).not.toBeCalled(); }); it('should call highlightCreateHandler if on desktop and the user double clicked', () => { annotator.highlightMouseupHandler({ type: 'dblclick' }); expect(annotator.highlightCreateHandler).toBeCalled(); - expect(annotator.resetHighlightSelection).not.toBeCalled(); - }); - - it('should reset highlight selection when mouse has not moved', () => { - annotator.highlightMouseupHandler(annotator.mouseDownEvent); - expect(annotator.highlightCreateHandler).not.toBeCalled(); - expect(annotator.resetHighlightSelection).toBeCalled(); }); it('should call highlighter.removeAllHighlghts', () => { @@ -807,15 +815,17 @@ describe('doc/DocAnnotator', () => { }; docUtil.isValidSelection = jest.fn().mockReturnValue(true); + docUtil.hasSelectionChanged = jest.fn().mockReturnValue(true); annotator.lastSelection = {}; annotator.highlighter = { removeAllHighlights: jest.fn() }; - annotator.resetHighlightSelection = jest.fn(); + annotator.resetHighlightOnOutsideClick = jest.fn(); window.getSelection = jest.fn().mockReturnValue({ anchorNode: { parentNode: 'Node' - } + }, + getRangeAt: jest.fn() }); util.getPageInfo = jest.fn().mockReturnValue({ page: 1 }); util.findClosestElWithClass = jest.fn().mockReturnValue(null); @@ -831,26 +841,22 @@ describe('doc/DocAnnotator', () => { }); it('should reset the selectionEndTimeout', () => { - annotator.selectionEndTimeout = 1; - docUtil.isValidSelection = jest.fn().mockReturnValue(false); + annotator.selectionEndTimeout = 123; annotator.onSelectionChange(event); - expect(annotator.selectionEndTimeout).toBeNull(); + expect(annotator.selectionEndTimeout).not.toEqual(123); }); it('should clear selection if the user is currently creating a highlight annotation', () => { annotator.isCreatingHighlight = true; util.isInDialog = jest.fn().mockReturnValue(true); annotator.onSelectionChange(event); - expect(annotator.resetHighlightSelection).toBeCalled(); + expect(annotator.resetHighlightOnOutsideClick).toBeCalled(); }); it('should clear out highlights and exit "annotation creation" mode if an invalid selection', () => { - annotator.lastHighlightEvent = event; docUtil.isValidSelection = jest.fn().mockReturnValue(false); - annotator.onSelectionChange(event); - expect(annotator.lastHighlightEvent).toBeNull(); - expect(annotator.resetHighlightSelection).toBeCalled(); + expect(annotator.resetHighlightOnOutsideClick).toBeCalled(); }); it('should do nothing the the user is currently creating a point annotation', () => { @@ -986,6 +992,8 @@ describe('doc/DocAnnotator', () => { annotator.clickThread = jest.fn(); annotator.hideAnnotations = jest.fn(); annotator.resetHighlightSelection = jest.fn(); + annotator.resetHighlightOnOutsideClick = jest.fn(); + annotator.highlightCreateHandler = jest.fn(); annotator.modeControllers = { highlight: controller }; @@ -994,26 +1002,33 @@ describe('doc/DocAnnotator', () => { }; annotator.activeThread = undefined; annotator.plainHighlightEnabled = false; - annotator.commentHighlightEnabled = false; + annotator.createHighlightDialog.isVisible = false; + annotator.createHighlightDialog = { isVisible: false }; }); it('should do nothing and return false if no highlight types are enabled', () => { expect(annotator.highlightClickHandler(event)).toBeFalsy(); - expect(annotator.hideAnnotations).not.toBeCalled(); }); - it('should do nothing and return true if a createHighlightDialog is visible', () => { + it('should do nothing if the user is creating a highlight and resets on outside click when necessary', () => { annotator.plainHighlightEnabled = true; annotator.createHighlightDialog.isVisible = true; expect(annotator.highlightClickHandler(event)).toBeTruthy(); - expect(annotator.hideAnnotations).not.toBeCalled(); + expect(annotator.resetHighlightOnOutsideClick).toBeCalled(); + }); + + it('should create a highlight if the user has double clicked', () => { + annotator.plainHighlightEnabled = true; + expect(annotator.highlightClickHandler(event)).toBeFalsy(); + event.detail = 2; + expect(annotator.highlightClickHandler(event)).toBeTruthy(); + expect(annotator.highlightCreateHandler).toBeCalledTimes(1); }); it('should find the active plain highlight', () => { annotator.plainHighlightEnabled = true; - annotator.highlightClickHandler(event); + expect(annotator.highlightClickHandler(event)).toBeFalsy(); expect(controller.getIntersectingThreads).toBeCalled(); - expect(annotator.hideAnnotations).toBeCalled(); expect(thread.show).not.toBeCalled(); }); @@ -1022,9 +1037,9 @@ describe('doc/DocAnnotator', () => { 'highlight-comment': controller }; annotator.commentHighlightEnabled = true; - annotator.highlightClickHandler(event); + annotator.activeThread = thread; + expect(annotator.highlightClickHandler(event)).toBeFalsy(); expect(controller.getIntersectingThreads).toBeCalled(); - expect(annotator.hideAnnotations).toBeCalled(); expect(thread.show).not.toBeCalled(); }); @@ -1033,8 +1048,7 @@ describe('doc/DocAnnotator', () => { controller.getIntersectingThreads = jest.fn(() => { annotator.activeThread = thread; }); - annotator.highlightClickHandler(event); - expect(annotator.hideAnnotations).toBeCalled(); + expect(annotator.highlightClickHandler(event)).toBeTruthy(); expect(thread.show).toBeCalled(); }); @@ -1043,36 +1057,19 @@ describe('doc/DocAnnotator', () => { util.shouldDisplayMobileUI = jest.fn().mockReturnValue(true); annotator.getLocationFromEvent = jest.fn().mockReturnValue({}); - annotator.highlightClickHandler(event); - expect(annotator.hideAnnotations).toBeCalled(); + expect(annotator.highlightClickHandler(event)).toBeFalsy(); expect(thread.show).not.toBeCalled(); }); - - it('should reset highlight selection if not on mobile and no active threads exist', () => { - annotator.plainHighlightEnabled = true; - document.getSelection = jest.fn().mockReturnValue({ - removeAllRanges: jest.fn() - }); - - annotator.highlightClickHandler(event); - expect(annotator.resetHighlightSelection).toBeCalled(); - }); }); - describe('hideAnnotations()', () => { - beforeEach(() => { - annotator.resetHighlightSelection = jest.fn(); - util.isInDialog = jest.fn().mockReturnValue(false); - }); + describe('toggleAnnotationMode()', () => { + it('should reset the annotation UI and toggle the annotation mode', () => { + annotator.modeControllers.something = controller; + annotator.hideAnnotations = jest.fn(); + annotator.resetAnnotationUI = jest.fn(); - it('should reset the highlight selection', () => { - util.isInDialog = jest.fn().mockReturnValue(true); - annotator.hideAnnotations({}); - expect(annotator.resetHighlightSelection).not.toBeCalled(); - }); - it('should reset the highlight selection', () => { - annotator.hideAnnotations({}); - expect(annotator.resetHighlightSelection).toBeCalled(); + annotator.toggleAnnotationMode('something'); + expect(annotator.resetAnnotationUI).toBeCalled(); }); }); From 001fbf43f83b85ce9744699b71f513dd4c288a95 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Thu, 17 Jan 2019 15:06:11 -0800 Subject: [PATCH 8/8] Chore: Remove thread.scrollIntoView() call on render --- src/AnnotationThread.js | 5 +---- src/__tests__/AnnotationThread-test.js | 6 +----- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/src/AnnotationThread.js b/src/AnnotationThread.js index b274bcec2..6e884c639 100644 --- a/src/AnnotationThread.js +++ b/src/AnnotationThread.js @@ -179,7 +179,6 @@ class AnnotationThread extends EventEmitter { const isPending = this.state === STATES.pending; const pageEl = this.getPopoverParent(); - const popoverLayer = util.getPopoverLayer(pageEl); this.popoverComponent = render( , - popoverLayer + util.getPopoverLayer(pageEl) ); - - this.scrollIntoView(); } /** diff --git a/src/__tests__/AnnotationThread-test.js b/src/__tests__/AnnotationThread-test.js index 28daab378..e65166fcf 100644 --- a/src/__tests__/AnnotationThread-test.js +++ b/src/__tests__/AnnotationThread-test.js @@ -119,19 +119,15 @@ describe('AnnotationThread', () => { }); describe('renderAnnotationPopover()', () => { - beforeEach(() => { + it('should render and display the popover for this annotation', () => { thread.getPopoverParent = jest.fn().mockReturnValue(rootElement); util.getPopoverLayer = jest.fn().mockReturnValue(rootElement); util.shouldDisplayMobileUI = jest.fn().mockReturnValue(false); - thread.scrollIntoView = jest.fn(); ReactDOM.render = jest.fn(); thread.position = jest.fn(); - }); - it('should render and display the popover for this annotation', () => { thread.renderAnnotationPopover(); expect(thread.popoverComponent).not.toBeUndefined(); - expect(thread.scrollIntoView).toBeCalled(); }); });