From d4d4fdd792eda17690b7e0d52be9e35f3337c5be Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Fri, 6 Apr 2018 12:54:44 -0700 Subject: [PATCH] Fix: Create highlight dialog is misaligned on first highlight (#159) --- src/__tests__/util-test.js | 15 +++++++-- src/controllers/AnnotationModeController.js | 2 +- src/doc/CreateHighlightDialog.js | 15 +++++---- src/doc/DocHighlightDialog.js | 33 +------------------ .../__tests__/CreateHighlightDialog-test.js | 11 ++++++- src/doc/__tests__/DocHighlightDialog-test.js | 21 +----------- src/util.js | 29 ++++++++++++++++ 7 files changed, 64 insertions(+), 62 deletions(-) diff --git a/src/__tests__/util-test.js b/src/__tests__/util-test.js index 4cf7d89e9..3e609abeb 100644 --- a/src/__tests__/util-test.js +++ b/src/__tests__/util-test.js @@ -41,7 +41,8 @@ import { isInDialog, focusTextArea, hasValidBoundaryCoordinates, - generateMobileDialogEl + generateMobileDialogEl, + getDialogWidth } from '../util'; import { STATES, @@ -53,7 +54,8 @@ import { CLASS_MOBILE_DIALOG_HEADER, CLASS_DIALOG_CLOSE, CLASS_ANNOTATION_PLAIN_HIGHLIGHT, - CLASS_ANIMATE_DIALOG + CLASS_ANIMATE_DIALOG, + CLASS_HIDDEN } from '../constants'; const DIALOG_WIDTH = 81; @@ -845,4 +847,13 @@ describe('util', () => { expect(el).to.not.have.class(`.${CLASS_ANIMATE_DIALOG}`); }); }); + + describe('getDialogWidth', () => { + it('should calculate dialog width without displaying the dialog', () => { + const dialogEl = document.querySelector(SELECTOR_ANNOTATION_DIALOG); + dialogEl.style.width = '100px'; + expect(getDialogWidth(dialogEl)).to.equal(100); + expect(dialogEl).to.have.class(CLASS_HIDDEN); + }); + }); }); diff --git a/src/controllers/AnnotationModeController.js b/src/controllers/AnnotationModeController.js index fefb29fee..11d6cea99 100644 --- a/src/controllers/AnnotationModeController.js +++ b/src/controllers/AnnotationModeController.js @@ -231,7 +231,7 @@ class AnnotationModeController extends EventEmitter { * @return {void} */ unregisterThread(thread) { - if (!thread || !thread.location || !thread.location.page) { + if (!thread || !thread.location || !thread.location.page || !this.threads[thread.location.page]) { return; } diff --git a/src/doc/CreateHighlightDialog.js b/src/doc/CreateHighlightDialog.js index 7798a21e8..2ad95b0ad 100644 --- a/src/doc/CreateHighlightDialog.js +++ b/src/doc/CreateHighlightDialog.js @@ -1,6 +1,6 @@ import CreateAnnotationDialog from '../CreateAnnotationDialog'; import { ICON_HIGHLIGHT, ICON_HIGHLIGHT_COMMENT } from '../icons/icons'; -import { generateBtn, repositionCaret, getPageInfo } from '../util'; +import { generateBtn, repositionCaret, getPageInfo, getDialogWidth, showElement } from '../util'; import { getDialogCoordsFromRange } from '../doc/docUtil'; import { CREATE_EVENT, @@ -120,14 +120,14 @@ class CreateHighlightDialog extends CreateAnnotationDialog { const dialogParentEl = this.isMobile ? newParentEl : pageInfo.pageEl; super.show(dialogParentEl); - if (!this.isMobile) { - this.setPosition(selection); - } - // Add to parent if it hasn't been added already if (!this.parentEl.querySelector(`.${CLASS_CREATE_DIALOG}`)) { this.parentEl.appendChild(this.containerEl); } + + if (!this.isMobile) { + this.setPosition(selection); + } } /** @inheritdoc */ @@ -188,7 +188,8 @@ class CreateHighlightDialog extends CreateAnnotationDialog { return; } - const dialogX = this.position.x - 1 - this.containerEl.clientWidth / 2; + const dialogWidth = getDialogWidth(this.containerEl); + const dialogX = this.position.x - 1 - dialogWidth / 2; const xPos = repositionCaret( this.containerEl, dialogX, @@ -201,6 +202,8 @@ class CreateHighlightDialog extends CreateAnnotationDialog { this.containerEl.style.left = `${xPos}px`; // Plus 5 pixels for caret this.containerEl.style.top = `${this.position.y + 5}px`; + + showElement(this.containerEl); } /** diff --git a/src/doc/DocHighlightDialog.js b/src/doc/DocHighlightDialog.js index 46ea0d559..3d7f0b755 100644 --- a/src/doc/DocHighlightDialog.js +++ b/src/doc/DocHighlightDialog.js @@ -143,7 +143,7 @@ class DocHighlightDialog extends AnnotationDialog { const [browserX, browserY] = this.getScaledPDFCoordinates(pageDimensions, pageHeight); pageEl.appendChild(this.element); - const highlightDialogWidth = this.getDialogWidth(); + const highlightDialogWidth = util.getDialogWidth(this.element); let dialogX = browserX - highlightDialogWidth / 2; // Center dialog // Shorten extra transparent border top if showing comments dialog @@ -470,37 +470,6 @@ class DocHighlightDialog extends AnnotationDialog { } } - /** - * Calculates the dialog width if the highlighter's name is to be displayed - * in the annotations dialog - * - * @private - * @return {number} Annotations dialog width - */ - getDialogWidth() { - // Ensure dialog will not be displayed off the page when - // calculating the dialog width - const prevDialogX = this.element.style.left; - this.element.style.left = 0; - - // Switches to 'visibility: hidden' to ensure that dialog takes up - // DOM space while still being invisible - util.hideElementVisibility(this.element); - util.showElement(this.element); - - this.highlightDialogWidth = this.element.getBoundingClientRect().width; - - // Switches back to 'display: none' to so that it no longer takes up place - // in the DOM while remaining hidden - util.hideElement(this.element); - util.showInvisibleElement(this.element); - - // Reset dialog left positioning - this.element.style.left = prevDialogX; - - return this.highlightDialogWidth; - } - /** * Get scaled coordinates for the lower center point of the highlight if the * highlight has comments or the lower right corner of the highlight for diff --git a/src/doc/__tests__/CreateHighlightDialog-test.js b/src/doc/__tests__/CreateHighlightDialog-test.js index c520c9058..b4797ae89 100644 --- a/src/doc/__tests__/CreateHighlightDialog-test.js +++ b/src/doc/__tests__/CreateHighlightDialog-test.js @@ -186,7 +186,14 @@ describe('doc/CreateHighlightDialog', () => { describe('updatePosition()', () => { beforeEach(() => { - dialog.show(); + sandbox.stub(util, 'showElement'); + dialog.isMobile = false; + }); + + it('should do nothing on mobile devices', () => { + dialog.isMobile = true; + dialog.updatePosition(); + expect(util.showElement).to.not.be.called; }); it('should update the top of the ui element', () => { @@ -194,6 +201,7 @@ describe('doc/CreateHighlightDialog', () => { dialog.position.y = y; dialog.updatePosition(); expect(dialog.containerEl.style.top).to.equal(`${y + 5}px`); + expect(util.showElement).to.be.called; }); it('should update the left of the ui element, to center it', () => { @@ -202,6 +210,7 @@ describe('doc/CreateHighlightDialog', () => { sandbox.stub(util, 'repositionCaret').returns(x); dialog.updatePosition(); expect(dialog.containerEl.style.left).to.equal(`${x}px`); + expect(util.showElement).to.be.called; }); }); diff --git a/src/doc/__tests__/DocHighlightDialog-test.js b/src/doc/__tests__/DocHighlightDialog-test.js index ed2152073..b0f3a5873 100644 --- a/src/doc/__tests__/DocHighlightDialog-test.js +++ b/src/doc/__tests__/DocHighlightDialog-test.js @@ -238,7 +238,7 @@ describe('doc/DocHighlightDialog', () => { describe('position()', () => { beforeEach(() => { stubs.scaled = sandbox.stub(dialog, 'getScaledPDFCoordinates').returns([150, 2]); - stubs.width = sandbox.stub(dialog, 'getDialogWidth'); + stubs.width = sandbox.stub(util, 'getDialogWidth'); stubs.caret = sandbox.stub(util, 'repositionCaret'); stubs.show = sandbox.stub(util, 'showElement'); stubs.fit = sandbox.stub(dialog, 'fitDialogHeightInPage'); @@ -619,25 +619,6 @@ describe('doc/DocHighlightDialog', () => { }); }); - describe('getDialogWidth', () => { - it('should calculate dialog width once annotator\'s user name has been populated', () => { - const highlightLabelEl = dialog.element.querySelector(`.${CLASS_HIGHLIGHT_LABEL}`); - highlightLabelEl.innerHTML = 'Bob highlighted'; - dialog.element.style.width = '100px'; - dialog.element.style.left = '30px'; - - const width = dialog.getDialogWidth(); - expect(width).to.equal(100); - expect(dialog.element.style.left).to.equal('30px'); - }); - - it('should return previously set dialog width if already calculated', () => { - dialog.element.style.width = '252px'; - const width = dialog.getDialogWidth(); - expect(width).to.equal(252); // Default comments dialog width - }); - }); - describe('getScaledPDFCoordinates()', () => { it('should lower right corner coordinates of dialog when a highlight does not have comments', () => { dialog.hasComments = false; diff --git a/src/util.js b/src/util.js index 2b7422060..28cea9678 100644 --- a/src/util.js +++ b/src/util.js @@ -835,3 +835,32 @@ export function generateMobileDialogEl() { return el; } + +/** + * Calculates the dialog width when visible + * + * @param {HTMLElement} dialogEl - Annotation dialog element + * @return {number} Annotations dialog width + */ +export function getDialogWidth(dialogEl) { + const element = dialogEl; + + // Switches to 'visibility: hidden' to ensure that dialog takes up + // DOM space while still being invisible + hideElementVisibility(element); + showElement(element); + + // Ensure dialog will not be displayed off the page when + // calculating the dialog width + element.style.left = 0; + + const boundingRect = element.getBoundingClientRect(); + const dialogWidth = boundingRect.width; + + // Switches back to 'display: none' to so that it no longer takes up place + // in the DOM while remaining hidden + hideElement(element); + showInvisibleElement(element); + + return dialogWidth; +}