From 3c74c63877f726ba6eddc4bf44c3057ea1f4fe3e Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Wed, 31 Oct 2018 15:21:32 -0700 Subject: [PATCH 1/7] Fix: highlighting on mobile --- .../ActionControls/ActionControls.scss | 1 + src/doc/DocAnnotator.js | 26 ++++++++++--------- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/src/components/ActionControls/ActionControls.scss b/src/components/ActionControls/ActionControls.scss index 8edb65eff..3000bfdaa 100644 --- a/src/components/ActionControls/ActionControls.scss +++ b/src/components/ActionControls/ActionControls.scss @@ -41,6 +41,7 @@ .ba-annotation-label ~ .ba-action-controls .ba-action-controls-draw, .ba-annotation-label ~ .ba-action-controls .ba-action-controls-highlight { border-left: 0; + margin: 0; } } diff --git a/src/doc/DocAnnotator.js b/src/doc/DocAnnotator.js index 9010bc3f2..6a4726988 100644 --- a/src/doc/DocAnnotator.js +++ b/src/doc/DocAnnotator.js @@ -74,6 +74,8 @@ class DocAnnotator extends Annotator { this.createHighlightThread = this.createHighlightThread.bind(this); // $FlowFixMe this.createPlainHighlight = this.createPlainHighlight.bind(this); + // $FlowFixMe + this.onSelectionChange = this.onSelectionChange.bind(this); } /** @@ -133,7 +135,7 @@ class DocAnnotator extends Annotator { if (annotationType === TYPES.point) { let clientEvent = event; - + // $FlowFixMe if (this.hasTouch && event.targetTouches) { if (event.targetTouches.length <= 0) { @@ -383,7 +385,7 @@ class DocAnnotator extends Annotator { /** * Handles click events when not in an annotation mode - * + * * @param {Event} event - Mouse event * @return {void} */ @@ -428,9 +430,9 @@ class DocAnnotator extends Annotator { }; /** - * Hides the create highlight dialog - * - * @param {Event} event - Mouse event + * Hides the create highlight dialog + * + * @param {Event} event - Mouse event * @return {void} */ hideCreateDialog(event: ?Event) { @@ -523,7 +525,7 @@ class DocAnnotator extends Annotator { * @param {Event} event The DOM event coming from interacting with the element. * @return {void} */ - onSelectionChange = (event: Event) => { + onSelectionChange(event: Event) { event.preventDefault(); event.stopPropagation(); @@ -576,14 +578,14 @@ class DocAnnotator extends Annotator { } let mouseEvent = event; - + // $FlowFixMe if (this.hasTouch && event.targetTouches) { mouseEvent = event.targetTouches[0]; } this.lastHighlightEvent = mouseEvent; this.lastSelection = selection; - }; + } /** * Mode controllers setup. @@ -631,7 +633,7 @@ class DocAnnotator extends Annotator { */ highlightMousedownHandler = (event: Event) => { this.mouseDownEvent = event; - + // $FlowFixMe if (this.hasTouch && event.targetTouches) { this.mouseDownEvent = event.targetTouches[0]; @@ -668,7 +670,7 @@ class DocAnnotator extends Annotator { } return isPending; }); - + // $FlowFixMe return isPending || this.createHighlightDialog.isVisible; } @@ -693,7 +695,7 @@ class DocAnnotator extends Annotator { } let mouseUpEvent = event; - + // $FlowFixMe if (this.hasTouch && event.targetTouches) { mouseUpEvent = event.targetTouches[0]; @@ -753,7 +755,7 @@ class DocAnnotator extends Annotator { if (!this.plainHighlightEnabled && !this.commentHighlightEnabled) { return false; } - + // $FlowFixMe if (this.createHighlightDialog.isVisible) { return true; From 274ecfecf131fcb76c53e153eb4acd7b6e3dce80 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Thu, 1 Nov 2018 15:06:29 -0700 Subject: [PATCH 2/7] Fix: Remove focus trap on AnnotationPopover on mobile --- src/Annotator.scss | 3 ++- .../ActionControls/ActionControls.scss | 10 ++++---- .../AnnotationPopover/AnnotationPopover.js | 5 ++-- .../AnnotationPopover/AnnotatorLabel.js | 2 +- .../AnnotationPopover/AnnotatorLabel.scss | 6 +++-- src/components/AnnotationPopover/Overlay.js | 23 +++++++++++++++++++ src/components/CommentList/CommentList.scss | 7 +++++- src/doc/CreateHighlightDialog.js | 7 ++++-- src/doc/DocAnnotator.js | 19 +++++++++------ src/doc/DocHighlightThread.js | 5 ++-- 10 files changed, 62 insertions(+), 25 deletions(-) create mode 100644 src/components/AnnotationPopover/Overlay.js diff --git a/src/Annotator.scss b/src/Annotator.scss index 353a76c7f..5d5366f0f 100644 --- a/src/Annotator.scss +++ b/src/Annotator.scss @@ -168,7 +168,8 @@ // Quad point positioning - the helper divs are positioned relative to the Rangy-created element .bp-doc .rangy-highlight { - background-color: #ff0; + background-color: lighten($highlight-yellow, 10%); + opacity: 25%; position: relative; } diff --git a/src/components/ActionControls/ActionControls.scss b/src/components/ActionControls/ActionControls.scss index 3000bfdaa..73518ba47 100644 --- a/src/components/ActionControls/ActionControls.scss +++ b/src/components/ActionControls/ActionControls.scss @@ -21,14 +21,14 @@ margin: 0; } - .ba-annotation-label ~ .ba-action-controls .ba-action-controls-draw, - .ba-annotation-label ~ .ba-action-controls .ba-action-controls-highlight { + .ba-annotator-label ~ .ba-action-controls .ba-action-controls-draw, + .ba-annotator-label ~ .ba-action-controls .ba-action-controls-highlight { border-left: 1px solid $see-see; margin-left: 5px; } // TABLET-SPECIFIC CSS - @media #{$tablet} { + @media #{$mobile}, #{$tablet} { .ba-create-popover .ba-action-controls { border: none; box-shadow: none; @@ -38,8 +38,8 @@ width: 100%; } - .ba-annotation-label ~ .ba-action-controls .ba-action-controls-draw, - .ba-annotation-label ~ .ba-action-controls .ba-action-controls-highlight { + .ba-annotator-label ~ .ba-action-controls .ba-action-controls-draw, + .ba-annotator-label ~ .ba-action-controls .ba-action-controls-highlight { border-left: 0; margin: 0; } diff --git a/src/components/AnnotationPopover/AnnotationPopover.js b/src/components/AnnotationPopover/AnnotationPopover.js index 8111f0979..1383f68f8 100644 --- a/src/components/AnnotationPopover/AnnotationPopover.js +++ b/src/components/AnnotationPopover/AnnotationPopover.js @@ -2,11 +2,11 @@ import React from 'react'; import classNames from 'classnames'; import noop from 'lodash/noop'; -import Overlay from 'box-react-ui/lib/components/flyout/Overlay'; 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 } from '../../constants'; @@ -93,8 +93,7 @@ class AnnotationPopover extends React.PureComponent { ) : ( )} - - + {hasComments ? ( ) : ( diff --git a/src/components/AnnotationPopover/AnnotatorLabel.js b/src/components/AnnotationPopover/AnnotatorLabel.js index 87dc0e7b9..8c2805c54 100644 --- a/src/components/AnnotationPopover/AnnotatorLabel.js +++ b/src/components/AnnotationPopover/AnnotatorLabel.js @@ -42,7 +42,7 @@ class AnnotatorLabel extends React.PureComponent { const { id, isPending } = this.props; return ( !isPending && ( - + ) diff --git a/src/components/AnnotationPopover/AnnotatorLabel.scss b/src/components/AnnotationPopover/AnnotatorLabel.scss index 071522775..b6bf83f91 100644 --- a/src/components/AnnotationPopover/AnnotatorLabel.scss +++ b/src/components/AnnotationPopover/AnnotatorLabel.scss @@ -1,15 +1,17 @@ @import '../../variables'; .ba { - .ba-annotation-label { + .ba-annotator-label { line-height: 23px; } // MOBILE & TABLET CSS @media #{$mobile}, #{$tablet} { - .ba-annotation-label { + .ba-annotator-label { background: white; border-bottom: 1px solid #ccc; + display: flex; + justify-content: center; left: 0; padding: 20px; position: fixed; diff --git a/src/components/AnnotationPopover/Overlay.js b/src/components/AnnotationPopover/Overlay.js new file mode 100644 index 000000000..121ca6924 --- /dev/null +++ b/src/components/AnnotationPopover/Overlay.js @@ -0,0 +1,23 @@ +// @flow +import * as React from 'react'; + +import OverlayComponent from 'box-react-ui/lib/components/flyout/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/components/CommentList/CommentList.scss b/src/components/CommentList/CommentList.scss index 2bddeabd3..1fd997f58 100644 --- a/src/components/CommentList/CommentList.scss +++ b/src/components/CommentList/CommentList.scss @@ -22,10 +22,15 @@ padding-top: 15px; } - .bcs-comment { + .bcs-comment, + .bcs-comment-user-name ~ .bcs-comment-created-at { margin: 0; } + .bcs-comment-content { + text-align: left; + } + // MOBILE CSS @media #{$mobile}, #{$tablet} { .ba-comment-list { diff --git a/src/doc/CreateHighlightDialog.js b/src/doc/CreateHighlightDialog.js index 6b860905b..35be6c9a9 100644 --- a/src/doc/CreateHighlightDialog.js +++ b/src/doc/CreateHighlightDialog.js @@ -90,11 +90,11 @@ class CreateHighlightDialog extends EventEmitter { /** * Render the popover * - * @param {HTMLElement} selection Current text selection + * @param {Selection} selection Current text selection * @param {AnnotationType} type - highlight type * @return {void} */ - show(selection: HTMLElement, type: AnnotationType = TYPES.highlight) { + show(selection: ?Selection, type: AnnotationType = TYPES.highlight) { if (!selection) { return; } @@ -265,6 +265,9 @@ class CreateHighlightDialog extends EventEmitter { onCommentClick = () => { this.emit(CREATE_EVENT.comment); this.renderAnnotationPopover(TYPES.highlight_comment); + + // $FlowFixMe + document.getSelection().removeAllRanges(); }; /** diff --git a/src/doc/DocAnnotator.js b/src/doc/DocAnnotator.js index 6a4726988..52556d634 100644 --- a/src/doc/DocAnnotator.js +++ b/src/doc/DocAnnotator.js @@ -50,7 +50,7 @@ class DocAnnotator extends Annotator { lastHighlightEvent: ?Event; /** @property {Selection} - For tracking diffs in text selection, for mobile highlights creation. */ - lastSelection: ?HTMLElement; + lastSelection: ?Selection; /** @property {boolean} - True if regular highlights are allowed to be read/written */ plainHighlightEnabled: boolean; @@ -76,6 +76,8 @@ class DocAnnotator extends Annotator { this.createPlainHighlight = this.createPlainHighlight.bind(this); // $FlowFixMe this.onSelectionChange = this.onSelectionChange.bind(this); + // $FlowFixMe + this.selectionTimeoutHandler = this.selectionTimeoutHandler.bind(this); } /** @@ -440,6 +442,7 @@ class DocAnnotator extends Annotator { return; } + // $FlowFixMe this.createHighlightDialog.unmountPopover(); } @@ -560,11 +563,7 @@ class DocAnnotator extends Annotator { return; } - this.selectionEndTimeout = setTimeout(() => { - if (this.createHighlightDialog && !this.createHighlightDialog.isVisible) { - this.createHighlightDialog.show(selection); - } - }, SELECTION_TIMEOUT); + this.selectionEndTimeout = setTimeout(this.selectionTimeoutHandler, SELECTION_TIMEOUT); const { page } = util.getPageInfo(event.target); @@ -587,6 +586,12 @@ class DocAnnotator extends Annotator { this.lastSelection = selection; } + selectionTimeoutHandler = () => { + if (this.createHighlightDialog && !this.createHighlightDialog.isVisible) { + this.createHighlightDialog.show(this.lastSelection); + } + }; + /** * Mode controllers setup. * @@ -619,7 +624,7 @@ class DocAnnotator extends Annotator { return; } - this.highlighter.highlightSelection('rangy-highlight', { + this.highlighter.highlightSelection(CLASS_RANGY_HIGHLIGHT, { containerElementId: this.annotatedElement.id }); } diff --git a/src/doc/DocHighlightThread.js b/src/doc/DocHighlightThread.js index 2a923cd21..dcf101897 100644 --- a/src/doc/DocHighlightThread.js +++ b/src/doc/DocHighlightThread.js @@ -124,11 +124,10 @@ class DocHighlightThread extends AnnotationThread { delete(annotation: Object, useServer: boolean = true): Promise { const promise = super.delete(annotation, useServer); - if (!this.threadID) { - return promise; + if (this.threadID) { + this.renderAnnotationPopover(); } - this.renderAnnotationPopover(); return promise; } From 286c5ef5e0458b7053f36c8297d8dd1ae4dbef1c Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Thu, 1 Nov 2018 17:55:58 -0700 Subject: [PATCH 3/7] Chore: Allow clicking on other highlights when createHighlightDialog is open --- src/doc/DocAnnotator.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/doc/DocAnnotator.js b/src/doc/DocAnnotator.js index 52556d634..8d3b03cc8 100644 --- a/src/doc/DocAnnotator.js +++ b/src/doc/DocAnnotator.js @@ -717,6 +717,8 @@ class DocAnnotator extends Annotator { // we trigger the create handler instead of the click handler if ((this.createHighlightDialog && hasMouseMoved) || event.type === 'dblclick') { this.highlightCreateHandler(event); + } else if (!hasMouseMoved) { + this.clickHandler(event); } }; From fdf7e07cbad9aa38f78763045b4caf208b89440a Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Fri, 2 Nov 2018 11:54:34 -0700 Subject: [PATCH 4/7] Chore: Add Snapshot tests --- .../__tests__/AnnotationPopover-test.js | 20 ++ .../__tests__/Overlay-test.js | 18 ++ .../AnnotationPopover-test.js.snap | 247 +++++++++++++++++- .../__snapshots__/Overlay-test.js.snap | 20 ++ .../__tests__/CreateHighlightDialog-test.js | 3 + 5 files changed, 303 insertions(+), 5 deletions(-) create mode 100644 src/components/AnnotationPopover/__tests__/Overlay-test.js create mode 100644 src/components/AnnotationPopover/__tests__/__snapshots__/Overlay-test.js.snap diff --git a/src/components/AnnotationPopover/__tests__/AnnotationPopover-test.js b/src/components/AnnotationPopover/__tests__/AnnotationPopover-test.js index 8112d6799..b12c0ffa0 100644 --- a/src/components/AnnotationPopover/__tests__/AnnotationPopover-test.js +++ b/src/components/AnnotationPopover/__tests__/AnnotationPopover-test.js @@ -73,6 +73,26 @@ describe('components/AnnotationPopover', () => { expect(wrapper.find('.ba-inline').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', () => { + const wrapper = render({ + canAnnotate: true, + comments, + isMobile: true + }); + expect(wrapper).toMatchSnapshot(); + expect(wrapper.find('Overlay').prop('shouldDefaultFocus')).toBeFalsy(); + }); + test('should correctly render a popover with comments and reply textarea', () => { const wrapper = render({ canAnnotate: true, diff --git a/src/components/AnnotationPopover/__tests__/Overlay-test.js b/src/components/AnnotationPopover/__tests__/Overlay-test.js new file mode 100644 index 000000000..aa2cb5076 --- /dev/null +++ b/src/components/AnnotationPopover/__tests__/Overlay-test.js @@ -0,0 +1,18 @@ +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__/AnnotationPopover-test.js.snap b/src/components/AnnotationPopover/__tests__/__snapshots__/AnnotationPopover-test.js.snap index 23702f89b..e6761bc9b 100644 --- a/src/components/AnnotationPopover/__tests__/__snapshots__/AnnotationPopover-test.js.snap +++ b/src/components/AnnotationPopover/__tests__/__snapshots__/AnnotationPopover-test.js.snap @@ -1,5 +1,242 @@ // 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 pending annotation 1`] = `
+
+ +`; + +exports[`components/Overlay should correctly render a div without a Focus Trap if on mobile 1`] = ` +
+`; diff --git a/src/doc/__tests__/CreateHighlightDialog-test.js b/src/doc/__tests__/CreateHighlightDialog-test.js index 589f3f421..d6225867e 100644 --- a/src/doc/__tests__/CreateHighlightDialog-test.js +++ b/src/doc/__tests__/CreateHighlightDialog-test.js @@ -158,6 +158,9 @@ describe('doc/CreateHighlightDialog', () => { describe('onCommentClick()', () => { it('should create a plain highlight and render the popover', () => { + document.getSelection = jest.fn().mockReturnValue({ + removeAllRanges: jest.fn() + }); dialog.onCommentClick({ preventDefault: jest.fn(), stopPropagation: jest.fn() }); expect(dialog.emit).toBeCalledWith(CREATE_EVENT.comment); expect(dialog.renderAnnotationPopover).toBeCalled(); From d339a26778ba4f01a2eb41b16d1c72fd5eb1a258 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Fri, 2 Nov 2018 12:57:00 -0700 Subject: [PATCH 5/7] Fix: Tests --- src/Annotator.scss | 2 +- .../__tests__/__snapshots__/Overlay-test.js.snap | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Annotator.scss b/src/Annotator.scss index 5d5366f0f..fbdbb18f5 100644 --- a/src/Annotator.scss +++ b/src/Annotator.scss @@ -169,7 +169,7 @@ // Quad point positioning - the helper divs are positioned relative to the Rangy-created element .bp-doc .rangy-highlight { background-color: lighten($highlight-yellow, 10%); - opacity: 25%; + opacity: .25; position: relative; } diff --git a/src/components/AnnotationPopover/__tests__/__snapshots__/Overlay-test.js.snap b/src/components/AnnotationPopover/__tests__/__snapshots__/Overlay-test.js.snap index 0f378743c..421c7334c 100644 --- a/src/components/AnnotationPopover/__tests__/__snapshots__/Overlay-test.js.snap +++ b/src/components/AnnotationPopover/__tests__/__snapshots__/Overlay-test.js.snap @@ -1,6 +1,6 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`components/Overlay should correctly render a BRUI Overlay if not on mobile 1`] = ` +exports[`components/Overlay should correctly render a Focus Trap if not on mobile 1`] = ` Date: Fri, 2 Nov 2018 15:04:08 -0700 Subject: [PATCH 6/7] Fix: Exiting annotations modes properly re-binds handlers --- src/controllers/AnnotationModeController.js | 7 +++++++ src/controllers/DrawingModeController.js | 3 +-- src/controllers/HighlightModeController.js | 2 +- src/controllers/PointModeController.js | 3 +-- .../__tests__/HighlightModeController-test.js | 4 ++-- .../__tests__/PointModeController-test.js | 18 ++++-------------- src/doc/DocAnnotator.js | 3 --- 7 files changed, 16 insertions(+), 24 deletions(-) diff --git a/src/controllers/AnnotationModeController.js b/src/controllers/AnnotationModeController.js index de692882a..17acbc047 100644 --- a/src/controllers/AnnotationModeController.js +++ b/src/controllers/AnnotationModeController.js @@ -211,12 +211,19 @@ class AnnotationModeController extends EventEmitter { this.emit(CONTROLLER_EVENT.toggleMode); } + /** + * @return {void} + */ + resetMode() {} + /** * Disables the specified annotation mode * * @return {void} */ exit(): void { + this.resetMode(); + if (this.currentThread) { this.currentThread.unmountPopover(); } diff --git a/src/controllers/DrawingModeController.js b/src/controllers/DrawingModeController.js index ecff6d792..ca1c2ea63 100644 --- a/src/controllers/DrawingModeController.js +++ b/src/controllers/DrawingModeController.js @@ -256,7 +256,7 @@ class DrawingModeController extends AnnotationModeController { } /** @inheritdoc */ - exit() { + resetMode() { if (this.currentThread) { this.currentThread.clearBoundary(); } @@ -272,7 +272,6 @@ class DrawingModeController extends AnnotationModeController { pageElements.forEach((pageEl) => clearCanvas(pageEl, CLASS_ANNOTATION_LAYER_DRAW_IN_PROGRESS)); this.annotatedElement.classList.remove(CLASS_ANNOTATION_DRAW_MODE); - super.exit(); } /** diff --git a/src/controllers/HighlightModeController.js b/src/controllers/HighlightModeController.js index 6fab65202..2ce7a8264 100644 --- a/src/controllers/HighlightModeController.js +++ b/src/controllers/HighlightModeController.js @@ -34,7 +34,7 @@ class HighlightModeController extends AnnotationModeController { } /** @inheritdoc */ - exit(): void { + resetMode(): void { this.destroyPendingThreads(); window.getSelection().removeAllRanges(); this.unbindListeners(); // Disable mode diff --git a/src/controllers/PointModeController.js b/src/controllers/PointModeController.js index 674787336..e7aaf8449 100644 --- a/src/controllers/PointModeController.js +++ b/src/controllers/PointModeController.js @@ -57,7 +57,7 @@ class PointModeController extends AnnotationModeController { } /** @inheritdoc */ - exit(): void { + resetMode(): void { if (this.buttonEl) { this.buttonEl.classList.remove(CLASS_ACTIVE); } @@ -70,7 +70,6 @@ class PointModeController extends AnnotationModeController { } this.pendingThreadID = null; - super.exit(); } /** @inheritdoc */ diff --git a/src/controllers/__tests__/HighlightModeController-test.js b/src/controllers/__tests__/HighlightModeController-test.js index c014ecb95..46940194d 100644 --- a/src/controllers/__tests__/HighlightModeController-test.js +++ b/src/controllers/__tests__/HighlightModeController-test.js @@ -62,7 +62,7 @@ describe('controllers/HighlightModeController', () => { }); }); - describe('exit()', () => { + describe('resetMode()', () => { it('should exit annotation mode', () => { controller.destroyPendingThreads = jest.fn(); controller.unbindListeners = jest.fn(); @@ -75,7 +75,7 @@ describe('controllers/HighlightModeController', () => { controller.annotatedElement = document.createElement('div'); controller.annotatedElement.classList.add(CLASS_ANNOTATION_MODE); - controller.exit(); + controller.resetMode(); expect(controller.destroyPendingThreads).toBeCalled(); expect(controller.emit).toBeCalledWith(CONTROLLER_EVENT.bindDOMListeners); expect(controller.unbindListeners).toBeCalled(); diff --git a/src/controllers/__tests__/PointModeController-test.js b/src/controllers/__tests__/PointModeController-test.js index 4aebe6fa7..3fc8a6e0f 100644 --- a/src/controllers/__tests__/PointModeController-test.js +++ b/src/controllers/__tests__/PointModeController-test.js @@ -90,11 +90,8 @@ describe('controllers/PointModeController', () => { }); }); - describe('exit()', () => { + describe('resetMode()', () => { beforeEach(() => { - controller.destroyPendingThreads = jest.fn(); - controller.unbindListeners = jest.fn(); - // Set up annotation mode controller.annotatedElement = document.createElement('div'); controller.annotatedElement.classList.add(CLASS_ANNOTATION_MODE); @@ -104,19 +101,12 @@ describe('controllers/PointModeController', () => { controller.buttonEl.classList.add(CLASS_ACTIVE); }); - it('should exit annotation mode', () => { - controller.exit(); - expect(controller.destroyPendingThreads).toBeCalled(); - expect(controller.emit).toBeCalledWith(CONTROLLER_EVENT.exit, expect.any(Object)); - expect(controller.unbindListeners).toBeCalled(); - expect(controller.hadPendingThreads).toBeFalsy(); - }); - - it('should deactive mode button if available', () => { + it('should reset annotation mode', () => { controller.buttonEl = document.createElement('button'); controller.buttonEl.classList.add(CLASS_ACTIVE); - controller.exit(); + controller.resetMode(); expect(controller.buttonEl.classList).not.toContain(CLASS_ACTIVE); + expect(controller.hadPendingThreads).toBeFalsy(); }); }); diff --git a/src/doc/DocAnnotator.js b/src/doc/DocAnnotator.js index d200a65a1..b6f0b7931 100644 --- a/src/doc/DocAnnotator.js +++ b/src/doc/DocAnnotator.js @@ -492,9 +492,6 @@ class DocAnnotator extends Annotator { const location = this.getLocationFromEvent(this.lastHighlightEvent, highlightType); const controller = this.modeControllers[highlightType]; - this.highlighter.removeAllHighlights(); - this.resetHighlightSelection(this.lastHighlightEvent); - if (!location || !controller) { return null; } From 39ebc8aba0ae403837dd24d8cc53f6d2835cecc9 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Sun, 4 Nov 2018 09:45:59 -0800 Subject: [PATCH 7/7] Chore: Cleanup --- src/doc/DocAnnotator.js | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/doc/DocAnnotator.js b/src/doc/DocAnnotator.js index b6f0b7931..2d2dd0af4 100644 --- a/src/doc/DocAnnotator.js +++ b/src/doc/DocAnnotator.js @@ -76,8 +76,6 @@ class DocAnnotator extends Annotator { this.createPlainHighlight = this.createPlainHighlight.bind(this); // $FlowFixMe this.onSelectionChange = this.onSelectionChange.bind(this); - // $FlowFixMe - this.selectionTimeoutHandler = this.selectionTimeoutHandler.bind(this); } /** @@ -560,7 +558,11 @@ class DocAnnotator extends Annotator { return; } - this.selectionEndTimeout = setTimeout(this.selectionTimeoutHandler, SELECTION_TIMEOUT); + this.selectionEndTimeout = setTimeout(() => { + if (this.createHighlightDialog && !this.createHighlightDialog.isVisible) { + this.createHighlightDialog.show(this.lastSelection); + } + }, SELECTION_TIMEOUT); const { page } = util.getPageInfo(event.target); @@ -583,12 +585,6 @@ class DocAnnotator extends Annotator { this.lastSelection = selection; } - selectionTimeoutHandler = () => { - if (this.createHighlightDialog && !this.createHighlightDialog.isVisible) { - this.createHighlightDialog.show(this.lastSelection); - } - }; - /** * Mode controllers setup. * @@ -714,8 +710,6 @@ class DocAnnotator extends Annotator { // we trigger the create handler instead of the click handler if ((this.createHighlightDialog && hasMouseMoved) || event.type === 'dblclick') { this.highlightCreateHandler(event); - } else if (!hasMouseMoved) { - this.clickHandler(event); } };