From 41dea1e38bbc19f89d13681e3cb8aeae3a17c23a Mon Sep 17 00:00:00 2001 From: Calixte Denizet Date: Tue, 7 Oct 2025 21:01:03 +0200 Subject: [PATCH] [Editor] Make sure that annotation positions in the DOM respect the visual order (bug 1992770) --- src/display/annotation_layer.js | 74 +++++++++++++------ src/display/editor/annotation_editor_layer.js | 6 +- test/integration/annotation_spec.mjs | 44 ++++++++++- test/integration/comment_spec.mjs | 61 +++++++++++++++ web/text_accessibility.js | 19 +++-- 5 files changed, 172 insertions(+), 32 deletions(-) diff --git a/src/display/annotation_layer.js b/src/display/annotation_layer.js index 21030c451..638f596f5 100644 --- a/src/display/annotation_layer.js +++ b/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 */ diff --git a/src/display/editor/annotation_editor_layer.js b/src/display/editor/annotation_editor_layer.js index 3f48c099a..ec73d4ece 100644 --- a/src/display/editor/annotation_editor_layer.js +++ b/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; } diff --git a/test/integration/annotation_spec.mjs b/test/integration/annotation_spec.mjs index cb5f5ec13..a5dcea58c 100644 --- a/test/integration/annotation_spec.mjs +++ b/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", + ]); + }) + ); + }); + }); }); diff --git a/test/integration/comment_spec.mjs b/test/integration/comment_spec.mjs index a08e63544..59fda6b91 100644 --- a/test/integration/comment_spec.mjs +++ b/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", + ]); + }) + ); + }); + }); }); diff --git a/web/text_accessibility.js b/web/text_accessibility.js index e9178baad..91450e7a5 100644 --- a/web/text_accessibility.js +++ b/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); }