diff --git a/src/AnnotationThread.js b/src/AnnotationThread.js index e3fd9e393..3a03e2f73 100644 --- a/src/AnnotationThread.js +++ b/src/AnnotationThread.js @@ -209,15 +209,15 @@ class AnnotationThread extends EventEmitter { */ unmountPopover() { this.reset(); - this.toggleFlippedThreadEl(); - const pageEl = this.getPopoverParent(); - const popoverLayer = pageEl.querySelector('.ba-dialog-layer'); - if (this.popoverComponent && popoverLayer) { - unmountComponentAtNode(popoverLayer); - this.popoverComponent = null; + const popoverLayers = this.container.querySelectorAll('.ba-dialog-layer'); + if (!this.popoverComponent || popoverLayers.length === 0) { + return; } + + popoverLayers.forEach(unmountComponentAtNode); + this.popoverComponent = null; } /** diff --git a/src/Annotator.js b/src/Annotator.js index e739ab7fe..be570f726 100644 --- a/src/Annotator.js +++ b/src/Annotator.js @@ -180,9 +180,8 @@ class Annotator extends EventEmitter { * @param {AnnotationType} annotationType - Type of annotation * @return {Object} Location object */ - /* eslint-disable no-unused-vars */ + /* eslint-disable-next-line no-unused-vars */ getLocationFromEvent = (event: Event, annotationType: AnnotationType): ?Location => {}; - /* eslint-enable no-unused-vars */ /** * Must be implemented to determine the annotated element in the viewer. @@ -190,9 +189,8 @@ class Annotator extends EventEmitter { * @param {HTMLElement} containerEl - Container element for the viewer * @return {HTMLElement} Annotated element in the viewer */ - /* eslint-disable no-unused-vars */ + /* eslint-disable-next-line no-unused-vars */ getAnnotatedEl(containerEl: HTMLElement): ?HTMLElement {} - /* eslint-enable no-unused-vars */ /** * Annotations setup. @@ -397,16 +395,23 @@ class Annotator extends EventEmitter { return thread; } + /** + * Resets any annotation UI on render/scale events + * + * @param {number} [pageNum] - optional page number + * @return {void} + */ + /* eslint-disable-next-line no-unused-vars */ + resetAnnotationUI(pageNum?: number) {} + /** * Renders annotations from memory. * * @return {void} */ render() { - Object.keys(this.modeControllers).forEach((mode) => { - const controller = this.modeControllers[mode]; - controller.render(); - }); + this.resetAnnotationUI(); + Object.keys(this.modeControllers).forEach((mode) => this.modeControllers[mode].render()); } /** @@ -416,6 +421,7 @@ class Annotator extends EventEmitter { * @return {void} */ renderPage(pageNum: number) { + this.resetAnnotationUI(pageNum); Object.keys(this.modeControllers).forEach((mode) => this.modeControllers[mode].renderPage(pageNum)); } diff --git a/src/__tests__/AnnotationThread-test.js b/src/__tests__/AnnotationThread-test.js index a48c29848..b753fb330 100644 --- a/src/__tests__/AnnotationThread-test.js +++ b/src/__tests__/AnnotationThread-test.js @@ -61,6 +61,34 @@ describe('AnnotationThread', () => { }); }); + describe('unmountPopover', () => { + beforeEach(() => { + thread.popoverComponent = {}; + thread.reset = jest.fn(); + thread.toggleFlippedThreadEl = jest.fn(); + thread.container = { + querySelectorAll: jest.fn().mockReturnValue([rootElement]) + }; + }); + + it('should there are no popover layers', () => { + thread.container = { + querySelectorAll: jest.fn().mockReturnValue([]) + }; + thread.unmountPopover(); + expect(thread.popoverComponent).not.toBeNull(); + expect(thread.reset).toBeCalled(); + expect(thread.toggleFlippedThreadEl).toBeCalled(); + }); + + it('should unmount any visible popovers', () => { + thread.unmountPopover(); + expect(thread.popoverComponent).toBeNull(); + expect(thread.reset).toBeCalled(); + expect(thread.toggleFlippedThreadEl).toBeCalled(); + }); + }); + describe('hide()', () => { it('should hide the thread element', () => { thread.unmountPopover = jest.fn(); diff --git a/src/controllers/AnnotationModeController.js b/src/controllers/AnnotationModeController.js index ff13da1cf..27cea78ac 100644 --- a/src/controllers/AnnotationModeController.js +++ b/src/controllers/AnnotationModeController.js @@ -329,9 +329,8 @@ class AnnotationModeController extends EventEmitter { * @param {Object} params - Annotation thread params * @return {AnnotationThread|null} Annotation thread instance or null */ - /* eslint-disable no-unused-vars */ + /* eslint-disable-next-line no-unused-vars */ instantiateThread(params: Object): AnnotationThread { - /* eslint-enable no-unused-vars */ throw new Error('Implement me!'); } diff --git a/src/doc/CreateHighlightDialog.js b/src/doc/CreateHighlightDialog.js index e47cb5276..2fd96843a 100644 --- a/src/doc/CreateHighlightDialog.js +++ b/src/doc/CreateHighlightDialog.js @@ -79,12 +79,18 @@ class CreateHighlightDialog extends EventEmitter { * @return {void} */ unmountPopover() { + if (!this.isVisible) { + return; + } + this.isVisible = false; - const popoverLayer = this.container.querySelector('.ba-dialog-layer'); - if (this.createPopoverComponent && popoverLayer) { - unmountComponentAtNode(popoverLayer); - this.createPopoverComponent = null; + const popoverLayers = this.container.querySelectorAll('.ba-dialog-layer'); + if (!this.createPopoverComponent || popoverLayers.length === 0) { + return; } + + popoverLayers.forEach(unmountComponentAtNode); + this.createPopoverComponent = null; } /** diff --git a/src/doc/DocAnnotator.js b/src/doc/DocAnnotator.js index 15dda9e02..5bbe847eb 100644 --- a/src/doc/DocAnnotator.js +++ b/src/doc/DocAnnotator.js @@ -76,6 +76,8 @@ class DocAnnotator extends Annotator { this.createPlainHighlight = this.createPlainHighlight.bind(this); // $FlowFixMe this.onSelectionChange = this.onSelectionChange.bind(this); + // $FlowFixMe + this.resetAnnotationUI = this.resetAnnotationUI.bind(this); } /** @@ -240,26 +242,21 @@ class DocAnnotator extends Annotator { return location; }; - /** - * Override to factor in highlight types being filtered out, if disabled. Also scales annotation canvases. - * - * @param {number} pageNum Page number - * @return {void} - */ - renderPage(pageNum: number) { + /** @inheritdoc */ + resetAnnotationUI(pageNum?: number) { // $FlowFixMe document.getSelection().removeAllRanges(); if (this.highlighter) { this.highlighter.removeAllHighlights(); } - // Scale existing canvases on re-render - this.scaleAnnotationCanvases(pageNum); - super.renderPage(pageNum); - if (this.createHighlightDialog) { this.createHighlightDialog.unmountPopover(); } + + if (pageNum) { + this.scaleAnnotationCanvases(pageNum); + } } /** @@ -331,6 +328,8 @@ class DocAnnotator extends Annotator { bindDOMListeners() { super.bindDOMListeners(); + this.container.addEventListener('resize', this.resetAnnotationUI); + // Highlight listeners on desktop & mobile if (this.plainHighlightEnabled || this.commentHighlightEnabled) { this.annotatedElement.addEventListener('wheel', this.hideCreateDialog); @@ -365,6 +364,7 @@ class DocAnnotator extends Annotator { unbindDOMListeners() { super.unbindDOMListeners(); + this.container.removeEventListener('resize', this.resetAnnotationUI); this.annotatedElement.removeEventListener('wheel', this.hideCreateDialog); this.annotatedElement.removeEventListener('touchend', this.hideCreateDialog); this.annotatedElement.removeEventListener('click', this.clickHandler); diff --git a/src/doc/__tests__/CreateHighlightDialog-test.js b/src/doc/__tests__/CreateHighlightDialog-test.js index 589f3f421..120a9f401 100644 --- a/src/doc/__tests__/CreateHighlightDialog-test.js +++ b/src/doc/__tests__/CreateHighlightDialog-test.js @@ -29,7 +29,6 @@ describe('doc/CreateHighlightDialog', () => { }); dialog.annotatedElement = rootElement; dialog.renderAnnotationPopover = jest.fn(); - dialog.unmountPopover = jest.fn(); dialog.emit = jest.fn(); util.shouldDisplayMobileUI = jest.fn().mockReturnValue(false); @@ -64,6 +63,36 @@ describe('doc/CreateHighlightDialog', () => { }); }); + describe('unmountPopover', () => { + beforeEach(() => { + dialog.isVisible = true; + dialog.createPopoverComponent = {}; + + dialog.container = { + querySelectorAll: jest.fn().mockReturnValue([rootElement]) + }; + }); + + it('should do nothing if the popover is not visible', () => { + dialog.isVisible = false; + dialog.unmountPopover(); + expect(dialog.createPopoverComponent).not.toBeNull(); + }); + + it('should there are no popover layers', () => { + dialog.container = { + querySelectorAll: jest.fn().mockReturnValue([]) + }; + dialog.unmountPopover(); + expect(dialog.createPopoverComponent).not.toBeNull(); + }); + + it('should unmount any visible create highlight popovers', () => { + dialog.unmountPopover(); + expect(dialog.createPopoverComponent).toBeNull(); + }); + }); + describe('show()', () => { const selection = { anchorNode: {} }; const pageInfo = { page: 1, pageEl: {} }; diff --git a/src/doc/__tests__/DocAnnotator-test.js b/src/doc/__tests__/DocAnnotator-test.js index eb84fd6a4..3f356d9a2 100644 --- a/src/doc/__tests__/DocAnnotator-test.js +++ b/src/doc/__tests__/DocAnnotator-test.js @@ -409,7 +409,7 @@ describe('doc/DocAnnotator', () => { }); }); - describe('renderPage()', () => { + describe('resetAnnotationUI()', () => { beforeEach(() => { document.getSelection = jest.fn().mockReturnValue({ removeAllRanges: jest.fn() @@ -418,27 +418,17 @@ describe('doc/DocAnnotator', () => { removeAllHighlights: jest.fn() }; annotator.scaleAnnotationCanvases = jest.fn(); - annotator.modeControllers = { - type: { - renderPage: jest.fn() - }, - type2: { - renderPage: jest.fn() - } - }; }); - it('should clear and hide createHighlightDialog on page render', () => { - annotator.createHighlightDialog = { - isVisible: true, - hide: jest.fn(), - destroy: jest.fn(), - unmountPopover: jest.fn() - }; - annotator.renderPage(1); + it('should clear and hide createHighlightDialog', () => { + annotator.resetAnnotationUI(); + expect(annotator.scaleAnnotationCanvases).not.toBeCalled(); + expect(annotator.createHighlightDialog.unmountPopover).toBeCalled(); + }); + + it('should scale annotation canvases if page number is provided', () => { + annotator.resetAnnotationUI(1); expect(annotator.scaleAnnotationCanvases).toBeCalledWith(1); - expect(annotator.modeControllers.type.renderPage).toBeCalledWith(1); - expect(annotator.modeControllers.type2.renderPage).toBeCalledWith(1); expect(annotator.createHighlightDialog.unmountPopover).toBeCalled(); }); }); @@ -478,10 +468,6 @@ describe('doc/DocAnnotator', () => { Object.defineProperty(Annotator.prototype, 'setupAnnotations', { value: jest.fn() }); rangy.createClassApplier = jest.fn(); rangy.createHighlighter = jest.fn().mockReturnValue(highlighter); - - annotator.createHighlightDialog = { - addListener: jest.fn() - }; }); afterEach(() => { @@ -518,6 +504,10 @@ describe('doc/DocAnnotator', () => { beforeEach(() => { Object.defineProperty(Annotator.prototype, 'bindDOMListeners', { value: jest.fn() }); + annotator.container = { + addEventListener: jest.fn(), + removeEventListener: jest.fn() + }; annotator.annotatedElement = { addEventListener: jest.fn(), removeEventListener: jest.fn() @@ -535,6 +525,7 @@ describe('doc/DocAnnotator', () => { it('should bind DOM listeners if user can annotate and highlight', () => { annotator.bindDOMListeners(); + expect(annotator.container.addEventListener).toBeCalledWith('resize', annotator.resetAnnotationUI); expect(annotator.annotatedElement.addEventListener).toBeCalledWith( 'mouseup', annotator.highlightMouseupHandler @@ -628,6 +619,9 @@ describe('doc/DocAnnotator', () => { describe('unbindDOMListeners()', () => { beforeEach(() => { + annotator.container = { + removeEventListener: jest.fn() + }; annotator.annotatedElement = { removeEventListener: jest.fn() }; @@ -640,6 +634,7 @@ describe('doc/DocAnnotator', () => { annotator.permissions.can_annotate = true; annotator.unbindDOMListeners(); + expect(annotator.container.removeEventListener).toBeCalledWith('resize', annotator.resetAnnotationUI); expect(annotator.annotatedElement.removeEventListener).toBeCalledWith( 'mouseup', annotator.highlightMouseupHandler @@ -967,7 +962,7 @@ describe('doc/DocAnnotator', () => { it('should do nothing and return true if a createHighlightDialog is visible', () => { annotator.plainHighlightEnabled = true; - annotator.createHighlightDialog = { isVisible: true }; + annotator.createHighlightDialog.isVisible = true; expect(annotator.highlightClickHandler(event)).toBeTruthy(); expect(annotator.hideAnnotations).not.toBeCalled(); }); diff --git a/src/drawing/DrawingThread.js b/src/drawing/DrawingThread.js index 8b04f5be8..ee8286bab 100644 --- a/src/drawing/DrawingThread.js +++ b/src/drawing/DrawingThread.js @@ -163,13 +163,13 @@ class DrawingThread extends AnnotationThread { this.annotatedElement.removeEventListener('touchend', eventToLocationHandler); } - /* eslint-disable no-unused-vars */ /** * Handle a pointer movement * * @param {Object} location - The location information of the pointer * @return {void} */ + /* eslint-disable-next-line no-unused-vars */ handleMove(location) {} /** @@ -178,6 +178,7 @@ class DrawingThread extends AnnotationThread { * @param {Object} location - The location information of the pointer * @return {void} */ + /* eslint-disable-next-line no-unused-vars */ handleStart(location) {} /** @@ -186,8 +187,8 @@ class DrawingThread extends AnnotationThread { * @param {Object} location - The location information of the pointer * @return {void} */ + /* eslint-disable-next-line no-unused-vars */ handleStop(location) {} - /* eslint-disable no-unused-vars */ /** * Delete a saved drawing thread by deleting each annotation @@ -430,10 +431,9 @@ class DrawingThread extends AnnotationThread { * Create an annotation data object to pass to annotation service. * * @param {string} type - Type of annotation - * @param {string} message - Annotation text * @return {Object} Annotation data */ - createAnnotationData(type, message) { + createAnnotationData(type) { return { item: { id: this.fileVersionId,