Merge pull request #20295 from calixteman/comment_update_position

[Editor] Update the color and the position of the comment button in reading mode they've been modified

src/display/annotation_layer.js

@@ -202,7 +202,19 @@ class AnnotationElement {
   }
 
   get hasPopupData() {
-    return AnnotationElement._hasPopupData(this.data);
+    return (
+      AnnotationElement._hasPopupData(this.data) ||
+      (this.enableComment && !!this.commentText)
+    );
+  }
+
+  get commentData() {
+    const { data } = this;
+    const editor = this.annotationStorage?.getEditor(data.id);
+    if (editor) {
+      return editor.getData();
+    }
+    return data;
   }
 
   get hasCommentButton() {
@@ -210,7 +222,11 @@ class AnnotationElement {
   }
 
   get commentButtonPosition() {
-    const { quadPoints, rect } = this.data;
+    const editor = this.annotationStorage?.getEditor(this.data.id);
+    if (editor) {
+      return editor.commentButtonPositionInPage;
+    }
+    const { quadPoints, inkLists, rect } = this.data;
     let maxX = -Infinity;
     let maxY = -Infinity;
     if (quadPoints?.length >= 8) {
@@ -224,6 +240,21 @@ class AnnotationElement {
       }
       return [maxX, maxY];
     }
+    if (inkLists?.length >= 1) {
+      for (const inkList of inkLists) {
+        for (let i = 0, ii = inkList.length; i < ii; i += 2) {
+          if (inkList[i + 1] > maxY) {
+            maxY = inkList[i + 1];
+            maxX = inkList[i];
+          } else if (inkList[i + 1] === maxY) {
+            maxX = Math.max(maxX, inkList[i]);
+          }
+        }
+      }
+      if (maxX !== Infinity) {
+        return [maxX, maxY];
+      }
+    }
     if (rect) {
       return [rect[2], rect[3]];
     }
@@ -2380,7 +2411,6 @@ class PopupElement {
     this.#dateObj = PDFDateString.toDateObject(modificationDate);
 
     if (commentManager) {
-      this.#popupAbortController = new AbortController();
       this.#renderCommentButton();
     } else {
       this.trigger = elements.flatMap(e => e.getElementsToTriggerPopup());
@@ -2457,7 +2487,7 @@ class PopupElement {
     button.ariaHasPopup = "dialog";
     button.ariaControls = "commentPopup";
 
-    const { signal } = this.#popupAbortController;
+    const { signal } = (this.#popupAbortController = new AbortController());
     button.addEventListener("keydown", this.#boundKeyDown, { signal });
     button.addEventListener(
       "click",
@@ -2488,19 +2518,26 @@ class PopupElement {
       },
       { signal }
     );
-    const { style } = button;
-    style.left = `calc(${this.#commentButtonPosition[0]}%)`;
-    style.top = `calc(${this.#commentButtonPosition[1]}% - var(--comment-button-dim))`;
-    if (this.commentButtonColor) {
-      style.backgroundColor = this.commentButtonColor;
-    }
+    this.#updateColor();
+    this.#updateCommentButtonPosition();
     parentContainer.after(button);
   }
 
+  #updateCommentButtonPosition() {
+    this.#renderCommentButton();
+    const [x, y] = this.#commentButtonPosition;
+    const { style } = this.#commentButton;
+    style.left = `calc(${x}%)`;
+    style.top = `calc(${y}% - var(--comment-button-dim))`;
+  }
+
+  #updateColor() {
+    this.#renderCommentButton();
+    this.#commentButton.style.backgroundColor = this.commentButtonColor || "";
+  }
+
   get commentButtonColor() {
-    const {
-      data: { color, opacity },
-    } = this.#firstElement;
+    const { color, opacity } = this.#firstElement.commentData;
     if (!color) {
       return null;
     }
@@ -2509,7 +2546,7 @@ class PopupElement {
 
   getData() {
     const { richText, color, opacity, creationDate, modificationDate } =
-      this.#firstElement.data;
+      this.#firstElement.commentData;
     return {
       contentsObj: { str: this.comment },
       richText,
@@ -2743,7 +2780,22 @@ class PopupElement {
 
   updateEdited({ rect, popup, deleted }) {
     if (this.#commentManager) {
-      this.#commentText = deleted ? null : popup.text;
+      if (deleted) {
+        this.remove();
+        this.#commentText = null;
+      } else if (popup) {
+        if (popup.deleted) {
+          this.remove();
+        } else {
+          this.#updateColor();
+          this.#commentText = popup.text;
+        }
+      }
+      if (rect) {
+        this.#commentButtonPosition = null;
+        this.#setCommentButtonPosition();
+        this.#updateCommentButtonPosition();
+      }
       return;
     }
     if (deleted || popup?.deleted) {
@@ -2758,7 +2810,7 @@ class PopupElement {
     if (rect) {
       this.#position = null;
     }
-    if (popup) {
+    if (popup && popup.text) {
       this.#richText = this.#makePopupContent(popup.text);
       this.#dateObj = PDFDateString.toDateObject(popup.date);
       this.#contentsObj = null;
@@ -3346,31 +3398,6 @@ class InkAnnotationElement extends AnnotationElement {
   addHighlightArea() {
     this.container.classList.add("highlightArea");
   }
-
-  get commentButtonPosition() {
-    const { inkLists, rect } = this.data;
-    if (inkLists?.length >= 1) {
-      let maxX = -Infinity;
-      let maxY = -Infinity;
-      for (const inkList of inkLists) {
-        for (let i = 0, ii = inkList.length; i < ii; i += 2) {
-          if (inkList[i + 1] > maxY) {
-            maxY = inkList[i + 1];
-            maxX = inkList[i];
-          } else if (inkList[i + 1] === maxY) {
-            maxX = Math.max(maxX, inkList[i]);
-          }
-        }
-      }
-      if (maxX !== Infinity) {
-        return [maxX, maxY];
-      }
-    }
-    if (rect) {
-      return [rect[2], rect[3]];
-    }
-    return null;
-  }
 }
 
 class HighlightAnnotationElement extends AnnotationElement {

src/display/annotation_storage.js

@@ -268,6 +268,10 @@ class AnnotationStorage {
     return false;
   }
 
+  getEditor(annotationId) {
+    return this.#editorsMap?.get(annotationId) || null;
+  }
+
   /**
    * @returns {{ids: Set<string>, hash: string}}
    */

src/display/editor/editor.js

@@ -1930,6 +1930,17 @@ class AnnotationEditor {
     return this._uiManager.direction === "ltr" ? [1, 0] : [0, 0];
   }
 
+  get commentButtonPositionInPage() {
+    const {
+      commentButtonPosition: [posX, posY],
+    } = this;
+    const [blX, blY, trX, trY] = this.getPDFRect();
+    return [
+      AnnotationEditor._round(blX + (trX - blX) * posX),
+      AnnotationEditor._round(blY + (trY - blY) * (1 - posY)),
+    ];
+  }
+
   get commentButtonColor() {
     return this._uiManager.makeCommentColor(
       this.getNonHCMColor(),

src/display/editor/freetext.js

@@ -904,13 +904,13 @@ class FreeTextEditor extends AnnotationEditor {
       content.append(div);
     }
 
-    const params = {
+    annotation.updateEdited({
       rect: this.getPDFRect(),
-    };
-    params.popup = this.hasEditedComment
-      ? this.comment
-      : { text: this.#content };
-    annotation.updateEdited(params);
+      popup:
+        this._uiManager.hasCommentManager() || this.hasEditedComment
+          ? this.comment
+          : { text: this.#content },
+    });
 
     return content;
   }

src/display/editor/highlight.js

@@ -172,12 +172,13 @@ class HighlightEditor extends AnnotationEditor {
     );
     this.#focusOutlines = outlinerForOutline.getOutlines();
 
-    // The last point is in the pages coordinate system.
-    const { firstPoint, lastPoint } = this.#focusOutlines;
+    const { firstPoint } = this.#highlightOutlines;
     this.#firstPoint = [
       (firstPoint[0] - this.x) / this.width,
       (firstPoint[1] - this.y) / this.height,
     ];
+    // The last point is in the pages coordinate system.
+    const { lastPoint } = this.#focusOutlines;
     this.#lastPoint = [
       (lastPoint[0] - this.x) / this.width,
       (lastPoint[1] - this.y) / this.height,
@@ -268,11 +269,12 @@ class HighlightEditor extends AnnotationEditor {
       }
     }
 
-    const { firstPoint, lastPoint } = this.#focusOutlines;
+    const { firstPoint } = highlightOutlines;
     this.#firstPoint = [
       (firstPoint[0] - x) / width,
       (firstPoint[1] - y) / height,
     ];
+    const { lastPoint } = this.#focusOutlines;
     this.#lastPoint = [(lastPoint[0] - x) / width, (lastPoint[1] - y) / height];
   }
 
@@ -1082,13 +1084,10 @@ class HighlightEditor extends AnnotationEditor {
       annotation.hide();
       return null;
     }
-    const params = {
+    annotation.updateEdited({
       rect: this.getPDFRect(),
-    };
-    if (this.hasEditedComment) {
-      params.popup = this.comment;
-    }
-    annotation.updateEdited(params);
+      popup: this.comment,
+    });
 
     return null;
   }

src/display/editor/ink.js

@@ -305,15 +305,12 @@ class InkEditor extends DrawingEditor {
       return null;
     }
     const { points, rect } = this.serializeDraw(/* isForCopying = */ false);
-    const params = {
+    annotation.updateEdited({
       rect,
       thickness: this._drawingOptions["stroke-width"],
       points,
-    };
-    if (this.hasEditedComment) {
-      params.popup = this.comment;
-    }
-    annotation.updateEdited(params);
+      popup: this.comment,
+    });
 
     return null;
   }

src/display/editor/stamp.js

@@ -942,13 +942,10 @@ class StampEditor extends AnnotationEditor {
       annotation.hide();
       return null;
     }
-    const params = {
+    annotation.updateEdited({
       rect: this.getPDFRect(),
-    };
-    if (this.hasEditedComment) {
-      params.popup = this.comment;
-    }
-    annotation.updateEdited(params);
+      popup: this.comment,
+    });
 
     return null;
   }

test/integration/comment_spec.mjs

@@ -15,16 +15,20 @@
 
 import {
   closePages,
+  dragAndDrop,
   getEditorSelector,
   getRect,
   getSpanRectFromText,
   loadAndWait,
   scrollIntoView,
+  selectEditor,
   switchToEditor,
   waitAndClick,
+  waitForSerialized,
 } from "./test_utils.mjs";
 
 const switchToHighlight = switchToEditor.bind(null, "Highlight");
+const switchToStamp = switchToEditor.bind(null, "Stamp");
 
 describe("Comment", () => {
   describe("Comment edit dialog must be visible in ltr", () => {
@@ -131,4 +135,105 @@ describe("Comment", () => {
       );
     });
   });
+
+  describe("Update comment position and color in reading mode", () => {
+    let pages;
+
+    beforeEach(async () => {
+      pages = await loadAndWait(
+        "comments.pdf",
+        ".annotationEditorLayer",
+        "page-fit",
+        null,
+        {
+          enableComment: true,
+          highlightEditorColors:
+            "yellow=#FFFF00,green=#00FF00,blue=#0000FF,pink=#FF00FF,red=#FF0000",
+        }
+      );
+    });
+
+    afterEach(async () => {
+      await closePages(pages);
+    });
+
+    it("must set the comment button at the right place", async () => {
+      await Promise.all(
+        pages.map(async ([browserName, page]) => {
+          await switchToStamp(page);
+
+          const stampSelector = getEditorSelector(8);
+          await selectEditor(page, stampSelector);
+          await dragAndDrop(page, stampSelector, [[100, 100]]);
+          await waitForSerialized(page, 1);
+          const rectCommentButton = await getRect(
+            page,
+            `${stampSelector} .annotationCommentButton`
+          );
+
+          await switchToStamp(page, /* disable = */ true);
+          const rectCommentButtonAfter = await getRect(
+            page,
+            `#pdfjs_internal_id_713R + .annotationCommentButton`
+          );
+
+          expect(Math.abs(rectCommentButtonAfter.x - rectCommentButton.x))
+            .withContext(`In ${browserName}`)
+            .toBeLessThanOrEqual(1);
+          expect(Math.abs(rectCommentButtonAfter.y - rectCommentButton.y))
+            .withContext(`In ${browserName}`)
+            .toBeLessThanOrEqual(1);
+          expect(
+            Math.abs(rectCommentButtonAfter.width - rectCommentButton.width)
+          )
+            .withContext(`In ${browserName}`)
+            .toBeLessThanOrEqual(1);
+          expect(
+            Math.abs(rectCommentButtonAfter.height - rectCommentButton.height)
+          )
+            .withContext(`In ${browserName}`)
+            .toBeLessThanOrEqual(1);
+        })
+      );
+    });
+
+    it("must set the right color to the comment button", async () => {
+      await Promise.all(
+        pages.map(async ([browserName, page]) => {
+          await switchToHighlight(page);
+
+          const highlightSelector = getEditorSelector(0);
+          await selectEditor(page, highlightSelector);
+          const colorButtonSelector = `${highlightSelector} .editToolbar button`;
+          await page.waitForSelector(`${colorButtonSelector}.colorPicker`);
+          await page.click(`${colorButtonSelector}.colorPicker`);
+          await page.waitForSelector(`${colorButtonSelector}[title = "Red"]`);
+          await page.click(`${colorButtonSelector}[title = "Red"]`);
+          await page.waitForSelector(
+            `.page[data-page-number = "1"] svg.highlight[fill = "#FF0000"]`
+          );
+
+          const commentButtonColor = await page.evaluate(selector => {
+            const button = document.querySelector(
+              `${selector} .annotationCommentButton`
+            );
+            return window.getComputedStyle(button).backgroundColor;
+          }, highlightSelector);
+
+          await switchToHighlight(page, /* disable = */ true);
+
+          const commentButtonColorAfter = await page.evaluate(() => {
+            const button = document.querySelector(
+              "section[data-annotation-id='612R'] + .annotationCommentButton"
+            );
+            return window.getComputedStyle(button).backgroundColor;
+          });
+
+          expect(commentButtonColorAfter)
+            .withContext(`In ${browserName}`)
+            .toEqual(commentButtonColor);
+        })
+      );
+    });
+  });
 });

test/pdfs/.gitignore

@@ -745,3 +745,4 @@
 !tracemonkey_annotation_on_page_8.pdf
 !issue20232.pdf
 !bug1989304.pdf
+!comments.pdf