From f5a6dd4164bddec7e329792864bcd88ffb32a1f7 Mon Sep 17 00:00:00 2001 From: Calixte Denizet Date: Wed, 8 Oct 2025 18:45:19 +0200 Subject: [PATCH] [Annotation] Use the annotations rect in order to fix the order in the DOM (bug 1987914) It's just a partial fix for bug 1987914 but the time spent to add the annotations in the DOM is divided by 5. --- src/display/annotation_layer.js | 259 +++++++++++++++++------- test/integration/accessibility_spec.mjs | 5 +- web/text_accessibility.js | 13 +- 3 files changed, 192 insertions(+), 85 deletions(-) diff --git a/src/display/annotation_layer.js b/src/display/annotation_layer.js index 638f596f5..7d21be7f4 100644 --- a/src/display/annotation_layer.js +++ b/src/display/annotation_layer.js @@ -184,6 +184,7 @@ class AnnotationElement { this.hasJSActions = parameters.hasJSActions; this._fieldObjects = parameters.fieldObjects; this.parent = parameters.parent; + this.hasOwnCommentButton = false; if (isRenderable) { this.contentElement = this.container = @@ -747,7 +748,7 @@ class AnnotationElement { contentsObj = data.contentsObj; modificationDate = data.modificationDate; } - const popup = (this.#popupElement = new PopupAnnotationElement({ + this.#popupElement = new PopupAnnotationElement({ data: { color: data.color, titleObj: data.titleObj, @@ -763,10 +764,7 @@ class AnnotationElement { linkService: this.linkService, parent: this.parent, elements: [this], - })); - if (!this.parent._commentManager) { - this.parent.div.append(popup.render()); - } + }); } get hasPopupElement() { @@ -916,7 +914,6 @@ class EditorAnnotationElement extends AnnotationElement { return; } this._createPopup(editor.comment); - this.extraPopupElement.popup.renderCommentButton(); } get hasCommentButton() { @@ -943,6 +940,7 @@ class EditorAnnotationElement extends AnnotationElement { } remove() { + this.parent.removeAnnotation(this.data.id); this.container.remove(); this.container = null; this.removePopup(); @@ -1274,6 +1272,7 @@ class TextAnnotationElement extends AnnotationElement { ); if (!this.data.popupRef && this.hasPopupData) { + this.hasOwnCommentButton = true; this._createPopup(); } @@ -2493,9 +2492,7 @@ class PopupElement { // The elements that will trigger the popup. this.trigger = elements.flatMap(e => e.getElementsToTriggerPopup()); - if (commentManager) { - this.renderCommentButton(); - } else { + if (!commentManager) { this.#addEventListeners(); this.#container.hidden = true; @@ -2550,6 +2547,9 @@ class PopupElement { renderCommentButton() { if (this.#commentButton) { + if (!this.#commentButton.parentNode) { + this.#firstElement.container.after(this.#commentButton); + } return; } @@ -2562,7 +2562,7 @@ class PopupElement { } const { signal } = (this.#popupAbortController = new AbortController()); - const hasOwnButton = !!this.#firstElement.extraPopupElement; + const hasOwnButton = this.#firstElement.hasOwnCommentButton; const togglePopup = () => { this.#commentManager.toggleCommentPopup( this, @@ -2623,7 +2623,9 @@ class PopupElement { // button position can't be changed. return; } - this.renderCommentButton(); + if (!this.#commentButton) { + this.renderCommentButton(); + } const [x, y] = this.#commentButtonPosition; const { style } = this.#commentButton; style.left = `calc(${x}%)`; @@ -2634,7 +2636,9 @@ class PopupElement { if (this.#firstElement.extraPopupElement) { return; } - this.renderCommentButton(); + if (!this.#commentButton) { + this.renderCommentButton(); + } this.#commentButton.style.backgroundColor = this.commentButtonColor || ""; } @@ -3085,6 +3089,7 @@ class FreeTextAnnotationElement extends AnnotationElement { } if (!this.data.popupRef && this.hasPopupData) { + this.hasOwnCommentButton = true; this._createPopup(); } @@ -3133,6 +3138,7 @@ class LineAnnotationElement extends AnnotationElement { // Create the popup ourselves so that we can bind it to the line instead // of to the entire container (which is the default). if (!data.popupRef && this.hasPopupData) { + this.hasOwnCommentButton = true; this._createPopup(); } @@ -3189,6 +3195,7 @@ class SquareAnnotationElement extends AnnotationElement { // Create the popup ourselves so that we can bind it to the square instead // of to the entire container (which is the default). if (!data.popupRef && this.hasPopupData) { + this.hasOwnCommentButton = true; this._createPopup(); } @@ -3246,6 +3253,7 @@ class CircleAnnotationElement extends AnnotationElement { // Create the popup ourselves so that we can bind it to the circle instead // of to the entire container (which is the default). if (!data.popupRef && this.hasPopupData) { + this.hasOwnCommentButton = true; this._createPopup(); } @@ -3319,6 +3327,7 @@ class PolylineAnnotationElement extends AnnotationElement { // Create the popup ourselves so that we can bind it to the polyline // instead of to the entire container (which is the default). if (!popupRef && this.hasPopupData) { + this.hasOwnCommentButton = true; this._createPopup(); } @@ -3353,6 +3362,7 @@ class CaretAnnotationElement extends AnnotationElement { this.container.classList.add("caretAnnotation"); if (!this.data.popupRef && this.hasPopupData) { + this.hasOwnCommentButton = true; this._createPopup(); } return this.container; @@ -3447,6 +3457,7 @@ class InkAnnotationElement extends AnnotationElement { } if (!popupRef && this.hasPopupData) { + this.hasOwnCommentButton = true; this._createPopup(); } @@ -3503,6 +3514,7 @@ class HighlightAnnotationElement extends AnnotationElement { data: { overlaidText, popupRef }, } = this; if (!popupRef && this.hasPopupData) { + this.hasOwnCommentButton = true; this._createPopup(); } @@ -3534,6 +3546,7 @@ class UnderlineAnnotationElement extends AnnotationElement { data: { overlaidText, popupRef }, } = this; if (!popupRef && this.hasPopupData) { + this.hasOwnCommentButton = true; this._createPopup(); } @@ -3564,6 +3577,7 @@ class SquigglyAnnotationElement extends AnnotationElement { data: { overlaidText, popupRef }, } = this; if (!popupRef && this.hasPopupData) { + this.hasOwnCommentButton = true; this._createPopup(); } @@ -3594,6 +3608,7 @@ class StrikeOutAnnotationElement extends AnnotationElement { data: { overlaidText, popupRef }, } = this; if (!popupRef && this.hasPopupData) { + this.hasOwnCommentButton = true; this._createPopup(); } @@ -3621,6 +3636,7 @@ class StampAnnotationElement extends AnnotationElement { this.container.setAttribute("role", "img"); if (!this.data.popupRef && this.hasPopupData) { + this.hasOwnCommentButton = true; this._createPopup(); } this._editOnDoubleClick(); @@ -3684,6 +3700,7 @@ class FileAttachmentAnnotationElement extends AnnotationElement { }); if (!data.popupRef && this.hasPopupData) { + this.hasOwnCommentButton = true; this._createPopup(); } else { trigger.classList.add("popupTriggerArea"); @@ -3748,6 +3765,10 @@ class AnnotationLayer { #linkService = null; + #elements = []; + + #hasAriaAttributesFromStructTree = false; + constructor({ div, accessibilityManager, @@ -3789,44 +3810,6 @@ class AnnotationLayer { return this.#editableAnnotations.size > 0; } - async #appendElement(element, id, popupElements) { - const { contentElement, container } = element; - const annotationId = (contentElement.id = `${AnnotationPrefix}${id}`); - const ariaAttributes = - await this.#structTreeLayer?.getAriaAttributes(annotationId); - if (ariaAttributes) { - for (const [key, value] of ariaAttributes) { - contentElement.setAttribute(key, value); - } - } - - if (popupElements) { - // Set the popup just after the first element associated with the popup. - popupElements.at(-1).container.after(container); - } else { - 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. * @@ -3839,6 +3822,7 @@ class AnnotationLayer { setLayerDimensions(layer, this.viewport); const popupToElements = new Map(); + const popupAnnotations = []; const elementParams = { data: null, layer, @@ -3871,6 +3855,10 @@ class AnnotationLayer { // Ignore popup annotations without a corresponding annotation. continue; } + if (!this._commentManager) { + popupAnnotations.push(data); + continue; + } elementParams.elements = elements; } elementParams.data = data; @@ -3880,12 +3868,16 @@ class AnnotationLayer { continue; } - if (!isPopupAnnotation && data.popupRef) { - const elements = popupToElements.get(data.popupRef); - if (!elements) { - popupToElements.set(data.popupRef, [element]); - } else { - elements.push(element); + if (!isPopupAnnotation) { + this.#elements.push(element); + + if (data.popupRef) { + const elements = popupToElements.get(data.popupRef); + if (!elements) { + popupToElements.set(data.popupRef, [element]); + } else { + elements.push(element); + } } } @@ -3893,8 +3885,6 @@ class AnnotationLayer { if (data.hidden) { rendered.style.visibility = "hidden"; } - await this.#appendElement(element, data.id, elementParams.elements); - element.extraPopupElement?.popup?.renderCommentButton(); if (element._isEditable) { this.#editableAnnotations.set(element.data.id, element); @@ -3902,9 +3892,124 @@ class AnnotationLayer { } } + await this.#addElementsToDOM(); + + for (const data of popupAnnotations) { + const elements = (elementParams.elements = popupToElements.get(data.id)); + elementParams.data = data; + const element = AnnotationElementFactory.create(elementParams); + if (!element.isRenderable) { + continue; + } + const rendered = element.render(); + element.contentElement.id = `${AnnotationPrefix}${data.id}`; + if (data.hidden) { + rendered.style.visibility = "hidden"; + } + elements.at(-1).container.after(rendered); + } + this.#setAnnotationCanvasMap(); } + async #addElementsToDOM() { + if (this.#elements.length === 0) { + return; + } + // Clear the existing annotations in order to make sure comment buttons + // don't have a parent any longer. + this.div.replaceChildren(); + + const promises = []; + if (!this.#hasAriaAttributesFromStructTree) { + this.#hasAriaAttributesFromStructTree = true; + for (const { + contentElement, + data: { id }, + } of this.#elements) { + const annotationId = (contentElement.id = `${AnnotationPrefix}${id}`); + promises.push( + this.#structTreeLayer + ?.getAriaAttributes(annotationId) + .then(ariaAttributes => { + if (ariaAttributes) { + for (const [key, value] of ariaAttributes) { + contentElement.setAttribute(key, value); + } + } + }) + ); + } + } + this.#elements.sort( + ( + { + data: { + rect: [a0, a1, a2, a3], + }, + }, + { + data: { + rect: [b0, b1, b2, b3], + }, + } + ) => { + // We are in page coordinates, which has the origin at the + // bottom left. + if (a0 === a2 && a1 === a3) { + return +1; + } + + if (b0 === b2 && b1 === b3) { + return -1; + } + + const top1 = a3; + const bot1 = a1; + const mid1 = (a1 + a3) / 2; + + const top2 = b3; + const bot2 = b1; + const mid2 = (b1 + b3) / 2; + + if (mid1 >= top2 && mid2 <= bot1) { + return -1; + } + + if (mid2 >= top1 && mid1 <= bot2) { + return +1; + } + + const centerX1 = (a0 + a2) / 2; + const centerX2 = (b0 + b2) / 2; + + return centerX1 - centerX2; + } + ); + + const fragment = document.createDocumentFragment(); + for (const element of this.#elements) { + fragment.append(element.container); + if (this._commentManager) { + ( + element.extraPopupElement?.popup || element.popup + )?.renderCommentButton(); + } else if (element.extraPopupElement) { + fragment.append(element.extraPopupElement.render()); + } + } + this.div.append(fragment); + await Promise.all(promises); + if (this.#accessibilityManager) { + for (const element of this.#elements) { + this.#accessibilityManager.addPointerInTextLayer( + element.contentElement, + /* isRemovable = */ false + ); + } + } + } + /** * Add link annotations to the annotation layer. * @@ -3930,8 +4035,10 @@ class AnnotationLayer { continue; } element.render(); - await this.#appendElement(element, data.id, null); + element.contentElement.id = `${AnnotationPrefix}${data.id}`; + this.#elements.push(element); } + await this.#addElementsToDOM(); } /** @@ -4015,30 +4122,36 @@ class AnnotationLayer { linkService: this.#linkService, annotationStorage: this.#annotationStorage, }); - const rendered = element.render(); - rendered.id = `${AnnotationPrefix}${id}`; - this.#moveElementInDOM(rendered, rendered); + element.render(); + element.contentElement.id = `${AnnotationPrefix}${id}`; element.createOrUpdatePopup(); + this.#elements.push(element); return element; } - togglePointerEvents(enabled = false) { - this.div.classList.toggle("disabled", !enabled); + removeAnnotation(id) { + const index = this.#elements.findIndex(el => el.data.id === id); + if (index < 0) { + return; + } + const [element] = this.#elements.splice(index, 1); + this.#accessibilityManager?.removePointerInTextLayer( + element.contentElement + ); } 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) - ); + for (const editor of editors) { + editor.updateFakeAnnotationElement(this); + } + this.#addElementsToDOM(); + } + + togglePointerEvents(enabled = false) { + this.div.classList.toggle("disabled", !enabled); } /** diff --git a/test/integration/accessibility_spec.mjs b/test/integration/accessibility_spec.mjs index dd5e13cf0..5dfca3785 100644 --- a/test/integration/accessibility_spec.mjs +++ b/test/integration/accessibility_spec.mjs @@ -181,7 +181,10 @@ describe("accessibility", () => { let pages; beforeEach(async () => { - pages = await loadAndWait("tagged_stamp.pdf", ".annotationLayer"); + pages = await loadAndWait( + "tagged_stamp.pdf", + ".annotationLayer #pdfjs_internal_id_21R" + ); }); afterEach(async () => { diff --git a/web/text_accessibility.js b/web/text_accessibility.js index 91450e7a5..001493a80 100644 --- a/web/text_accessibility.js +++ b/web/text_accessibility.js @@ -222,14 +222,7 @@ class TextAccessibilityManager { * @param {HTMLDivElement} element * @returns {string|null} The id in the struct tree if any. */ - moveElementInDOM( - container, - element, - contentElement, - isRemovable, - filter, - inserter - ) { + moveElementInDOM(container, element, contentElement, isRemovable) { const id = this.addPointerInTextLayer(contentElement, isRemovable); if (!container.hasChildNodes()) { @@ -238,7 +231,7 @@ class TextAccessibilityManager { } const children = Array.from(container.childNodes).filter( - node => node !== element && (!filter || filter(node)) + node => node !== element ); if (children.length === 0) { @@ -253,8 +246,6 @@ class TextAccessibilityManager { if (index === 0) { children[0].before(element); - } else if (inserter) { - inserter(children[index - 1], element); } else { children[index - 1].after(element); }