Merge pull request #20077 from calixteman/remove_popup

[Editor] Remove a popup from the DOM when it's deleted

Author: timvandermeij

src/display/annotation_layer.js

@@ -2125,6 +2125,8 @@ class PopupElement {
 
   #popup = null;
 
+  #popupAbortController = null;
+
   #position = null;
 
   #rect = null;
@@ -2166,18 +2168,7 @@ class PopupElement {
     this.#dateObj = PDFDateString.toDateObject(modificationDate);
 
     this.trigger = elements.flatMap(e => e.getElementsToTriggerPopup());
-    // Attach the event listeners to the trigger element.
-    for (const element of this.trigger) {
-      element.addEventListener("click", this.#boundToggle);
-      element.addEventListener("mouseenter", this.#boundShow);
-      element.addEventListener("mouseleave", this.#boundHide);
-      element.classList.add("popupTriggerArea");
-    }
-
-    // Attach the event listener to toggle the popup with the keyboard.
-    for (const element of elements) {
-      element.container?.addEventListener("keydown", this.#boundKeyDown);
-    }
+    this.#addEventListeners();
 
     this.#container.hidden = true;
     if (open) {
@@ -2195,6 +2186,29 @@ class PopupElement {
     }
   }
 
+  #addEventListeners() {
+    if (this.#popupAbortController) {
+      return;
+    }
+    this.#popupAbortController = new AbortController();
+    const { signal } = this.#popupAbortController;
+
+    // Attach the event listeners to the trigger element.
+    for (const element of this.trigger) {
+      element.addEventListener("click", this.#boundToggle, { signal });
+      element.addEventListener("mouseenter", this.#boundShow, { signal });
+      element.addEventListener("mouseleave", this.#boundHide, { signal });
+      element.classList.add("popupTriggerArea");
+    }
+
+    // Attach the event listener to toggle the popup with the keyboard.
+    for (const element of this.#elements) {
+      element.container?.addEventListener("keydown", this.#boundKeyDown, {
+        signal,
+      });
+    }
+  }
+
   render() {
     if (this.#popup) {
       return;
@@ -2338,7 +2352,12 @@ class PopupElement {
     }
   }
 
-  updateEdited({ rect, popupContent }) {
+  updateEdited({ rect, popupContent, deleted }) {
+    if (deleted) {
+      this.remove();
+      return;
+    }
+    this.#addEventListeners();
     this.#updates ||= {
       contentsObj: this.#contentsObj,
       richText: this.#richText,
@@ -2366,6 +2385,18 @@ class PopupElement {
     this.#position = null;
   }
 
+  remove() {
+    this.#popupAbortController?.abort();
+    this.#popupAbortController = null;
+    this.#popup?.remove();
+    this.#popup = null;
+    this.#wasVisible = false;
+    this.#pinned = false;
+    for (const element of this.trigger) {
+      element.classList.remove("popupTriggerArea");
+    }
+  }
+
   #setPosition() {
     if (this.#position !== null) {
       return;
@@ -2465,6 +2496,7 @@ class PopupElement {
   }
 
   maybeShow() {
+    this.#addEventListeners();
     if (!this.#wasVisible) {
       return;
     }

src/display/editor/annotation_editor_layer.js

@@ -359,6 +359,9 @@ class AnnotationEditorLayer {
       for (const editable of editables) {
         const { id } = editable.data;
         if (this.#uiManager.isDeletedAnnotationElement(id)) {
+          editable.updateEdited({
+            deleted: true,
+          });
           continue;
         }
         let editor = resetAnnotations.get(id);

src/display/editor/freetext.js

@@ -860,9 +860,6 @@ class FreeTextEditor extends AnnotationEditor {
   /** @inheritdoc */
   renderAnnotationElement(annotation) {
     const content = super.renderAnnotationElement(annotation);
-    if (this.deleted) {
-      return content;
-    }
     const { style } = content;
     style.fontSize = `calc(${this.#fontSize}px * var(--total-scale-factor))`;
     style.color = this.#color;

test/integration/freetext_editor_spec.mjs

@@ -1086,6 +1086,61 @@ describe("FreeText Editor", () => {
         })
       );
     });
+
+    it("must delete an existing annotation with a popup", async () => {
+      await Promise.all(
+        pages.map(async ([browserName, page]) => {
+          await page.click("[data-annotation-id='26R']");
+          // Wait for the popup to be displayed.
+          const popupSelector = "[data-annotation-id='popup_26R'] .popup";
+          await page.waitForSelector(popupSelector, { visible: true });
+
+          await switchToFreeText(page);
+
+          const editorSelector = getEditorSelector(0);
+          await selectEditor(page, editorSelector);
+          await page.keyboard.press("Backspace");
+          await page.waitForFunction(
+            sel => !document.querySelector(sel),
+            {},
+            editorSelector
+          );
+
+          await waitForSerialized(page, 1);
+          const serialized = await getSerialized(page);
+          expect(serialized).toEqual([
+            {
+              pageIndex: 0,
+              id: "26R",
+              deleted: true,
+              popupRef: "",
+            },
+          ]);
+
+          // Disable editing mode.
+          await switchToFreeText(page, /* disable = */ true);
+
+          await page.waitForSelector(":not([data-annotation-id='26R'] .popup)");
+
+          // Re-enable editing mode.
+          await switchToFreeText(page);
+          await page.focus(".annotationEditorLayer");
+
+          await kbUndo(page);
+          await waitForSerialized(page, 0);
+
+          // Disable editing mode.
+          await switchToFreeText(page, /* disable = */ true);
+
+          const popupAreaSelector =
+            "[data-annotation-id='26R'].popupTriggerArea";
+          await page.waitForSelector(popupAreaSelector, { visible: true });
+          await page.click("[data-annotation-id='26R']");
+          // Wait for the popup to be displayed.
+          await page.waitForSelector(popupSelector, { visible: true });
+        })
+      );
+    });
   });
 
   describe("FreeText (copy/paste existing)", () => {