From d5a0e557c22c97dd985a570263ccbab9e03a815c Mon Sep 17 00:00:00 2001 From: Calixte Denizet Date: Fri, 22 Mar 2024 19:30:56 +0100 Subject: [PATCH] [Editor] Fix undoing an editor deletion (bug 1886959) The original bug was because the parent was null when trying to show an highlight annotation which led to an exception. That led me to think about having some null/non-null parent when removing an editor: it's a mess especially if a destroyed parent is still attached to an editor. Consequently, this patch always sets the parent to null when deleting the editor. --- src/display/editor/annotation_editor_layer.js | 5 +- src/display/editor/editor.js | 6 +- src/display/editor/highlight.js | 11 +- src/display/editor/ink.js | 2 +- src/display/editor/stamp.js | 5 +- src/display/editor/tools.js | 1 + test/integration/freetext_editor_spec.mjs | 115 ++++++++++++ test/integration/highlight_editor_spec.mjs | 154 +++++++++++++++++ test/integration/ink_editor_spec.mjs | 163 ++++++++++++++++++ test/integration/stamp_editor_spec.mjs | 123 +++++++++++++ 10 files changed, 574 insertions(+), 11 deletions(-) diff --git a/src/display/editor/annotation_editor_layer.js b/src/display/editor/annotation_editor_layer.js index 9061d7c2b..9118c72b3 100644 --- a/src/display/editor/annotation_editor_layer.js +++ b/src/display/editor/annotation_editor_layer.js @@ -441,9 +441,6 @@ class AnnotationEditorLayer { * @param {AnnotationEditor} editor */ remove(editor) { - // Since we can undo a removal we need to keep the - // parent property as it is, so don't null it! - this.detach(editor); this.#uiManager.removeEditor(editor); editor.div.remove(); @@ -546,6 +543,7 @@ class AnnotationEditorLayer { if (editor.needsToBeRebuilt()) { editor.parent ||= this; editor.rebuild(); + editor.show(); } else { this.add(editor); } @@ -842,6 +840,7 @@ class AnnotationEditorLayer { setLayerDimensions(this.div, viewport); for (const editor of this.#uiManager.getEditors(this.pageIndex)) { this.add(editor); + editor.rebuild(); } // We're maybe rendering a layer which was invisible when we started to edit // so we must set the different callbacks for it. diff --git a/src/display/editor/editor.js b/src/display/editor/editor.js index edab27b79..589a6ad8f 100644 --- a/src/display/editor/editor.js +++ b/src/display/editor/editor.js @@ -1270,7 +1270,6 @@ class AnnotationEditor { rebuild() { this.div?.addEventListener("focusin", this.#boundFocusin); this.div?.addEventListener("focusout", this.#boundFocusout); - this.show(this._isVisible); } /** @@ -1354,6 +1353,7 @@ class AnnotationEditor { } this.#telemetryTimeouts = null; } + this.parent = null; } /** @@ -1676,9 +1676,9 @@ class AnnotationEditor { /** * Show or hide this editor. - * @param {boolean} visible + * @param {boolean|undefined} visible */ - show(visible) { + show(visible = this._isVisible) { this.div.classList.toggle("hidden", !visible); this._isVisible = visible; } diff --git a/src/display/editor/highlight.js b/src/display/editor/highlight.js index 8b819427f..346922381 100644 --- a/src/display/editor/highlight.js +++ b/src/display/editor/highlight.js @@ -418,11 +418,11 @@ class HighlightEditor extends AnnotationEditor { /** @inheritdoc */ remove() { - super.remove(); this.#cleanDrawLayer(); this._reportTelemetry({ action: "deleted", }); + super.remove(); } /** @inheritdoc */ @@ -628,6 +628,9 @@ class HighlightEditor extends AnnotationEditor { /** @inheritdoc */ select() { super.select(); + if (!this.#outlineId) { + return; + } this.parent?.drawLayer.removeClass(this.#outlineId, "hovered"); this.parent?.drawLayer.addClass(this.#outlineId, "selected"); } @@ -635,6 +638,9 @@ class HighlightEditor extends AnnotationEditor { /** @inheritdoc */ unselect() { super.unselect(); + if (!this.#outlineId) { + return; + } this.parent?.drawLayer.removeClass(this.#outlineId, "selected"); if (!this.#isFreeHighlight) { this.#setCaret(/* start = */ false); @@ -646,7 +652,8 @@ class HighlightEditor extends AnnotationEditor { return !this.#isFreeHighlight; } - show(visible) { + /** @inheritdoc */ + show(visible = this._isVisible) { super.show(visible); if (this.parent) { this.parent.drawLayer.show(this.#id, visible); diff --git a/src/display/editor/ink.js b/src/display/editor/ink.js index bcf345db8..f07947634 100644 --- a/src/display/editor/ink.js +++ b/src/display/editor/ink.js @@ -471,7 +471,7 @@ class InkEditor extends AnnotationEditor { this.allRawPaths.push(currentPath); this.paths.push(bezier); this.bezierPath2D.push(path2D); - this.rebuild(); + this._uiManager.rebuild(this); }; const undo = () => { diff --git a/src/display/editor/stamp.js b/src/display/editor/stamp.js index 785fe6c48..ab08dcf0c 100644 --- a/src/display/editor/stamp.js +++ b/src/display/editor/stamp.js @@ -218,7 +218,7 @@ class StampEditor extends AnnotationEditor { return; } - if (this.#bitmapId) { + if (this.#bitmapId && this.#canvas === null) { this.#getBitmap(); } @@ -241,7 +241,8 @@ class StampEditor extends AnnotationEditor { this.#bitmapPromise || this.#bitmap || this.#bitmapUrl || - this.#bitmapFile + this.#bitmapFile || + this.#bitmapId ); } diff --git a/src/display/editor/tools.js b/src/display/editor/tools.js index beaec5a44..fc7f9f6ec 100644 --- a/src/display/editor/tools.js +++ b/src/display/editor/tools.js @@ -1713,6 +1713,7 @@ class AnnotationEditorUIManager { layer.addOrRebuild(editor); } else { this.addEditor(editor); + this.addToAnnotationStorage(editor); } } diff --git a/test/integration/freetext_editor_spec.mjs b/test/integration/freetext_editor_spec.mjs index 0cfb9d059..ffa64f01f 100644 --- a/test/integration/freetext_editor_spec.mjs +++ b/test/integration/freetext_editor_spec.mjs @@ -3431,4 +3431,119 @@ describe("FreeText Editor", () => { ); }); }); + + describe("Delete a freetext and undo it on another page", () => { + let pages; + + beforeAll(async () => { + pages = await loadAndWait("tracemonkey.pdf", ".annotationEditorLayer"); + }); + + afterAll(async () => { + await closePages(pages); + }); + + it("must check that a freetext can be undone", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + await switchToFreeText(page); + + const rect = await page.$eval(".annotationEditorLayer", el => { + const { x, y } = el.getBoundingClientRect(); + return { x, y }; + }); + + const data = "Hello PDF.js World !!"; + await page.mouse.click(rect.x + 100, rect.y + 100); + await page.waitForSelector(getEditorSelector(0), { + visible: true, + }); + await page.type(`${getEditorSelector(0)} .internal`, data); + + // Commit. + await page.keyboard.press("Escape"); + await page.waitForSelector( + `${getEditorSelector(0)} .overlay.enabled` + ); + await waitForSerialized(page, 1); + + await page.waitForSelector(`${getEditorSelector(0)} button.delete`); + await page.click(`${getEditorSelector(0)} button.delete`); + await waitForSerialized(page, 0); + + const twoToFourteen = Array.from(new Array(13).keys(), n => n + 2); + for (const pageNumber of twoToFourteen) { + const pageSelector = `.page[data-page-number = "${pageNumber}"]`; + await scrollIntoView(page, pageSelector); + } + + await kbUndo(page); + await waitForSerialized(page, 1); + + const thirteenToOne = Array.from(new Array(13).keys(), n => 13 - n); + for (const pageNumber of thirteenToOne) { + const pageSelector = `.page[data-page-number = "${pageNumber}"]`; + await scrollIntoView(page, pageSelector); + } + + await page.waitForSelector(getEditorSelector(0)); + }) + ); + }); + }); + + describe("Delete a freetext, scroll and undo it", () => { + let pages; + + beforeAll(async () => { + pages = await loadAndWait("tracemonkey.pdf", ".annotationEditorLayer"); + }); + + afterAll(async () => { + await closePages(pages); + }); + + it("must check that a freetext can be undone", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + await switchToFreeText(page); + + const rect = await page.$eval(".annotationEditorLayer", el => { + const { x, y } = el.getBoundingClientRect(); + return { x, y }; + }); + + const data = "Hello PDF.js World !!"; + await page.mouse.click(rect.x + 100, rect.y + 100); + await page.waitForSelector(getEditorSelector(0), { + visible: true, + }); + await page.type(`${getEditorSelector(0)} .internal`, data); + + // Commit. + await page.keyboard.press("Escape"); + await page.waitForSelector( + `${getEditorSelector(0)} .overlay.enabled` + ); + await waitForSerialized(page, 1); + + await page.waitForSelector(`${getEditorSelector(0)} button.delete`); + await page.click(`${getEditorSelector(0)} button.delete`); + await waitForSerialized(page, 0); + + const twoToOne = Array.from(new Array(13).keys(), n => n + 2).concat( + Array.from(new Array(13).keys(), n => 13 - n) + ); + for (const pageNumber of twoToOne) { + const pageSelector = `.page[data-page-number = "${pageNumber}"]`; + await scrollIntoView(page, pageSelector); + } + + await kbUndo(page); + await waitForSerialized(page, 1); + await page.waitForSelector(getEditorSelector(0)); + }) + ); + }); + }); }); diff --git a/test/integration/highlight_editor_spec.mjs b/test/integration/highlight_editor_spec.mjs index 94cffc4c7..ea48ce981 100644 --- a/test/integration/highlight_editor_spec.mjs +++ b/test/integration/highlight_editor_spec.mjs @@ -25,6 +25,7 @@ import { kbFocusNext, kbFocusPrevious, kbSelectAll, + kbUndo, loadAndWait, scrollIntoView, waitForSerialized, @@ -1647,4 +1648,157 @@ describe("Highlight Editor", () => { ); }); }); + + describe("Undo a highlight", () => { + let pages; + + beforeAll(async () => { + pages = await loadAndWait("tracemonkey.pdf", ".annotationEditorLayer"); + }); + + afterAll(async () => { + await closePages(pages); + }); + + it("must check that a highlight can be undone", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + await page.click("#editorHighlight"); + await page.waitForSelector(".annotationEditorLayer.highlightEditing"); + + const rect = await getSpanRectFromText(page, 1, "Abstract"); + const x = rect.x + rect.width / 2; + const y = rect.y + rect.height / 2; + await page.mouse.click(x, y, { count: 2, delay: 100 }); + await page.waitForSelector(getEditorSelector(0)); + await waitForSerialized(page, 1); + + await page.waitForSelector(`${getEditorSelector(0)} button.delete`); + await page.click(`${getEditorSelector(0)} button.delete`); + await waitForSerialized(page, 0); + + await kbUndo(page); + await waitForSerialized(page, 1); + await page.waitForSelector(getEditorSelector(0)); + }) + ); + }); + }); + + describe("Delete a highlight and undo it an other page", () => { + let pages; + + beforeAll(async () => { + pages = await loadAndWait( + "tracemonkey.pdf", + ".annotationEditorLayer", + null, + null, + { + highlightEditorColors: + "yellow=#FFFF00,green=#00FF00,blue=#0000FF,pink=#FF00FF,red=#FF0000", + } + ); + }); + + afterAll(async () => { + await closePages(pages); + }); + + it("must check that a highlight can be undone", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + await page.click("#editorHighlight"); + await page.waitForSelector(".annotationEditorLayer.highlightEditing"); + + const rect = await getSpanRectFromText(page, 1, "Abstract"); + const x = rect.x + rect.width / 2; + const y = rect.y + rect.height / 2; + await page.mouse.click(x, y, { count: 2, delay: 100 }); + await page.waitForSelector(getEditorSelector(0)); + await waitForSerialized(page, 1); + + await page.waitForSelector(`${getEditorSelector(0)} button.delete`); + await page.click(`${getEditorSelector(0)} button.delete`); + await waitForSerialized(page, 0); + + const twoToFourteen = Array.from(new Array(13).keys(), n => n + 2); + for (const pageNumber of twoToFourteen) { + const pageSelector = `.page[data-page-number = "${pageNumber}"]`; + await scrollIntoView(page, pageSelector); + } + + await kbUndo(page); + await waitForSerialized(page, 1); + + const thirteenToOne = Array.from(new Array(13).keys(), n => 13 - n); + for (const pageNumber of thirteenToOne) { + const pageSelector = `.page[data-page-number = "${pageNumber}"]`; + await scrollIntoView(page, pageSelector); + } + + await page.waitForSelector(getEditorSelector(0)); + await page.waitForSelector( + `.page[data-page-number = "1"] svg.highlight[fill = "#FFFF00"]` + ); + }) + ); + }); + }); + + describe("Delete a highlight, scroll and undo it", () => { + let pages; + + beforeAll(async () => { + pages = await loadAndWait( + "tracemonkey.pdf", + ".annotationEditorLayer", + null, + null, + { + highlightEditorColors: + "yellow=#FFFF00,green=#00FF00,blue=#0000FF,pink=#FF00FF,red=#FF0000", + } + ); + }); + + afterAll(async () => { + await closePages(pages); + }); + + it("must check that a highlight can be undone", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + await page.click("#editorHighlight"); + await page.waitForSelector(".annotationEditorLayer.highlightEditing"); + + const rect = await getSpanRectFromText(page, 1, "Abstract"); + const x = rect.x + rect.width / 2; + const y = rect.y + rect.height / 2; + await page.mouse.click(x, y, { count: 2, delay: 100 }); + await page.waitForSelector(getEditorSelector(0)); + await waitForSerialized(page, 1); + + await page.waitForSelector(`${getEditorSelector(0)} button.delete`); + await page.click(`${getEditorSelector(0)} button.delete`); + await waitForSerialized(page, 0); + + const twoToOne = Array.from(new Array(13).keys(), n => n + 2).concat( + Array.from(new Array(13).keys(), n => 13 - n) + ); + for (const pageNumber of twoToOne) { + const pageSelector = `.page[data-page-number = "${pageNumber}"]`; + await scrollIntoView(page, pageSelector); + } + + await kbUndo(page); + await waitForSerialized(page, 1); + await page.waitForSelector(getEditorSelector(0)); + await page.waitForSelector( + `.page[data-page-number = "1"] svg.highlight[fill = "#FFFF00"]` + ); + }) + ); + }); + }); }); diff --git a/test/integration/ink_editor_spec.mjs b/test/integration/ink_editor_spec.mjs index 8b7c9a67d..81b1ff079 100644 --- a/test/integration/ink_editor_spec.mjs +++ b/test/integration/ink_editor_spec.mjs @@ -24,6 +24,7 @@ import { kbUndo, loadAndWait, scrollIntoView, + waitForSerialized, waitForStorageEntries, } from "./test_utils.mjs"; @@ -296,4 +297,166 @@ describe("Ink Editor", () => { ); }); }); + + describe("Undo a draw", () => { + let pages; + + beforeAll(async () => { + pages = await loadAndWait("tracemonkey.pdf", ".annotationEditorLayer"); + }); + + afterAll(async () => { + await closePages(pages); + }); + + it("must check that a draw can be undone", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + await page.click("#editorInk"); + await page.waitForSelector(".annotationEditorLayer.inkEditing"); + + const rect = await page.$eval(".annotationEditorLayer", el => { + const { x, y } = el.getBoundingClientRect(); + return { x, y }; + }); + + const xStart = rect.x + 300; + const yStart = rect.y + 300; + const clickHandle = await waitForPointerUp(page); + await page.mouse.move(xStart, yStart); + await page.mouse.down(); + await page.mouse.move(xStart + 50, yStart + 50); + await page.mouse.up(); + await awaitPromise(clickHandle); + await commit(page); + + await page.waitForSelector(getEditorSelector(0)); + await waitForSerialized(page, 1); + + await page.waitForSelector(`${getEditorSelector(0)} button.delete`); + await page.click(`${getEditorSelector(0)} button.delete`); + await waitForSerialized(page, 0); + + await kbUndo(page); + await waitForSerialized(page, 1); + await page.waitForSelector(getEditorSelector(0)); + }) + ); + }); + }); + + describe("Delete a draw and undo it on another page", () => { + let pages; + + beforeAll(async () => { + pages = await loadAndWait("tracemonkey.pdf", ".annotationEditorLayer"); + }); + + afterAll(async () => { + await closePages(pages); + }); + + it("must check that a draw can be undone", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + await page.click("#editorInk"); + await page.waitForSelector(".annotationEditorLayer.inkEditing"); + + const rect = await page.$eval(".annotationEditorLayer", el => { + const { x, y } = el.getBoundingClientRect(); + return { x, y }; + }); + + const xStart = rect.x + 300; + const yStart = rect.y + 300; + const clickHandle = await waitForPointerUp(page); + await page.mouse.move(xStart, yStart); + await page.mouse.down(); + await page.mouse.move(xStart + 50, yStart + 50); + await page.mouse.up(); + await awaitPromise(clickHandle); + await commit(page); + + await page.waitForSelector(getEditorSelector(0)); + await waitForSerialized(page, 1); + + await page.waitForSelector(`${getEditorSelector(0)} button.delete`); + await page.click(`${getEditorSelector(0)} button.delete`); + await waitForSerialized(page, 0); + + const twoToFourteen = Array.from(new Array(13).keys(), n => n + 2); + for (const pageNumber of twoToFourteen) { + const pageSelector = `.page[data-page-number = "${pageNumber}"]`; + await scrollIntoView(page, pageSelector); + } + + await kbUndo(page); + await waitForSerialized(page, 1); + + const thirteenToOne = Array.from(new Array(13).keys(), n => 13 - n); + for (const pageNumber of thirteenToOne) { + const pageSelector = `.page[data-page-number = "${pageNumber}"]`; + await scrollIntoView(page, pageSelector); + } + + await page.waitForSelector(getEditorSelector(0)); + }) + ); + }); + }); + + describe("Delete a draw, scroll and undo it", () => { + let pages; + + beforeAll(async () => { + pages = await loadAndWait("tracemonkey.pdf", ".annotationEditorLayer"); + }); + + afterAll(async () => { + await closePages(pages); + }); + + it("must check that a draw can be undone", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + await page.click("#editorInk"); + await page.waitForSelector(".annotationEditorLayer.inkEditing"); + + const rect = await page.$eval(".annotationEditorLayer", el => { + const { x, y } = el.getBoundingClientRect(); + return { x, y }; + }); + + const xStart = rect.x + 300; + const yStart = rect.y + 300; + const clickHandle = await waitForPointerUp(page); + await page.mouse.move(xStart, yStart); + await page.mouse.down(); + await page.mouse.move(xStart + 50, yStart + 50); + await page.mouse.up(); + await awaitPromise(clickHandle); + await commit(page); + + await page.waitForSelector(getEditorSelector(0)); + await waitForSerialized(page, 1); + + await page.waitForSelector(`${getEditorSelector(0)} button.delete`); + await page.click(`${getEditorSelector(0)} button.delete`); + await waitForSerialized(page, 0); + + const twoToOne = Array.from(new Array(13).keys(), n => n + 2).concat( + Array.from(new Array(13).keys(), n => 13 - n) + ); + for (const pageNumber of twoToOne) { + const pageSelector = `.page[data-page-number = "${pageNumber}"]`; + await scrollIntoView(page, pageSelector); + } + + await kbUndo(page); + await waitForSerialized(page, 1); + await page.waitForSelector(getEditorSelector(0)); + }) + ); + }); + }); }); diff --git a/test/integration/stamp_editor_spec.mjs b/test/integration/stamp_editor_spec.mjs index 116bc558f..015ad7892 100644 --- a/test/integration/stamp_editor_spec.mjs +++ b/test/integration/stamp_editor_spec.mjs @@ -24,10 +24,13 @@ import { kbCopy, kbPaste, kbSelectAll, + kbUndo, loadAndWait, + scrollIntoView, serializeBitmapDimensions, waitForAnnotationEditorLayer, waitForSelectedEditor, + waitForSerialized, waitForStorageEntries, } from "./test_utils.mjs"; import { fileURLToPath } from "url"; @@ -605,4 +608,124 @@ describe("Stamp Editor", () => { } }); }); + + describe("Undo a stamp", () => { + let pages; + + beforeAll(async () => { + pages = await loadAndWait("tracemonkey.pdf", ".annotationEditorLayer"); + }); + + afterAll(async () => { + await closePages(pages); + }); + + it("must check that a stamp can be undone", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + await page.click("#editorStamp"); + await page.waitForSelector(".annotationEditorLayer.stampEditing"); + + await copyImage(page, "../images/firefox_logo.png", 0); + await page.waitForSelector(getEditorSelector(0)); + await waitForSerialized(page, 1); + + await page.waitForSelector(`${getEditorSelector(0)} button.delete`); + await page.click(`${getEditorSelector(0)} button.delete`); + await waitForSerialized(page, 0); + + await kbUndo(page); + await waitForSerialized(page, 1); + await page.waitForSelector(getEditorSelector(0)); + }) + ); + }); + }); + + describe("Delete a stamp and undo it on another page", () => { + let pages; + + beforeAll(async () => { + pages = await loadAndWait("tracemonkey.pdf", ".annotationEditorLayer"); + }); + + afterAll(async () => { + await closePages(pages); + }); + + it("must check that a stamp can be undone", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + await page.click("#editorStamp"); + await page.waitForSelector(".annotationEditorLayer.stampEditing"); + + await copyImage(page, "../images/firefox_logo.png", 0); + await page.waitForSelector(getEditorSelector(0)); + await waitForSerialized(page, 1); + + await page.waitForSelector(`${getEditorSelector(0)} button.delete`); + await page.click(`${getEditorSelector(0)} button.delete`); + await waitForSerialized(page, 0); + + const twoToFourteen = Array.from(new Array(13).keys(), n => n + 2); + for (const pageNumber of twoToFourteen) { + const pageSelector = `.page[data-page-number = "${pageNumber}"]`; + await scrollIntoView(page, pageSelector); + } + + await kbUndo(page); + await waitForSerialized(page, 1); + + const thirteenToOne = Array.from(new Array(13).keys(), n => 13 - n); + for (const pageNumber of thirteenToOne) { + const pageSelector = `.page[data-page-number = "${pageNumber}"]`; + await scrollIntoView(page, pageSelector); + } + + await page.waitForSelector(getEditorSelector(0)); + }) + ); + }); + }); + + describe("Delete a stamp, scroll and undo it", () => { + let pages; + + beforeAll(async () => { + pages = await loadAndWait("tracemonkey.pdf", ".annotationEditorLayer"); + }); + + afterAll(async () => { + await closePages(pages); + }); + + it("must check that a stamp can be undone", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + await page.click("#editorStamp"); + await page.waitForSelector(".annotationEditorLayer.stampEditing"); + + await copyImage(page, "../images/firefox_logo.png", 0); + await page.waitForSelector(getEditorSelector(0)); + await waitForSerialized(page, 1); + + await page.waitForSelector(`${getEditorSelector(0)} button.delete`); + await page.click(`${getEditorSelector(0)} button.delete`); + await waitForSerialized(page, 0); + + const twoToOne = Array.from(new Array(13).keys(), n => n + 2).concat( + Array.from(new Array(13).keys(), n => 13 - n) + ); + for (const pageNumber of twoToOne) { + const pageSelector = `.page[data-page-number = "${pageNumber}"]`; + await scrollIntoView(page, pageSelector); + } + + await kbUndo(page); + await waitForSerialized(page, 1); + await page.waitForSelector(getEditorSelector(0)); + }) + ); + }); + }); });