Merge pull request #20344 from calixteman/bug1992770

[Editor] Make sure that annotation positions in the DOM respect the visual order (bug 1992770)

Author: calixteman

src/display/annotation_layer.js

@@ -186,7 +186,8 @@ class AnnotationElement {
     this.parent = parameters.parent;
 
     if (isRenderable) {
-      this.container = this._createContainer(ignoreBorder);
+      this.contentElement = this.container =
+        this._createContainer(ignoreBorder);
     }
     if (createQuadrilaterals) {
       this._createQuadrilaterals();
@@ -1008,6 +1009,7 @@ class LinkAnnotationElement extends AnnotationElement {
 
     this.container.classList.add("linkAnnotation");
     if (isBound) {
+      this.contentElement = link;
       this.container.append(link);
     }
 
@@ -1517,6 +1519,7 @@ class TextWidgetAnnotationElement extends WidgetAnnotationElement {
         element.hidden = true;
       }
       GetElementsByNameSet.add(element);
+      this.contentElement = element;
       element.setAttribute("data-element-id", id);
 
       element.disabled = this.data.readOnly;
@@ -3070,7 +3073,7 @@ class FreeTextAnnotationElement extends AnnotationElement {
     this.container.classList.add("freeTextAnnotation");
 
     if (this.textContent) {
-      const content = document.createElement("div");
+      const content = (this.contentElement = document.createElement("div"));
       content.classList.add("annotationTextContent");
       content.setAttribute("role", "comment");
       for (const line of this.textContent) {
@@ -3787,7 +3790,7 @@ class AnnotationLayer {
   }
 
   async #appendElement(element, id, popupElements) {
-    const contentElement = element.firstChild || element;
+    const { contentElement, container } = element;
     const annotationId = (contentElement.id = `${AnnotationPrefix}${id}`);
     const ariaAttributes =
       await this.#structTreeLayer?.getAriaAttributes(annotationId);
@@ -3799,18 +3802,31 @@ class AnnotationLayer {
 
     if (popupElements) {
       // Set the popup just after the first element associated with the popup.
-      popupElements.at(-1).container.after(element);
+      popupElements.at(-1).container.after(container);
     } else {
-      this.div.append(element);
-      this.#accessibilityManager?.moveElementInDOM(
-        this.div,
-        element,
-        contentElement,
-        /* isRemovable = */ false
-      );
+      this.#moveElementInDOM(container, contentElement);
     }
   }
 
+  #moveElementInDOM(container, contentElement) {
+    this.div.append(container);
+    this.#accessibilityManager?.moveElementInDOM(
+      this.div,
+      container,
+      contentElement,
+      /* isRemovable = */ false,
+      /* filter = */ node => node.nodeName === "SECTION",
+      /* inserter = */ (prevNode, node) => {
+        if (prevNode.nextElementSibling.nodeName === "BUTTON") {
+          // In case we have a comment button, insert after the button.
+          prevNode.nextElementSibling.after(node);
+        } else {
+          prevNode.after(node);
+        }
+      }
+    );
+  }
+
   /**
    * Render a new annotation layer with all annotation elements.
    *
@@ -3877,7 +3893,7 @@ class AnnotationLayer {
       if (data.hidden) {
         rendered.style.visibility = "hidden";
       }
-      await this.#appendElement(rendered, data.id, elementParams.elements);
+      await this.#appendElement(element, data.id, elementParams.elements);
       element.extraPopupElement?.popup?.renderCommentButton();
 
       if (element._isEditable) {
@@ -3913,8 +3929,8 @@ class AnnotationLayer {
       if (!element.isRenderable) {
         continue;
       }
-      const rendered = element.render();
-      await this.#appendElement(rendered, data.id, null);
+      element.render();
+      await this.#appendElement(element, data.id, null);
     }
   }
 
@@ -3999,18 +4015,32 @@ class AnnotationLayer {
       linkService: this.#linkService,
       annotationStorage: this.#annotationStorage,
     });
-    const htmlElement = element.render();
-    div.append(htmlElement);
-    this.#accessibilityManager?.moveElementInDOM(
-      div,
-      htmlElement,
-      htmlElement,
-      /* isRemovable = */ false
-    );
+    const rendered = element.render();
+    rendered.id = `${AnnotationPrefix}${id}`;
+    this.#moveElementInDOM(rendered, rendered);
     element.createOrUpdatePopup();
     return element;
   }
 
+  togglePointerEvents(enabled = false) {
+    this.div.classList.toggle("disabled", !enabled);
+  }
+
+  updateFakeAnnotations(editors) {
+    if (editors.length === 0) {
+      return;
+    }
+    // In order to ensure that the annotations are correctly moved in the DOM
+    // we need to make sure that this has been laid out.
+    window.requestAnimationFrame(() =>
+      setTimeout(() => {
+        for (const editor of editors) {
+          editor.updateFakeAnnotationElement(this);
+        }
+      }, 10)
+    );
+  }
+
   /**
    * @private
    */

src/display/editor/annotation_editor_layer.js

@@ -242,7 +242,7 @@ class AnnotationEditorLayer {
   }
 
   toggleAnnotationLayerPointerEvents(enabled = false) {
-    this.#annotationLayer?.div.classList.toggle("disabled", !enabled);
+    this.#annotationLayer?.togglePointerEvents(enabled);
   }
 
   get #allEditorsIterator() {
@@ -354,13 +354,14 @@ class AnnotationEditorLayer {
     }
 
     const annotationLayer = this.#annotationLayer;
+    const needFakeAnnotation = [];
     if (annotationLayer) {
       const changedAnnotations = new Map();
       const resetAnnotations = new Map();
       for (const editor of this.#allEditorsIterator) {
         editor.disableEditing();
         if (!editor.annotationElementId) {
-          editor.updateFakeAnnotationElement(annotationLayer);
+          needFakeAnnotation.push(editor);
           continue;
         }
         if (editor.serialize() !== null) {
@@ -411,6 +412,7 @@ class AnnotationEditorLayer {
     }
     this.disableTextSelection();
     this.toggleAnnotationLayerPointerEvents(true);
+    annotationLayer?.updateFakeAnnotations(needFakeAnnotation);
 
     this.#isDisabling = false;
   }

test/integration/annotation_spec.mjs

@@ -95,7 +95,7 @@ describe("Annotation highlight", () => {
         pages.map(async ([browserName, page]) => {
           for (const i of [23, 22, 14]) {
             await page.click(getAnnotationSelector(`${i}R`));
-            await page.waitForSelector(`#pdfjs_internal_id_${i}R:focus`);
+            await page.waitForSelector(`#pdfjs_internal_id_${i}R:focus-within`);
           }
         })
       );
@@ -878,4 +878,46 @@ a dynamic compiler for JavaScript based on our`);
       });
     });
   });
+
+  describe("Annotation order in the DOM", () => {
+    let pages;
+
+    beforeEach(async () => {
+      pages = await loadAndWait(
+        "comments.pdf",
+        ".page[data-page-number='1'] .annotationLayer #pdfjs_internal_id_661R"
+      );
+    });
+
+    afterEach(async () => {
+      await closePages(pages);
+    });
+
+    it("must check that annotations are in the visual order", async () => {
+      await Promise.all(
+        pages.map(async ([browserName, page]) => {
+          const sectionIds = await page.evaluate(() =>
+            [
+              ...document.querySelectorAll(
+                ".page[data-page-number='1'] .annotationLayer > section:not(.popupAnnotation)"
+              ),
+            ].map(el => el.id.split("_").pop())
+          );
+          expect(sectionIds)
+            .withContext(`In ${browserName}`)
+            .toEqual([
+              "612R",
+              "693R",
+              "687R",
+              "690R",
+              "713R",
+              "673R",
+              "613R",
+              "680R",
+              "661R",
+            ]);
+        })
+      );
+    });
+  });
 });

test/integration/comment_spec.mjs

@@ -737,4 +737,65 @@ describe("Comment", () => {
       );
     });
   });
+
+  describe("Annotations order in reading mode", () => {
+    let pages;
+
+    beforeEach(async () => {
+      pages = await loadAndWait(
+        "comments.pdf",
+        ".annotationEditorLayer",
+        "page-fit",
+        null,
+        { enableComment: true }
+      );
+    });
+
+    afterEach(async () => {
+      await closePages(pages);
+    });
+
+    it("must check that the annotations are in the right order", async () => {
+      await Promise.all(
+        pages.map(async ([browserName, page]) => {
+          await switchToHighlight(page);
+          await highlightSpan(
+            page,
+            1,
+            "method provides cheap inter-procedural type specialization, and an"
+          );
+          await editComment(page, getEditorSelector(9), "Hello world!");
+
+          await highlightSpan(page, 1, "Andreas Gal");
+          await editComment(page, getEditorSelector(10), "Hello world!");
+
+          await switchToHighlight(page, /* disable = */ true);
+          await page.waitForSelector(
+            ".annotationLayer section:nth-child(4).editorAnnotation"
+          );
+
+          const sectionIds = await page.evaluate(() =>
+            [
+              ...document.querySelectorAll(
+                ".page[data-page-number='1'] .annotationLayer > section:not(.popupAnnotation)"
+              ),
+            ].map(el => el.id.split("_").pop())
+          );
+          expect(sectionIds).withContext(`In ${browserName}`).toEqual([
+            "612R",
+            "693R",
+            "10", // shortcut for pdfjs_internal_id_pdfjs_internal_editor_10
+            "687R",
+            "690R",
+            "713R",
+            "9", // shortcut for pdfjs_internal_id_pdfjs_internal_editor_9
+            "673R",
+            "613R",
+            "680R",
+            "661R",
+          ]);
+        })
+      );
+    });
+  });
 });

web/text_accessibility.js

@@ -222,7 +222,14 @@ class TextAccessibilityManager {
    * @param {HTMLDivElement} element
    * @returns {string|null} The id in the struct tree if any.
    */
-  moveElementInDOM(container, element, contentElement, isRemovable) {
+  moveElementInDOM(
+    container,
+    element,
+    contentElement,
+    isRemovable,
+    filter,
+    inserter
+  ) {
     const id = this.addPointerInTextLayer(contentElement, isRemovable);
 
     if (!container.hasChildNodes()) {
@@ -231,25 +238,23 @@ class TextAccessibilityManager {
     }
 
     const children = Array.from(container.childNodes).filter(
-      node => node !== element
+      node => node !== element && (!filter || filter(node))
     );
 
     if (children.length === 0) {
       return id;
     }
 
-    const elementToCompare = contentElement || element;
     const index = binarySearchFirstItem(
       children,
       node =>
-        TextAccessibilityManager.#compareElementPositions(
-          elementToCompare,
-          node
-        ) < 0
+        TextAccessibilityManager.#compareElementPositions(element, node) < 0
     );
 
     if (index === 0) {
       children[0].before(element);
+    } else if (inserter) {
+      inserter(children[index - 1], element);
     } else {
       children[index - 1].after(element);
     }