Merge pull request #21154 from calixteman/bug2034804

Fix merging a PDF after a page deletion (bug 2034804)

src/core/editor/pdf_editor.js

@@ -660,11 +660,88 @@ class PDFEditor {
       }
     }
 
-    // Sort by insertAfter value so that each value is interpreted relative to
-    // the same base sequential sequence, then insert into the sequence.
-    // The offset accumulates the number of pages already inserted, converting
-    // base-relative positions to current-sequence positions.
     insertAfterList.sort((a, b) => a.insertAfter - b.insertAfter);
+
+    // Partial pageIndices would auto-fill into free slots at extraction time,
+    // which collides with the positions insertAfter shifts/assigns. Reject.
+    if (insertAfterList.length > 0) {
+      for (const info of pageInfos) {
+        if (!info.document || !info.pageIndices) {
+          continue;
+        }
+        const filteredCount = this.#getFilteredPageIndices(info).length;
+        if (info.pageIndices.length < filteredCount) {
+          throw new Error(
+            "extractPages: partial pageIndices cannot be combined with insertAfter entries."
+          );
+        }
+      }
+    }
+
+    // If the base sequential sequence is empty but some entries carry explicit
+    // pageIndices (e.g. after a deletion), resolve each insertAfter against
+    // that explicit layout: shift existing positions past the insertion point
+    // and fill the gap. Entries without a document are ignored. With no
+    // explicit entries we fall through to the sequential branch, whose
+    // Array.splice already clamps to position 0 on an empty sequence.
+    const hasExplicitLayout =
+      insertAfterList.length > 0 &&
+      pageInfos.some(info => info.document && info.pageIndices);
+    if (sequence.length === 0 && hasExplicitLayout) {
+      const updatedPageInfos = pageInfos.slice();
+      let maxExistingPos = -1;
+      for (const info of pageInfos) {
+        if (info.document && info.pageIndices) {
+          for (const idx of info.pageIndices) {
+            if (idx > maxExistingPos) {
+              maxExistingPos = idx;
+            }
+          }
+        }
+      }
+      let offset = 0;
+      for (const { i, insertAfter, count } of insertAfterList) {
+        // Clamp to [-1, maxExistingPos] so out-of-range values don't produce
+        // negative or gap-creating positions.
+        const threshold = Math.min(
+          Math.max(insertAfter, -1) + offset,
+          maxExistingPos
+        );
+        for (let j = 0; j < updatedPageInfos.length; j++) {
+          const existingInfo = updatedPageInfos[j];
+          if (
+            !existingInfo.document ||
+            !existingInfo.pageIndices ||
+            existingInfo.pageIndices.every(idx => idx <= threshold)
+          ) {
+            continue;
+          }
+          updatedPageInfos[j] = {
+            ...existingInfo,
+            pageIndices: existingInfo.pageIndices.map(idx =>
+              idx > threshold ? idx + count : idx
+            ),
+          };
+        }
+        const insertedIndices = [];
+        for (let k = 0; k < count; k++) {
+          insertedIndices.push(threshold + 1 + k);
+        }
+        const newInfo = {
+          ...updatedPageInfos[i],
+          pageIndices: insertedIndices,
+        };
+        delete newInfo.insertAfter;
+        updatedPageInfos[i] = newInfo;
+        offset += count;
+        maxExistingPos += count;
+      }
+      return updatedPageInfos;
+    }
+
+    // insertAfter values are relative to the base sequential sequence (stable
+    // across entries thanks to the sort above); offset converts them to
+    // current-sequence positions as we splice.
     let offset = 0;
     for (const { i, insertAfter, count } of insertAfterList) {
       const insertPos = insertAfter + 1 + offset;

src/core/worker.js

@@ -652,8 +652,7 @@ class WorkerMessageHandler {
           );
           return buffer;
         } catch (reason) {
-          // eslint-disable-next-line no-console
+          warn(`extractPages: "${reason}".`);
-          console.error(reason);
           return null;
         } finally {
           if (task) {

src/display/api.js

@@ -982,6 +982,26 @@ class PDFDocumentProxy {
    *  included ranges or indices.
    * @property {Array<Array<number>|number>} [excludePages]
    *  excluded ranges or indices.
+   * @property {Array<number>} [pageIndices] Explicit 0-based positions in the
+   *  final document for pages contributed by this entry. If shorter than the
+   *  filtered page list, the remaining pages are placed in the first free
+   *  slots at extraction time. Positions must not overlap with those of
+   *  other entries, and the union of all explicit/auto-filled positions
+   *  across the call must form a dense `[0, N)` range (where `N` is the
+   *  total page count of the final document) — sparse layouts leave empty
+   *  slots and are not supported. Cannot be combined with `insertAfter` on
+   *  the same entry, and must fully cover the filtered page list when any
+   *  entry in the same call specifies `insertAfter` (partial arrays are
+   *  rejected in that case).
+   * @property {number} [insertAfter] 0-based index in the base sequential
+   *  sequence (the concatenation of entries that have neither `pageIndices`
+   *  nor `insertAfter`) after which to insert the pages. When every
+   *  contributing entry carries explicit `pageIndices`, this is interpreted
+   *  against that explicit layout instead, shifting any existing positions
+   *  beyond the insertion point to make room. Use `-1` to insert before
+   *  everything. Values beyond the current layout are clamped so the pages
+   *  are appended at the end. Cannot be combined with `pageIndices` on the
+   *  same entry.
    */
 
   /**

test/integration/reorganize_pages_spec.mjs

@@ -3097,6 +3097,58 @@ describe("Reorganize Pages View", () => {
       );
     });
 
+    it("should merge a PDF after the current page when first page was deleted (bug 2034804)", async () => {
+      await Promise.all(
+        pages.map(async ([browserName, page]) => {
+          await waitForThumbnailVisible(page, 1);
+
+          // Select page 1 and delete it.
+          await waitAndClick(
+            page,
+            `.thumbnail:has(${getThumbnailSelector(1)}) input`
+          );
+          const handlePagesEdited = await waitForPagesEdited(page);
+          await waitAndClick(page, "#viewsManagerStatusActionButton");
+          await waitAndClick(page, "#viewsManagerStatusActionDelete");
+          await awaitPromise(handlePagesEdited);
+
+          // After deletion page 1 is the former page 2; navigate to it.
+          await page.waitForFunction(
+            () => window.PDFViewerApplication.page === 1
+          );
+          await waitAndClick(page, getThumbnailSelector(1));
+
+          const handleMerged = await createPromise(page, resolve => {
+            window.PDFViewerApplication.eventBus._on(
+              "thumbnailsloaded",
+              resolve,
+              { once: true }
+            );
+          });
+
+          const picker = await page.$("#viewsManagerAddFilePicker");
+          await picker.uploadFile(
+            path.join(__dirname, "../pdfs/three_pages_with_number.pdf")
+          );
+          await awaitPromise(handleMerged);
+
+          // 2 remaining original + 3 merged = 5 pages.
+          await page.waitForFunction(
+            () => parseInt(document.getElementById("pageNumber").max, 10) === 5
+          );
+
+          // Former page 2, the 3 merged pages, then former page 3.
+          await waitForHavingContents(page, [2, 1, 2, 3, 3]);
+
+          await waitForTextToBe(
+            page,
+            "#viewsManagerStatusActionLabel",
+            `${FSI}3${PDI} selected`
+          );
+        })
+      );
+    });
+
     it("must mark document as needing save after merge (bug 2034461)", async () => {
       await Promise.all(
         pages.map(async ([browserName, page]) => {

test/unit/api_spec.js

@@ -7124,6 +7124,183 @@ small scripts as well as for`);
 
         await loadingTask.destroy();
       });
+
+      it("insertAfter works when all base pages have explicit pageIndices", async function () {
+        // Mirrors a post-deletion merge: explicit pages 0 and 2 at pos 0/1,
+        // insert page 1 after pos 0 → "1"·"2"·"3".
+        let loadingTask = getDocument(
+          buildGetDocumentParams("page_with_number.pdf")
+        );
+        let pdfDoc = await loadingTask.promise;
+        const data = await pdfDoc.extractPages([
+          { document: null, includePages: [0, 2], pageIndices: [0, 1] },
+          { document: null, includePages: [1], insertAfter: 0 },
+        ]);
+        await loadingTask.destroy();
+
+        loadingTask = getDocument(data);
+        pdfDoc = await loadingTask.promise;
+        expect(pdfDoc.numPages).toEqual(3);
+
+        for (const [pageNum, expected] of [
+          [1, "1"],
+          [2, "2"],
+          [3, "3"],
+        ]) {
+          const pdfPage = await pdfDoc.getPage(pageNum);
+          const { items } = await pdfPage.getTextContent();
+          expect(mergeText(items))
+            .withContext(`Page ${pageNum}`)
+            .toEqual(expected);
+        }
+
+        await loadingTask.destroy();
+      });
+
+      it("two insertAfter entries at the same position with explicit pageIndices", async function () {
+        // Two insertAfter: 0 entries must land consecutively in entry order,
+        // yielding "1"·"2"·"3"·"4" (explicit pages 0 and 3 around them).
+        let loadingTask = getDocument(
+          buildGetDocumentParams("page_with_number.pdf")
+        );
+        let pdfDoc = await loadingTask.promise;
+        const data = await pdfDoc.extractPages([
+          { document: null, includePages: [0, 3], pageIndices: [0, 1] },
+          { document: null, includePages: [1], insertAfter: 0 },
+          { document: null, includePages: [2], insertAfter: 0 },
+        ]);
+        await loadingTask.destroy();
+
+        loadingTask = getDocument(data);
+        pdfDoc = await loadingTask.promise;
+        expect(pdfDoc.numPages).toEqual(4);
+
+        for (const [pageNum, expected] of [
+          [1, "1"],
+          [2, "2"],
+          [3, "3"],
+          [4, "4"],
+        ]) {
+          const pdfPage = await pdfDoc.getPage(pageNum);
+          const { items } = await pdfPage.getTextContent();
+          expect(mergeText(items))
+            .withContext(`Page ${pageNum}`)
+            .toEqual(expected);
+        }
+
+        await loadingTask.destroy();
+      });
+
+      it("insertAfter: -1 prepends pages when all base pages have explicit pageIndices", async function () {
+        // insertAfter: -1 prepends pages 0 and 1 before the explicit page 2
+        // → "1"·"2"·"3".
+        let loadingTask = getDocument(
+          buildGetDocumentParams("page_with_number.pdf")
+        );
+        let pdfDoc = await loadingTask.promise;
+        const data = await pdfDoc.extractPages([
+          { document: null, includePages: [2], pageIndices: [0] },
+          { document: null, includePages: [0, 1], insertAfter: -1 },
+        ]);
+        await loadingTask.destroy();
+
+        loadingTask = getDocument(data);
+        pdfDoc = await loadingTask.promise;
+        expect(pdfDoc.numPages).toEqual(3);
+
+        for (const [pageNum, expected] of [
+          [1, "1"],
+          [2, "2"],
+          [3, "3"],
+        ]) {
+          const pdfPage = await pdfDoc.getPage(pageNum);
+          const { items } = await pdfPage.getTextContent();
+          expect(mergeText(items))
+            .withContext(`Page ${pageNum}`)
+            .toEqual(expected);
+        }
+
+        await loadingTask.destroy();
+      });
+
+      it("insertAfter beyond the explicit layout appends pages at the end", async function () {
+        // Out-of-range insertAfter (5) must clamp to end rather than leave a
+        // gap → "1"·"2"·"3".
+        let loadingTask = getDocument(
+          buildGetDocumentParams("page_with_number.pdf")
+        );
+        let pdfDoc = await loadingTask.promise;
+        const data = await pdfDoc.extractPages([
+          { document: null, includePages: [0, 1], pageIndices: [0, 1] },
+          { document: null, includePages: [2], insertAfter: 5 },
+        ]);
+        await loadingTask.destroy();
+
+        loadingTask = getDocument(data);
+        pdfDoc = await loadingTask.promise;
+        expect(pdfDoc.numPages).toEqual(3);
+
+        for (const [pageNum, expected] of [
+          [1, "1"],
+          [2, "2"],
+          [3, "3"],
+        ]) {
+          const pdfPage = await pdfDoc.getPage(pageNum);
+          const { items } = await pdfPage.getTextContent();
+          expect(mergeText(items))
+            .withContext(`Page ${pageNum}`)
+            .toEqual(expected);
+        }
+
+        await loadingTask.destroy();
+      });
+
+      it("rejects partial pageIndices combined with insertAfter", async function () {
+        // Partial pageIndices + insertAfter could race over the same slots;
+        // the resolver throws and the worker surfaces it as a null result.
+        const loadingTask = getDocument(
+          buildGetDocumentParams("page_with_number.pdf")
+        );
+        const pdfDoc = await loadingTask.promise;
+        const data = await pdfDoc.extractPages([
+          { document: null, includePages: [0, 2], pageIndices: [0] },
+          { document: null, includePages: [1], insertAfter: 0 },
+        ]);
+        expect(data).toBeNull();
+
+        await loadingTask.destroy();
+      });
+
+      it("insertAfter alone (no base sequential or explicit layout) places pages from position 0", async function () {
+        // With no base layout, insertAfter must still land pages from pos 0
+        // (splice clamps on the empty sequence) → "1"·"2".
+        let loadingTask = getDocument(
+          buildGetDocumentParams("page_with_number.pdf")
+        );
+        let pdfDoc = await loadingTask.promise;
+        const data = await pdfDoc.extractPages([
+          { document: null, includePages: [0], insertAfter: 0 },
+          { document: null, includePages: [1], insertAfter: 0 },
+        ]);
+        await loadingTask.destroy();
+
+        loadingTask = getDocument(data);
+        pdfDoc = await loadingTask.promise;
+        expect(pdfDoc.numPages).toEqual(2);
+
+        for (const [pageNum, expected] of [
+          [1, "1"],
+          [2, "2"],
+        ]) {
+          const pdfPage = await pdfDoc.getPage(pageNum);
+          const { items } = await pdfPage.getTextContent();
+          expect(mergeText(items))
+            .withContext(`Page ${pageNum}`)
+            .toEqual(expected);
+        }
+
+        await loadingTask.destroy();
+      });
     });
   });
 });