From 8ab4e2e6e7aa39288d502b16016de2208be10d67 Mon Sep 17 00:00:00 2001 From: Calixte Denizet Date: Thu, 7 Sep 2023 12:30:29 +0200 Subject: [PATCH] [Editor] Avoid to use parent of editors in destroyed pages --- src/display/editor/annotation_editor_layer.js | 5 ++ src/display/editor/editor.js | 9 +- src/display/editor/tools.js | 10 ++- test/integration/freetext_editor_spec.js | 85 +++++++++++++++++++ test/integration/test_utils.js | 2 + 5 files changed, 106 insertions(+), 5 deletions(-) diff --git a/src/display/editor/annotation_editor_layer.js b/src/display/editor/annotation_editor_layer.js index 72faafc4c514b..88516c38f4152 100644 --- a/src/display/editor/annotation_editor_layer.js +++ b/src/display/editor/annotation_editor_layer.js @@ -377,7 +377,10 @@ class AnnotationEditorLayer { editor.isAttachedToDOM = true; } + // The editor must have the right position before being moved in the DOM. + editor.fixAndSetPosition(); this.moveEditorInDOM(editor); + editor.onceAdded(); this.#uiManager.addToAnnotationStorage(editor); } @@ -675,6 +678,8 @@ class AnnotationEditorLayer { */ destroy() { if (this.#uiManager.getActive()?.parent === this) { + // We need to commit the current editor before destroying the layer. + this.#uiManager.commitOrRemove(); this.#uiManager.setActiveEditor(null); } diff --git a/src/display/editor/editor.js b/src/display/editor/editor.js index 2b4b61bdf4a0e..1fe4bc56c78fa 100644 --- a/src/display/editor/editor.js +++ b/src/display/editor/editor.js @@ -347,7 +347,14 @@ class AnnotationEditor { const [parentWidth, parentHeight] = this.parentDimensions; this.x += tx / parentWidth; this.y += ty / parentHeight; - if (this.x < 0 || this.x > 1 || this.y < 0 || this.y > 1) { + if (this.parent && (this.x < 0 || this.x > 1 || this.y < 0 || this.y > 1)) { + // It's possible to not have a parent: for example, when the user is + // dragging all the selected editors but this one on a page which has been + // destroyed. + // It's why we need to check for it. In such a situation, it isn't really + // a problem to not find a new parent: it's something which is related to + // what the user is seeing, hence it depends on how pages are layed out. + // The element will be outside of its parent so change the parent. const { x, y } = this.div.getBoundingClientRect(); if (this.parent.findNewParent(this, x, y)) { diff --git a/src/display/editor/tools.js b/src/display/editor/tools.js index ec1bdf3435b67..f0177494d2a10 100644 --- a/src/display/editor/tools.js +++ b/src/display/editor/tools.js @@ -1565,6 +1565,8 @@ class AnnotationEditorUIManager { * Set up the drag session for moving the selected editors. */ setUpDragSession() { + // Note: don't use any references to the editor's parent which can be null + // if the editor belongs to a destroyed page. if (!this.hasSelection) { return; } @@ -1575,7 +1577,7 @@ class AnnotationEditorUIManager { this.#draggingEditors.set(editor, { savedX: editor.x, savedY: editor.y, - savedPageIndex: editor.parent.pageIndex, + savedPageIndex: editor.pageIndex, newX: 0, newY: 0, newPageIndex: -1, @@ -1596,14 +1598,14 @@ class AnnotationEditorUIManager { this.#draggingEditors = null; let mustBeAddedInUndoStack = false; - for (const [{ x, y, parent }, value] of map) { + for (const [{ x, y, pageIndex }, value] of map) { value.newX = x; value.newY = y; - value.newPageIndex = parent.pageIndex; + value.newPageIndex = pageIndex; mustBeAddedInUndoStack ||= x !== value.savedX || y !== value.savedY || - parent.pageIndex !== value.savedPageIndex; + pageIndex !== value.savedPageIndex; } if (!mustBeAddedInUndoStack) { diff --git a/test/integration/freetext_editor_spec.js b/test/integration/freetext_editor_spec.js index 6f582bf11fc21..bfa05e7882324 100644 --- a/test/integration/freetext_editor_spec.js +++ b/test/integration/freetext_editor_spec.js @@ -2344,4 +2344,89 @@ describe("FreeText Editor", () => { ); }); }); + + describe("FreeText on several pages", () => { + let pages; + + beforeAll(async () => { + pages = await loadAndWait("tracemonkey.pdf", ".annotationEditorLayer"); + }); + + afterAll(async () => { + await closePages(pages); + }); + + it("must check that first annotation is selected without errors", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + await page.click("#editorFreeText"); + + const page1Selector = `.page[data-page-number = "1"] > .annotationEditorLayer`; + let rect = await page.$eval(page1Selector, el => { + const { x, y } = el.getBoundingClientRect(); + return { x, y }; + }); + await page.mouse.click(rect.x + 10, rect.y + 10); + await page.waitForTimeout(10); + await page.type(`${getEditorSelector(0)} .internal`, "Hello"); + + // Commit. + await page.keyboard.press("Escape"); + await page.waitForTimeout(10); + + // Go to the last page. + await page.keyboard.press("End"); + await page.waitForTimeout(10); + + const page14Selector = `.page[data-page-number = "14"] > .annotationEditorLayer`; + await page.waitForSelector(page14Selector, { + visible: true, + timeout: 0, + }); + await page.waitForTimeout(10); + + rect = await page.$eval(page14Selector, el => { + const { x, y } = el.getBoundingClientRect(); + return { x, y }; + }); + await page.mouse.click(rect.x + 10, rect.y + 10); + await page.waitForTimeout(10); + await page.type(`${getEditorSelector(0)} .internal`, "World"); + + await page.keyboard.press("Escape"); + await page.waitForTimeout(10); + + for (let i = 0; i <= 13; i++) { + await page.keyboard.press("P"); + await page.waitForTimeout(10); + } + + await page.waitForSelector(getEditorSelector(0), { + visible: true, + timeout: 0, + }); + await page.waitForTimeout(10); + + rect = await page.$eval(getEditorSelector(0), el => { + const { x, y, width, height } = el.getBoundingClientRect(); + return { + x, + y, + width, + height, + }; + }); + await page.mouse.click( + rect.x + rect.width / 2, + rect.y + rect.height / 2 + ); + + const content = await page.$eval(getEditorSelector(0), el => + el.innerText.trimEnd() + ); + expect(content).withContext(`In ${browserName}`).toEqual("Hello"); + }) + ); + }); + }); }); diff --git a/test/integration/test_utils.js b/test/integration/test_utils.js index 08fc01925d434..d39426166453c 100644 --- a/test/integration/test_utils.js +++ b/test/integration/test_utils.js @@ -53,6 +53,8 @@ exports.loadAndWait = (filename, selector, zoom, pageSetup) => exports.closePages = pages => Promise.all( pages.map(async ([_, page]) => { + // Avoid to keep something from a previous test. + await page.evaluate(() => window.localStorage.clear()); await page.close(); }) );