Fix: Reset popover UI on re-render/scale events

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;
     }
 
     /**

src/Annotator.js

@@ -180,19 +180,17 @@ 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.
      *
      * @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));
     }
 

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();

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!');
     }
 

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;
     }
 
     /**

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);

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: {} };

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();
         });

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,