Fix: Ensure popover parent element is determined correctly (#292)

box · Nov 20, 2018 · eab98ec · eab98ec
1 parent b60bc56
commit eab98ec
Show file tree

Hide file tree

Showing 5 changed files with 42 additions and 23 deletions.
diff --git a/src/AnnotationThread.js b/src/AnnotationThread.js
@@ -157,6 +157,10 @@ class AnnotationThread extends EventEmitter {
      * @return {HTMLElement} Parent element for the annotation popover
      */
     getPopoverParent() {
+        if (!this.location || !this.location.page) {
+            return this.annotatedElement;
+        }
+
         return util.shouldDisplayMobileUI(this.container)
             ? this.container
             : // $FlowFixMe
@@ -507,8 +511,10 @@ class AnnotationThread extends EventEmitter {
             return;
         }
 
-        const pageEl = this.getPopoverParent();
-        pageEl.scrollIntoView();
+        const pageEl = util.getPageEl(this.annotatedElement, this.location.page);
+        if (pageEl) {
+            pageEl.scrollIntoView();
+        }
     }
 
     /**

diff --git a/src/__tests__/AnnotationThread-test.js b/src/__tests__/AnnotationThread-test.js
@@ -61,6 +61,34 @@ describe('AnnotationThread', () => {
         });
     });
 
+    describe('getPopoverParent()', () => {
+        beforeEach(() => {
+            thread.annotatedElement = 'annotatedElement';
+            thread.container = 'container';
+            util.getPageEl = jest.fn().mockReturnValue('pageEl');
+        });
+
+        it('should return the annotated element if no location or location page is set', () => {
+            thread.location = undefined;
+            expect(thread.getPopoverParent()).toEqual('annotatedElement');
+
+            thread.location = {};
+            expect(thread.getPopoverParent()).toEqual('annotatedElement');
+        });
+
+        it('should return container if user should see the mobile UI', () => {
+            thread.location = { page: 1 };
+            util.shouldDisplayMobileUI = jest.fn().mockReturnValue(true);
+            expect(thread.getPopoverParent()).toEqual('container');
+        });
+
+        it('should return the page element if user should NOT see the mobile UI', () => {
+            thread.location = { page: 1 };
+            util.shouldDisplayMobileUI = jest.fn().mockReturnValue(false);
+            expect(thread.getPopoverParent()).toEqual('pageEl');
+        });
+    });
+
     describe('unmountPopover', () => {
         beforeEach(() => {
             thread.popoverComponent = {};
@@ -380,7 +408,7 @@ describe('AnnotationThread', () => {
             pageEl = {
                 scrollIntoView: jest.fn()
             };
-            thread.getPopoverParent = jest.fn().mockReturnValue(pageEl);
+            util.getPageEl = jest.fn().mockReturnValue(pageEl);
         });
 
         it('should do nothing if annotation does not have a location or page', () => {

diff --git a/src/doc/DocHighlightThread.js b/src/doc/DocHighlightThread.js
@@ -463,7 +463,7 @@ class DocHighlightThread extends AnnotationThread {
         // Position it below lower right corner or center of the highlight - we need
         // to reposition every time since the DOM could have changed from
         // zooming
-        const pageEl = this.getPopoverParent();
+        const pageEl = this.popoverParent;
         const pageDimensions = pageEl.getBoundingClientRect();
         const pageHeight = pageDimensions.height - PAGE_PADDING_TOP - PAGE_PADDING_BOTTOM;
         const [browserX, browserY] = docUtil.getScaledPDFCoordinates(

diff --git a/src/doc/DocPointThread.js b/src/doc/DocPointThread.js
@@ -53,8 +53,7 @@ class DocPointThread extends AnnotationThread {
             return;
         }
 
-        const pageEl = this.getPopoverParent();
-
+        const pageEl = this.popoverParent;
         const popoverEl = findElement(
             this.annotatedElement,
             SELECTOR_CLASS_ANNOTATION_POPOVER,

diff --git a/src/image/ImagePointThread.js b/src/image/ImagePointThread.js
@@ -13,16 +13,6 @@ import {
 } from '../constants';
 
 class ImagePointThread extends AnnotationThread {
-    /**
-     * Gets the popover parent for image point threads. The popover parent
-     * should not the image element but rather the annotatedElement
-     *
-     * @return {HTMLElement} The correct parent based on mobile view or not
-     */
-    getPopoverParent() {
-        return shouldDisplayMobileUI(this.container) ? this.container : this.annotatedElement;
-    }
-
     /** @inheritdoc */
     show() {
         const [browserX, browserY] = getBrowserCoordinatesFromLocation(this.location, this.annotatedElement);
@@ -55,15 +45,11 @@ class ImagePointThread extends AnnotationThread {
         const dialogDimensions = popoverEl.getBoundingClientRect();
         const dialogWidth = dialogDimensions.width;
 
-        // Get image tag inside viewer, based on page number. All images are page 1 by default.
-        const imageEl = this.getPopoverParent();
-
         // Center middle of dialog with point - this coordinate is with respect to the page
         const threadIconLeftX = this.element.offsetLeft + POINT_ANNOTATION_ICON_WIDTH / 2;
         let dialogLeftX = threadIconLeftX - dialogWidth / 2;
 
-        const isUpperHalf = isInUpperHalf(this.element, imageEl);
-
+        const isUpperHalf = isInUpperHalf(this.element, this.popoverParent);
         const flippedPopoverOffset = isUpperHalf
             ? 0
             : popoverEl.getBoundingClientRect().height +
@@ -85,8 +71,8 @@ class ImagePointThread extends AnnotationThread {
         // just center the dialog and cause scrolling since there is nothing
         // else we can do
         const pageWidth =
-            imageEl.clientWidth > this.annotatedElement.clientWidth
-                ? imageEl.clientWidth
+            this.popoverParent.clientWidth > this.annotatedElement.clientWidth
+                ? this.popoverParent.clientWidth
                 : this.annotatedElement.clientWidth;
         dialogLeftX = repositionCaret(popoverEl, dialogLeftX, dialogWidth, threadIconLeftX, pageWidth, !isUpperHalf);