Merge pull request #16911 from calixteman/issue16872

[Editor] Avoid to use parent of editors in destroyed pages

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);
     }
 

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)) {

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) {

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");
+        })
+      );
+    });
+  });
 });

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();
     })
   );