From 8410252eb8d3f008782464d56f25cd5aa611edd6 Mon Sep 17 00:00:00 2001 From: Calixte Denizet Date: Wed, 2 Oct 2024 14:36:35 +0200 Subject: [PATCH] [Editor] Make stamp annotations editable (bug 1921291) --- src/core/annotation.js | 22 +++- src/display/annotation_layer.js | 7 +- src/display/editor/annotation_editor_layer.js | 5 + src/display/editor/stamp.js | 105 ++++++++++++++++- src/display/editor/toolbar.js | 2 +- src/display/editor/tools.js | 25 +++- test/integration/stamp_editor_spec.mjs | 107 ++++++++++++++++++ test/integration/test_utils.mjs | 5 + web/annotation_editor_layer_builder.css | 3 +- web/annotation_editor_layer_builder.js | 7 ++ web/pdf_page_view.js | 11 +- 11 files changed, 281 insertions(+), 18 deletions(-) diff --git a/src/core/annotation.js b/src/core/annotation.js index 379a89de42af1..8f415e9c050a5 100644 --- a/src/core/annotation.js +++ b/src/core/annotation.js @@ -4859,14 +4859,34 @@ class StrikeOutAnnotation extends MarkupAnnotation { } class StampAnnotation extends MarkupAnnotation { + #savedHasOwnCanvas; + constructor(params) { super(params); this.data.annotationType = AnnotationType.STAMP; - this.data.hasOwnCanvas = this.data.noRotate; + this.#savedHasOwnCanvas = this.data.hasOwnCanvas = this.data.noRotate; + this.data.isEditable = !this.data.noHTML; + // We want to be able to add mouse listeners to the annotation. this.data.noHTML = false; } + mustBeViewedWhenEditing(isEditing, modifiedIds = null) { + if (isEditing) { + if (!this.data.isEditable) { + return false; + } + // When we're editing, we want to ensure that the stamp annotation is + // drawn on a canvas in order to use it in the annotation editor layer. + this.#savedHasOwnCanvas = this.data.hasOwnCanvas; + this.data.hasOwnCanvas = true; + return true; + } + this.data.hasOwnCanvas = this.#savedHasOwnCanvas; + + return !modifiedIds?.has(this.data.id); + } + static async createImage(bitmap, xref) { // TODO: when printing, we could have a specific internal colorspace // (e.g. something like DeviceRGBA) in order avoid any conversion (i.e. no diff --git a/src/display/annotation_layer.js b/src/display/annotation_layer.js index 5b8eff0254350..aac732c27e99c 100644 --- a/src/display/annotation_layer.js +++ b/src/display/annotation_layer.js @@ -2863,10 +2863,8 @@ class InkAnnotationElement extends AnnotationElement { } this.container.append(svg); + this._editOnDoubleClick(); - if (this._isEditable) { - this._editOnDoubleClick(); - } return this.container; } @@ -2961,6 +2959,7 @@ class StrikeOutAnnotationElement extends AnnotationElement { class StampAnnotationElement extends AnnotationElement { constructor(parameters) { super(parameters, { isRenderable: true, ignoreBorder: true }); + this.annotationEditorType = AnnotationEditorType.STAMP; } render() { @@ -2970,6 +2969,8 @@ class StampAnnotationElement extends AnnotationElement { if (!this.data.popupRef && this.hasPopupData) { this._createPopup(); } + this._editOnDoubleClick(); + return this.container; } } diff --git a/src/display/editor/annotation_editor_layer.js b/src/display/editor/annotation_editor_layer.js index 2328940d34a27..2021b6bbdb398 100644 --- a/src/display/editor/annotation_editor_layer.js +++ b/src/display/editor/annotation_editor_layer.js @@ -22,6 +22,8 @@ // eslint-disable-next-line max-len /** @typedef {import("../annotation_layer.js").AnnotationLayer} AnnotationLayer */ /** @typedef {import("../draw_layer.js").DrawLayer} DrawLayer */ +// eslint-disable-next-line max-len +/** @typedef {import("../src/display/struct_tree_layer_builder.js").StructTreeLayerBuilder} StructTreeLayerBuilder */ import { AnnotationEditorType, FeatureTest } from "../../shared/util.js"; import { AnnotationEditor } from "./editor.js"; @@ -35,6 +37,7 @@ import { StampEditor } from "./stamp.js"; * @typedef {Object} AnnotationEditorLayerOptions * @property {Object} mode * @property {HTMLDivElement} div + * @property {StructTreeLayerBuilder} structTreeLayer * @property {AnnotationEditorUIManager} uiManager * @property {boolean} enabled * @property {TextAccessibilityManager} [accessibilityManager] @@ -95,6 +98,7 @@ class AnnotationEditorLayer { uiManager, pageIndex, div, + structTreeLayer, accessibilityManager, annotationLayer, drawLayer, @@ -119,6 +123,7 @@ class AnnotationEditorLayer { this.viewport = viewport; this.#textLayer = textLayer; this.drawLayer = drawLayer; + this._structTree = structTreeLayer; this.#uiManager.addLayer(this); } diff --git a/src/display/editor/stamp.js b/src/display/editor/stamp.js index 623a7d77d986a..ae631ef55a84f 100644 --- a/src/display/editor/stamp.js +++ b/src/display/editor/stamp.js @@ -13,7 +13,11 @@ * limitations under the License. */ -import { AnnotationEditorType, shadow } from "../../shared/util.js"; +import { + AnnotationEditorType, + AnnotationPrefix, + shadow, +} from "../../shared/util.js"; import { OutputScale, PixelsPerInch } from "../display_utils.js"; import { AnnotationEditor } from "./editor.js"; import { StampAnnotationElement } from "../annotation_layer.js"; @@ -383,7 +387,7 @@ class StampEditor extends AnnotationEditor { this.#getBitmap(); } - if (this.width) { + if (this.width && !this.annotationElementId) { // This editor was created in using copy (ctrl+c). const [parentWidth, parentHeight] = this.parentDimensions; this.setAt( @@ -431,7 +435,8 @@ class StampEditor extends AnnotationEditor { if ( !this._uiManager.useNewAltTextWhenAddingImage || - !this._uiManager.useNewAltTextFlow + !this._uiManager.useNewAltTextFlow || + this.annotationElementId ) { div.hidden = false; } @@ -769,13 +774,55 @@ class StampEditor extends AnnotationEditor { /** @inheritdoc */ static async deserialize(data, parent, uiManager) { + let initialData = null; if (data instanceof StampAnnotationElement) { - return null; + const { + data: { rect, rotation, id, structParent, popupRef }, + container, + parent: { + page: { pageNumber }, + }, + } = data; + const canvas = container.querySelector("canvas"); + const imageData = uiManager.imageManager.getFromCanvas( + container.id, + canvas + ); + canvas.remove(); + + // When switching to edit mode, we wait for the structure tree to be + // ready (see pdf_viewer.js), so it's fine to use getAriaAttributesSync. + const altText = + ( + await parent._structTree.getAriaAttributes(`${AnnotationPrefix}${id}`) + )?.get("aria-label") || ""; + + initialData = data = { + annotationType: AnnotationEditorType.STAMP, + bitmapId: imageData.id, + bitmap: imageData.bitmap, + pageIndex: pageNumber - 1, + rect: rect.slice(0), + rotation, + id, + deleted: false, + accessibilityData: { + decorative: false, + altText, + }, + isSvg: false, + structParent, + popupRef, + }; } const editor = await super.deserialize(data, parent, uiManager); - const { rect, bitmapUrl, bitmapId, isSvg, accessibilityData } = data; + const { rect, bitmap, bitmapUrl, bitmapId, isSvg, accessibilityData } = + data; if (bitmapId && uiManager.imageManager.isValidId(bitmapId)) { editor.#bitmapId = bitmapId; + if (bitmap) { + editor.#bitmap = bitmap; + } } else { editor.#bitmapUrl = bitmapUrl; } @@ -785,9 +832,11 @@ class StampEditor extends AnnotationEditor { editor.width = (rect[2] - rect[0]) / parentWidth; editor.height = (rect[3] - rect[1]) / parentHeight; + editor.annotationElementId = data.id || null; if (accessibilityData) { editor.altTextData = accessibilityData; } + editor._initialData = initialData; return editor; } @@ -798,6 +847,10 @@ class StampEditor extends AnnotationEditor { return null; } + if (this.deleted) { + return this.serializeDeleted(); + } + const serialized = { annotationType: AnnotationEditorType.STAMP, bitmapId: this.#bitmapId, @@ -821,6 +874,20 @@ class StampEditor extends AnnotationEditor { if (!decorative && altText) { serialized.accessibilityData = { type: "Figure", alt: altText }; } + if (this.annotationElementId) { + const changes = this.#hasElementChanged(serialized); + if (changes.isSame) { + // Nothing has been changed. + return null; + } + if (changes.isSameAltText) { + delete serialized.accessibilityData; + } else { + serialized.accessibilityData.structParent = + this._initialData.structParent ?? -1; + } + } + serialized.id = this.annotationElementId; if (context === null) { return serialized; @@ -848,6 +915,34 @@ class StampEditor extends AnnotationEditor { } return serialized; } + + #hasElementChanged(serialized) { + const { + rect, + pageIndex, + accessibilityData: { altText }, + } = this._initialData; + + const isSameRect = serialized.rect.every( + (x, i) => Math.abs(x - rect[i]) < 1 + ); + const isSamePageIndex = serialized.pageIndex === pageIndex; + const isSameAltText = (serialized.accessibilityData?.alt || "") === altText; + + return { + isSame: isSameRect && isSamePageIndex && isSameAltText, + isSameAltText, + }; + } + + /** @inheritdoc */ + renderAnnotationElement(annotation) { + annotation.updateEdited({ + rect: this.getRect(0, 0), + }); + + return null; + } } export { StampEditor }; diff --git a/src/display/editor/toolbar.js b/src/display/editor/toolbar.js index 64ed92d792431..1d67778197096 100644 --- a/src/display/editor/toolbar.js +++ b/src/display/editor/toolbar.js @@ -41,7 +41,7 @@ class EditorToolbar { render() { const editToolbar = (this.#toolbar = document.createElement("div")); - editToolbar.className = "editToolbar"; + editToolbar.classList.add("editToolbar", "hidden"); editToolbar.setAttribute("role", "toolbar"); const signal = this.#editor._uiManager._signal; editToolbar.addEventListener("contextmenu", noContextMenu, { signal }); diff --git a/src/display/editor/tools.js b/src/display/editor/tools.js index 2823cdd53b6e4..db755c08bd5e7 100644 --- a/src/display/editor/tools.js +++ b/src/display/editor/tools.js @@ -200,6 +200,27 @@ class ImageManager { return this.getFromUrl(data.url); } + getFromCanvas(id, canvas) { + this.#cache ||= new Map(); + let data = this.#cache.get(id); + if (data?.bitmap) { + data.refCounter += 1; + return data; + } + const offscreen = new OffscreenCanvas(canvas.width, canvas.height); + const ctx = offscreen.getContext("2d"); + ctx.drawImage(canvas, 0, 0); + data = { + bitmap: offscreen.transferToImageBitmap(), + id: `image_${this.#baseId}_${this.#id++}`, + refCounter: 1, + isSvg: false, + }; + this.#cache.set(id, data); + this.#cache.set(data.id, data); + return data; + } + getSvgUrl(id) { const data = this.#cache.get(id); if (!data?.isSvg) { @@ -218,6 +239,7 @@ class ImageManager { if (data.refCounter !== 0) { return; } + data.bitmap.close?.(); data.bitmap = null; } @@ -1619,7 +1641,8 @@ class AnnotationEditorUIManager { if (editor.annotationElementId === editId) { this.setSelected(editor); editor.enterInEditMode(); - break; + } else { + editor.unselect(); } } diff --git a/test/integration/stamp_editor_spec.mjs b/test/integration/stamp_editor_spec.mjs index ed706d0c0b769..2f3b683c4b710 100644 --- a/test/integration/stamp_editor_spec.mjs +++ b/test/integration/stamp_editor_spec.mjs @@ -20,7 +20,10 @@ import { closePages, copy, copyToClipboard, + dragAndDropAnnotation, + getAnnotationSelector, getEditorDimensions, + getEditors, getEditorSelector, getFirstSerialized, getRect, @@ -1281,4 +1284,108 @@ describe("Stamp Editor", () => { ); }); }); + + describe("Stamp (move existing)", () => { + let pages; + + beforeAll(async () => { + pages = await loadAndWait("stamps.pdf", getAnnotationSelector("25R")); + }); + + afterAll(async () => { + await closePages(pages); + }); + + it("must move an annotation", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + await page.click(getAnnotationSelector("25R"), { count: 2 }); + await waitForSelectedEditor(page, getEditorSelector(0)); + + const editorIds = await getEditors(page, "stamp"); + expect(editorIds.length).withContext(`In ${browserName}`).toEqual(5); + + // All the current annotations should be serialized as null objects + // because they haven't been edited yet. + const serialized = await getSerialized(page); + expect(serialized).withContext(`In ${browserName}`).toEqual([]); + + const editorRect = await page.$eval(getEditorSelector(0), el => { + const { x, y, width, height } = el.getBoundingClientRect(); + return { x, y, width, height }; + }); + + // Select the annotation we want to move. + await page.mouse.click(editorRect.x + 2, editorRect.y + 2); + await waitForSelectedEditor(page, getEditorSelector(0)); + + await dragAndDropAnnotation( + page, + editorRect.x + editorRect.width / 2, + editorRect.y + editorRect.height / 2, + 100, + 100 + ); + await waitForSerialized(page, 1); + }) + ); + }); + }); + + describe("Stamp (change alt-text)", () => { + let pages; + + beforeAll(async () => { + pages = await loadAndWait("stamps.pdf", getAnnotationSelector("58R")); + }); + + afterAll(async () => { + await closePages(pages); + }); + + it("must update an existing alt-text", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + await page.click(getAnnotationSelector("58R"), { count: 2 }); + await waitForSelectedEditor(page, getEditorSelector(4)); + + const editorIds = await getEditors(page, "stamp"); + expect(editorIds.length).withContext(`In ${browserName}`).toEqual(5); + + await page.click(`${getEditorSelector(4)} button.altText`); + await page.waitForSelector("#altTextDialog", { visible: true }); + + const textareaSelector = "#altTextDialog textarea"; + await page.waitForFunction( + sel => document.querySelector(sel).value !== "", + {}, + textareaSelector + ); + + const altText = await page.evaluate( + sel => document.querySelector(sel).value, + textareaSelector + ); + expect(altText).toEqual("An elephant"); + + await page.evaluate(sel => { + document.querySelector(sel).value = ""; + }, textareaSelector); + + await page.click(textareaSelector); + await page.type(textareaSelector, "Hello World"); + + // All the current annotations should be serialized as null objects + // because they haven't been edited yet. + const serialized = await getSerialized(page); + expect(serialized).withContext(`In ${browserName}`).toEqual([]); + + const saveButtonSelector = "#altTextDialog #altTextSave"; + await page.click(saveButtonSelector); + + await waitForSerialized(page, 1); + }) + ); + }); + }); }); diff --git a/test/integration/test_utils.mjs b/test/integration/test_utils.mjs index 66673867d65cf..7e6ec548b0f22 100644 --- a/test/integration/test_utils.mjs +++ b/test/integration/test_utils.mjs @@ -225,6 +225,10 @@ function getEditorSelector(n) { return `#pdfjs_internal_editor_${n}`; } +function getAnnotationSelector(id) { + return `[data-annotation-id="${id}"]`; +} + function getSelectedEditors(page) { return page.evaluate(() => { const elements = document.querySelectorAll(".selectedEditor"); @@ -769,6 +773,7 @@ export { createPromise, dragAndDropAnnotation, firstPageOnTop, + getAnnotationSelector, getAnnotationStorage, getComputedStyleSelector, getEditorDimensions, diff --git a/web/annotation_editor_layer_builder.css b/web/annotation_editor_layer_builder.css index a5b8d47e18ba0..82129fa72c225 100644 --- a/web/annotation_editor_layer_builder.css +++ b/web/annotation_editor_layer_builder.css @@ -82,7 +82,8 @@ } } -#viewerContainer.pdfPresentationMode:fullscreen { +#viewerContainer.pdfPresentationMode:fullscreen, +.annotationEditorLayer.disabled { .noAltTextBadge { display: none !important; } diff --git a/web/annotation_editor_layer_builder.js b/web/annotation_editor_layer_builder.js index ba5178774b680..46082c163f838 100644 --- a/web/annotation_editor_layer_builder.js +++ b/web/annotation_editor_layer_builder.js @@ -23,6 +23,8 @@ /** @typedef {import("./interfaces").IL10n} IL10n */ // eslint-disable-next-line max-len /** @typedef {import("../src/display/annotation_layer.js").AnnotationLayer} AnnotationLayer */ +// eslint-disable-next-line max-len +/** @typedef {import("../src/display/struct_tree_layer_builder.js").StructTreeLayerBuilder} StructTreeLayerBuilder */ import { AnnotationEditorLayer } from "pdfjs-lib"; import { GenericL10n } from "web-null_l10n"; @@ -32,6 +34,7 @@ import { GenericL10n } from "web-null_l10n"; * @property {AnnotationEditorUIManager} [uiManager] * @property {PDFPageProxy} pdfPage * @property {IL10n} [l10n] + * @property {StructTreeLayerBuilder} [structTreeLayer] * @property {TextAccessibilityManager} [accessibilityManager] * @property {AnnotationLayer} [annotationLayer] * @property {TextLayer} [textLayer] @@ -46,6 +49,8 @@ class AnnotationEditorLayerBuilder { #onAppend = null; + #structTreeLayer = null; + #textLayer = null; #uiManager; @@ -68,6 +73,7 @@ class AnnotationEditorLayerBuilder { this.#textLayer = options.textLayer || null; this.#drawLayer = options.drawLayer || null; this.#onAppend = options.onAppend || null; + this.#structTreeLayer = options.structTreeLayer || null; } /** @@ -100,6 +106,7 @@ class AnnotationEditorLayerBuilder { this.annotationEditorLayer = new AnnotationEditorLayer({ uiManager: this.#uiManager, div, + structTreeLayer: this.#structTreeLayer, accessibilityManager: this.accessibilityManager, pageIndex: this.pdfPage.pageNumber - 1, l10n: this.l10n, diff --git a/web/pdf_page_view.js b/web/pdf_page_view.js index 4737775eeea32..010aa18ae437c 100644 --- a/web/pdf_page_view.js +++ b/web/pdf_page_view.js @@ -1098,12 +1098,10 @@ class PDFPageView { showCanvas?.(true); await this.#finishRenderTask(renderTask); - if (this.textLayer || this.annotationLayer) { - this.structTreeLayer ||= new StructTreeLayerBuilder( - pdfPage, - viewport.rawDims - ); - } + this.structTreeLayer ||= new StructTreeLayerBuilder( + pdfPage, + viewport.rawDims + ); this.#renderTextLayer(); @@ -1126,6 +1124,7 @@ class PDFPageView { uiManager: annotationEditorUIManager, pdfPage, l10n, + structTreeLayer: this.structTreeLayer, accessibilityManager: this._accessibilityManager, annotationLayer: this.annotationLayer?.annotationLayer, textLayer: this.textLayer,