Fix: Create highlight dialog is misaligned on first highlight (#159)

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

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

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

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

src/doc/__tests__/CreateHighlightDialog-test.js

@@ -186,14 +186,22 @@ 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', () => {
             const y = 50;
             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;
         });
     });
 

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;

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;
+}