diff --git a/src/AnnotationThread.js b/src/AnnotationThread.js index 3a03e2f73..ba7fc4d0c 100644 --- 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 index b753fb330..d46f91136 100644 --- 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 index 9bcec84c8..369a76502 100644 --- 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 index 416f05eab..a2858c823 100644 --- 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 index 78a22d976..f05b8ba06 100644 --- 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);