From 75cb69eef2575863440610997d77c595df5cfca9 Mon Sep 17 00:00:00 2001 From: Calixte Denizet Date: Tue, 17 Mar 2026 19:37:59 +0100 Subject: [PATCH] Fix various bug around copy/paste/delete/undo (bug 2022586, bug 2022824, bug 2022884, bug 2023171, bug 2023176) Those bugs are more or less related so it's why they're all fixed together in the same patch. --- src/display/api.js | 93 +--- src/display/pages_mapper.js | 309 +++++------ test/integration/reorganize_pages_spec.mjs | 596 +++++++++++++++++++++ web/pdf_page_view.js | 2 +- web/pdf_thumbnail_view.js | 4 +- web/pdf_thumbnail_viewer.js | 46 +- web/pdf_viewer.js | 19 +- 7 files changed, 795 insertions(+), 274 deletions(-) diff --git a/src/display/api.js b/src/display/api.js index 4cc16a7b9..b14833647 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -1360,6 +1360,19 @@ class PDFPageProxy { this.imageCoordinates = null; } + clone(id) { + const clone = new PDFPageProxy( + id, + this._pageInfo, + this._transport, + this.#pagesMapper, + this._pdfBug + ); + clone.clonedFromIndex = this.clonedFromIndex ?? this._pageIndex; + this._transport.updatePage(clone); + return clone; + } + /** * @type {number} Page number of the page. First page is 1. */ @@ -1372,6 +1385,7 @@ class PDFPageProxy { */ set pageNumber(value) { this._pageIndex = value - 1; + this._transport.updatePage(this); } /** @@ -2428,10 +2442,6 @@ class WorkerTransport { #passwordCapability = null; - #copiedPageInfo = null; - - #savedPageInfo = null; - constructor( messageHandler, loadingTask, @@ -2465,7 +2475,6 @@ class WorkerTransport { this.setupMessageHandler(); this.pagesMapper = pagesMapper; - this.pagesMapper.addListener(this.#updateCaches.bind(this)); if (typeof PDFJSDev === "undefined" || PDFJSDev.test("TESTING")) { // For testing purposes. @@ -2491,76 +2500,10 @@ class WorkerTransport { } } - #updateCaches({ type, pageNumbers }) { - if (type === "copy") { - this.#copiedPageInfo = new Map(); - for (const pageNum of pageNumbers) { - this.#copiedPageInfo.set(pageNum, { - proxy: this.#pageCache.get(pageNum - 1) || null, - promise: this.#pagePromises.get(pageNum - 1) || null, - }); - } - return; - } - - if (type === "cancelCopy") { - this.#copiedPageInfo = null; - return; - } - - if (type === "delete") { - this.#savedPageInfo = { - pageCache: new Map(this.#pageCache), - pagePromises: new Map(this.#pagePromises), - }; - for (const pageNum of pageNumbers) { - this.#pageCache.delete(pageNum - 1); - this.#pagePromises.delete(pageNum - 1); - } - } - - if (type === "cancelDelete") { - if (this.#savedPageInfo) { - this.#pageCache = this.#savedPageInfo.pageCache; - this.#pagePromises = this.#savedPageInfo.pagePromises; - this.#savedPageInfo = null; - } - return; - } - - if (type === "cleanSavedData") { - this.#savedPageInfo = null; - return; - } - - const newPageCache = new Map(); - const newPromiseCache = new Map(); - const { pagesMapper } = this; - for (let i = 0, ii = pagesMapper.pagesNumber; i < ii; i++) { - const prevPageNumber = pagesMapper.getPrevPageNumber(i + 1); - if (prevPageNumber < 0) { - const { proxy, promise } = - this.#copiedPageInfo?.get(-prevPageNumber) || {}; - if (proxy) { - newPageCache.set(i, proxy); - } - if (promise) { - newPromiseCache.set(i, promise); - } - continue; - } - const prevPageIndex = prevPageNumber - 1; - const page = this.#pageCache.get(prevPageIndex); - if (page) { - newPageCache.set(i, page); - } - const promise = this.#pagePromises.get(prevPageIndex); - if (promise) { - newPromiseCache.set(i, promise); - } - } - this.#pageCache = newPageCache; - this.#pagePromises = newPromiseCache; + updatePage(page) { + const { _pageIndex } = page; + this.#pageCache.set(_pageIndex, page); + this.#pagePromises.set(_pageIndex, Promise.resolve(page)); } #cacheSimpleMethod(name, data = null) { diff --git a/src/display/pages_mapper.js b/src/display/pages_mapper.js index 2ada560d6..1a4654fd8 100644 --- a/src/display/pages_mapper.js +++ b/src/display/pages_mapper.js @@ -19,146 +19,75 @@ import { makeArr, MathClamp } from "../shared/util.js"; * Maps between page IDs and page numbers, allowing bidirectional conversion * between the two representations. This is useful when the page numbering * in the PDF document doesn't match the default sequential ordering. + * + * When #pageNumberToId is null the mapping is the identity (page N has ID N). */ class PagesMapper { /** - * Maps page IDs to their corresponding page numbers. - * @type {Map>|null} - */ - #idToPageNumber = null; - - /** - * Maps page numbers to their corresponding page IDs. + * Maps page positions (0-indexed) to their page IDs (1-indexed). + * Null when the mapping is the identity. * @type {Uint32Array|null} */ #pageNumberToId = null; /** - * Previous mapping of page IDs to page numbers. + * Previous page number for each position, used to track what happened to + * each page after a mutation. Negative values indicate copied pages. * @type {Int32Array|null} */ #prevPageNumbers = null; - /** - * The total number of pages. - * @type {number} - */ + /** @type {number} */ #pagesNumber = 0; /** - * Listeners for page changes. - * @type {Array} + * Clipboard state for copy/paste operations. + * @type {{pageNumbers: Uint32Array, pageIds: Uint32Array}|null} */ - #listeners = []; - - /** - * Maps page numbers to their corresponding page IDs (used in copy - * operations). - * @type {Uint32Array|null} - */ - #copiedPageIds = null; - - /** - * Maps page IDs to their corresponding page numbers, used in copy operations. - * @type {Uint32Array|null} - */ - #copiedPageNumbers = null; + #clipboard = null; + /** Saved state for undoing a delete. */ #savedData = null; - /** - * Gets the total number of pages. - * @returns {number} The number of pages. - */ get pagesNumber() { return this.#pagesNumber; } - /** - * Sets the total number of pages and initializes default mappings - * where page IDs equal page numbers (1-indexed). - * @param {number} n - The total number of pages. - */ set pagesNumber(n) { if (this.#pagesNumber === n) { return; } this.#pagesNumber = n; - this.#reset(); - } - - /** - * Resets the page mappings to their default state, where page IDs equal page - * numbers (1-indexed). This is called when the number of pages changes, or - * when the current mapping matches the default mapping after a move - * operation. - */ - #reset() { this.#pageNumberToId = null; - this.#idToPageNumber = null; + this.#prevPageNumbers = null; } /** - * Adds a listener function that will be called whenever the page mappings - * are updated. - * @param {function} listener + * Ensures the identity mapping is initialized. + * Must be called before any mutation or before reading #pageNumberToId. */ - addListener(listener) { - this.#listeners.push(listener); - } - - /** - * Removes a previously added listener function. - * @param {function} listener - */ - removeListener(listener) { - const index = this.#listeners.indexOf(listener); - if (index >= 0) { - this.#listeners.splice(index, 1); - } - } - - /** - * Calls all registered listener functions to notify them of changes to the - * page mappings. - * @param {Object} data - An object containing information about the update. - */ - #updateListeners(data) { - for (const listener of this.#listeners) { - listener(data); - } - } - - /** - * Initializes the page mappings if they haven't been initialized yet. - * @param {boolean} mustInit - */ - #init(mustInit) { + #ensureInit() { if (this.#pageNumberToId) { return; } const n = this.#pagesNumber; - const pageNumberToId = (this.#pageNumberToId = new Uint32Array(n)); - this.#prevPageNumbers = new Int32Array(pageNumberToId); - const idToPageNumber = (this.#idToPageNumber = new Map()); - if (mustInit) { - for (let i = 1; i <= n; i++) { - pageNumberToId[i - 1] = i; - idToPageNumber.set(i, [i]); - } + for (let i = 0; i < n; i++) { + pageNumberToId[i] = i + 1; } + this.#prevPageNumbers = new Int32Array(pageNumberToId); } /** - * Updates the mapping from page IDs to page numbers based on the current - * mapping from page numbers to page IDs. This should be called after any - * changes to the page-number-to-ID mapping to keep the two mappings in sync. + * Builds and returns the inverse map (id to page numbers) from + * #pageNumberToId. + * Since a page ID can appear multiple times (after a copy), the value is an + * array of all page numbers that share that ID. + * @returns {Map>} */ - #updateIdToPageNumber() { - const idToPageNumber = this.#idToPageNumber; + #buildIdToPageNumber() { + const idToPageNumber = new Map(); const pageNumberToId = this.#pageNumberToId; - idToPageNumber.clear(); for (let i = 0, ii = this.#pagesNumber; i < ii; i++) { const id = pageNumberToId[i]; const pageNumbers = idToPageNumber.get(id); @@ -168,20 +97,20 @@ class PagesMapper { idToPageNumber.set(id, [i + 1]); } } + return idToPageNumber; } /** - * Move a set of pages to a new position while keeping ID→number mappings in - * sync. + * Move a set of pages to a new position. * * @param {Set} selectedPages - Page numbers being moved (1-indexed). * @param {number[]} pagesToMove - Ordered list of page numbers to move. * @param {number} index - Zero-based insertion index in the page-number list. */ movePages(selectedPages, pagesToMove, index) { - this.#init(true); + this.#ensureInit(); const pageNumberToId = this.#pageNumberToId; - const idToPageNumber = this.#idToPageNumber; + const prevIdToPageNumber = this.#buildIdToPageNumber(); const movedCount = pagesToMove.length; const mappedPagesToMove = new Uint32Array(movedCount); let removedBeforeTarget = 0; @@ -190,63 +119,60 @@ class PagesMapper { const pageIndex = pagesToMove[i] - 1; mappedPagesToMove[i] = pageNumberToId[pageIndex]; if (pageIndex < index) { - removedBeforeTarget += 1; + removedBeforeTarget++; } } const pagesNumber = this.#pagesNumber; - // target index after removing elements that were before it - let adjustedTarget = index - removedBeforeTarget; const remainingLen = pagesNumber - movedCount; - adjustedTarget = MathClamp(adjustedTarget, 0, remainingLen); + const adjustedTarget = MathClamp( + index - removedBeforeTarget, + 0, + remainingLen + ); - // Create the new mapping. - // First copy over the pages that are not being moved. - // Then insert the moved pages at the target position. + // Compact: keep only non-moved pages. for (let i = 0, r = 0; i < pagesNumber; i++) { if (!selectedPages.has(i + 1)) { pageNumberToId[r++] = pageNumberToId[i]; } } - // Shift the pages after the target position. + // Make room at the target and insert. pageNumberToId.copyWithin( adjustedTarget + movedCount, adjustedTarget, remainingLen ); - // Finally insert the moved pages. pageNumberToId.set(mappedPagesToMove, adjustedTarget); - this.#setPrevPageNumbers(idToPageNumber, null); - this.#updateIdToPageNumber(); - this.#updateListeners({ type: "move" }); + this.#updatePrevPageNumbers(prevIdToPageNumber); if (pageNumberToId.every((id, i) => id === i + 1)) { - this.#reset(); + this.#pageNumberToId = null; } } /** - * Deletes a set of pages while keeping ID→number mappings in sync. - * @param {Array} pagesToDelete - Page numbers to delete (1-indexed). - * These must be unique and sorted in ascending order. + * Deletes a set of pages. + * @param {Array} pagesToDelete - Page numbers to delete (1-indexed), + * unique and sorted ascending. */ deletePages(pagesToDelete) { - this.#init(true); + this.#ensureInit(); const pageNumberToId = this.#pageNumberToId; - const prevIdToPageNumber = this.#idToPageNumber; + const prevIdToPageNumber = this.#buildIdToPageNumber(); this.#savedData = { pageNumberToId: pageNumberToId.slice(), - idToPageNumber: new Map(prevIdToPageNumber), - pageNumber: this.#pagesNumber, + pagesNumber: this.#pagesNumber, prevPageNumbers: this.#prevPageNumbers.slice(), }; - this.pagesNumber -= pagesToDelete.length; - this.#init(false); - const newPageNumberToId = this.#pageNumberToId; + const newN = this.#pagesNumber - pagesToDelete.length; + this.#pagesNumber = newN; + const newPageNumberToId = (this.#pageNumberToId = new Uint32Array(newN)); + this.#prevPageNumbers = new Int32Array(newN); let sourceIndex = 0; let destIndex = 0; @@ -265,107 +191,119 @@ class PagesMapper { newPageNumberToId.set(pageNumberToId.subarray(sourceIndex), destIndex); } - this.#setPrevPageNumbers(prevIdToPageNumber, null); - this.#updateIdToPageNumber(); - this.#updateListeners({ type: "delete", pageNumbers: pagesToDelete }); + this.#updatePrevPageNumbers(prevIdToPageNumber, new Set(pagesToDelete)); } cancelDelete() { if (this.#savedData) { this.#pageNumberToId = this.#savedData.pageNumberToId; - this.#idToPageNumber = this.#savedData.idToPageNumber; - this.pagesNumber = this.#savedData.pageNumber; + this.#pagesNumber = this.#savedData.pagesNumber; this.#prevPageNumbers = this.#savedData.prevPageNumbers; this.#savedData = null; - this.#updateListeners({ type: "cancelDelete" }); } } cleanSavedData() { this.#savedData = null; - this.#updateListeners({ type: "cleanSavedData" }); } /** - * Copies a set of pages while keeping ID→number mappings in sync. + * Records which pages are being copied so that pastePages can insert them. * @param {Uint32Array} pagesToCopy - Page numbers to copy (1-indexed). */ copyPages(pagesToCopy) { - this.#init(true); - this.#copiedPageNumbers = pagesToCopy; - this.#copiedPageIds = pagesToCopy.map( - pageNumber => this.#pageNumberToId[pageNumber - 1] - ); - this.#updateListeners({ type: "copy", pageNumbers: pagesToCopy }); + this.#ensureInit(); + this.#clipboard = { + pageNumbers: pagesToCopy, + pageIds: pagesToCopy.map(n => this.#pageNumberToId[n - 1]), + }; } cancelCopy() { - this.#copiedPageIds = null; - this.#copiedPageNumbers = null; - this.#updateListeners({ type: "cancelCopy" }); + this.#clipboard = null; } /** - * Pastes a set of pages while keeping ID→number mappings in sync. + * Inserts the previously copied pages at the given position. * @param {number} index - Zero-based insertion index in the page-number list. */ pastePages(index) { - this.#init(true); + this.#ensureInit(); const pageNumberToId = this.#pageNumberToId; - const prevIdToPageNumber = this.#idToPageNumber; - const copiedPageNumbers = this.#copiedPageNumbers; + const prevIdToPageNumber = this.#buildIdToPageNumber(); + const { pageNumbers: copiedPageNumbers, pageIds: copiedPageIds } = + this.#clipboard; - const copiedPageMapping = new Map(); - let base = index; - for (const pageNumber of copiedPageNumbers) { - copiedPageMapping.set(++base, pageNumber); - } - this.pagesNumber += copiedPageNumbers.length; - this.#init(false); - const newPageNumberToId = this.#pageNumberToId; + const newN = this.#pagesNumber + copiedPageNumbers.length; + this.#pagesNumber = newN; + const newPageNumberToId = (this.#pageNumberToId = new Uint32Array(newN)); + this.#prevPageNumbers = new Int32Array(newN); newPageNumberToId.set(pageNumberToId.subarray(0, index), 0); - newPageNumberToId.set(this.#copiedPageIds, index); + newPageNumberToId.set(copiedPageIds, index); newPageNumberToId.set( pageNumberToId.subarray(index), index + copiedPageNumbers.length ); - this.#setPrevPageNumbers(prevIdToPageNumber, copiedPageMapping); - this.#updateIdToPageNumber(); - this.#updateListeners({ type: "paste" }); - - this.#copiedPageIds = null; - this.#copiedPageNumbers = null; + this.#updatePrevPageNumbers( + prevIdToPageNumber, + null, + index, + copiedPageNumbers + ); + this.#clipboard = null; } /** - * Updates the previous page numbers based on the current page-number-to-ID - * mapping and the provided previous ID-to-page-number mapping. - * This is used to keep track of the original page numbers for each page ID. - * @param {Map} prevIdToPageNumber - * @param {Map|null} copiedPageMapping + * Recomputes #prevPageNumbers after a mutation, using the pre-mutation + * id to pageNumbers map to track where each page came from. + * + * @param {Map>} prevIdToPageNumber - Id to pageNumbers + * before the mutation. + * @param {Set|null} [deletedPageNumbers] - Page numbers that were + * deleted (so their old positions are skipped). + * @param {number} [pasteIndex] - If this is a paste, the zero-based + * insertion index; paired with copiedPageNumbers. + * @param {Uint32Array} [copiedPageNumbers] - Source page numbers of the + * pasted pages; paired with pasteIndex. */ - #setPrevPageNumbers(prevIdToPageNumber, copiedPageMapping) { + #updatePrevPageNumbers( + prevIdToPageNumber, + deletedPageNumbers = null, + pasteIndex = -1, + copiedPageNumbers = null + ) { const prevPageNumbers = this.#prevPageNumbers; const newPageNumberToId = this.#pageNumberToId; + const pasteEnd = pasteIndex + (copiedPageNumbers?.length ?? 0); const idsIndices = new Map(); + for (let i = 0, ii = this.#pagesNumber; i < ii; i++) { - const oldPageNumber = copiedPageMapping?.get(i + 1); - if (oldPageNumber) { - prevPageNumbers[i] = -oldPageNumber; + if (i >= pasteIndex && i < pasteEnd) { + // Negative value signals this page is a copy; encodes its source. + prevPageNumbers[i] = -copiedPageNumbers[i - pasteIndex]; continue; } const id = newPageNumberToId[i]; - const j = idsIndices.get(id) || 0; - prevPageNumbers[i] = prevIdToPageNumber.get(id)?.[j]; + const oldPositions = prevIdToPageNumber.get(id); + let j = idsIndices.get(id) || 0; + if (deletedPageNumbers && oldPositions) { + while ( + j < oldPositions.length && + deletedPageNumbers.has(oldPositions[j]) + ) { + j++; + } + } + prevPageNumbers[i] = oldPositions?.[j]; idsIndices.set(id, j + 1); } } /** * Checks if the page mappings have been altered from their initial state. - * @returns {boolean} True if the mappings have been altered, false otherwise. + * @returns {boolean} */ hasBeenAltered() { return this.#pageNumberToId !== null; @@ -373,9 +311,11 @@ class PagesMapper { /** * Gets the current page mapping suitable for saving. - * @returns {Object} An object containing the page indices. + * @param {Map>} [idToPageNumber] + * @returns {Array} */ - getPageMappingForSaving(idToPageNumber = this.#idToPageNumber) { + getPageMappingForSaving(idToPageNumber = null) { + idToPageNumber ??= this.#buildIdToPageNumber(); // idToPageNumber maps used 1-based IDs to 1-based page numbers. // For example if the final pdf contains page 3 twice and they are moved at // page 1 and 4, then it contains: @@ -441,28 +381,37 @@ class PagesMapper { /** * Gets the previous page number for a given page number. + * Negative values indicate a copied page (the absolute value is the source). * @param {number} pageNumber - * @returns {number} The previous page number for the given page number, or 0 - * if no mapping exists. + * @returns {number} */ getPrevPageNumber(pageNumber) { - return this.#prevPageNumbers[pageNumber - 1] ?? 0; + return this.#prevPageNumbers?.[pageNumber - 1] ?? 0; } /** - * Gets the page number for a given page ID. + * Gets the first page number that currently maps to the given page ID. * @param {number} id - The page ID (1-indexed). - * @returns {number} The page number, or 0 if no mapping exists. + * @returns {number} The page number, or 0 if not found. */ getPageNumber(id) { - return this.#idToPageNumber ? (this.#idToPageNumber.get(id)?.[0] ?? 0) : id; + if (!this.#pageNumberToId) { + return id; // identity mapping + } + const pageNumberToId = this.#pageNumberToId; + for (let i = 0, ii = this.#pagesNumber; i < ii; i++) { + if (pageNumberToId[i] === id) { + return i + 1; + } + } + return 0; } /** * Gets the page ID for a given page number. * @param {number} pageNumber - The page number (1-indexed). * @returns {number} The page ID, or the page number itself if no mapping - * exists. + * exists. */ getPageId(pageNumber) { return this.#pageNumberToId?.[pageNumber - 1] ?? pageNumber; diff --git a/test/integration/reorganize_pages_spec.mjs b/test/integration/reorganize_pages_spec.mjs index f0be6c59e..2ee980053 100644 --- a/test/integration/reorganize_pages_spec.mjs +++ b/test/integration/reorganize_pages_spec.mjs @@ -32,6 +32,7 @@ import { scrollIntoView, showViewsManager, waitAndClick, + waitForBrowserTrip, waitForDOMMutation, waitForTextToBe, waitForTooltipToBe, @@ -1819,6 +1820,122 @@ describe("Reorganize Pages View", () => { `In ${browserName}, dragging should be disabled when pasting` ) .toBeUndefined(); + + // Wait a tick to ensure that the controller.abort() has taken effect + // before leaving. + await waitForBrowserTrip(page); + }) + ); + }); + }); + + describe("Copy, paste and delete pages with keyboard shortcuts", () => { + let pages; + + beforeEach(async () => { + pages = await loadAndWait( + "page_with_number.pdf", + "#viewsManagerToggleButton", + "page-width", + null, + { enableSplitMerge: true } + ); + }); + + afterEach(async () => { + await closePages(pages); + }); + + it("should copy, paste and delete pages correctly", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + await waitForThumbnailVisible(page, 1); + + // Check thumbnail 1 and copy it with Ctrl+C. + await waitAndClick( + page, + `.thumbnail:has(${getThumbnailSelector(1)}) input` + ); + let handlePagesEdited = await waitForPagesEdited(page, "copy"); + await kbCopy(page); + let pageIndices = await awaitPromise(handlePagesEdited); + let expected = [ + 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, + ]; + expect(pageIndices) + .withContext(`In ${browserName}`) + .toEqual(expected); + + // Paste after page 3. + handlePagesEdited = await waitForPagesEdited(page); + await waitAndClick(page, `${getThumbnailSelector(3)}+button`); + pageIndices = await awaitPromise(handlePagesEdited); + expected = [ + 1, 2, 3, 1, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, + ]; + expect(pageIndices) + .withContext(`In ${browserName}`) + .toEqual(expected); + + // Check thumbnail 1 and delete it. + await waitAndClick( + page, + `.thumbnail:has(${getThumbnailSelector(1)}) input` + ); + handlePagesEdited = await waitForPagesEdited(page); + await kbDelete(page); + + pageIndices = await awaitPromise(handlePagesEdited); + expected = [ + 2, 3, 1, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, + ]; + expect(pageIndices) + .withContext(`In ${browserName}`) + .toEqual(expected); + + // Check thumbnail 4 and copy it with Ctrl+C. + await waitAndClick( + page, + `.thumbnail:has(${getThumbnailSelector(3)}) input` + ); + handlePagesEdited = await waitForPagesEdited(page, "copy"); + await kbCopy(page); + pageIndices = await awaitPromise(handlePagesEdited); + expected = [ + 2, 3, 1, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, + ]; + expect(pageIndices) + .withContext(`In ${browserName}`) + .toEqual(expected); + + // Paste before page 1. + handlePagesEdited = await waitForPagesEdited(page); + await waitAndClick( + page, + `button.thumbnailPasteButton:has(+ ${getThumbnailSelector(1)})` + ); + pageIndices = await awaitPromise(handlePagesEdited); + expected = [ + 1, 2, 3, 1, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, + ]; + expect(pageIndices) + .withContext(`In ${browserName}`) + .toEqual(expected); + + // Check thumbnail 5 and delete it. + await waitAndClick( + page, + `.thumbnail:has(${getThumbnailSelector(4)}) input` + ); + handlePagesEdited = await waitForPagesEdited(page); + await kbDelete(page); + pageIndices = await awaitPromise(handlePagesEdited); + expected = [ + 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, + ]; + expect(pageIndices) + .withContext(`In ${browserName}`) + .toEqual(expected); }) ); }); @@ -1967,4 +2084,483 @@ describe("Reorganize Pages View", () => { ); }); }); + + describe("Dismissing undo after copy, paste and delete doesn't destroy the visible thumbnail", () => { + let pages; + + beforeEach(async () => { + pages = await loadAndWait( + "two_pages.pdf", + "#viewsManagerToggleButton", + "page-fit", + null, + { enableSplitMerge: true } + ); + }); + + afterEach(async () => { + await closePages(pages); + }); + + it("should keep the pasted thumbnail visible after dismissing the undo bar", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + await waitForThumbnailVisible(page, 1); + await page.waitForSelector("#viewsManagerStatusActionButton", { + visible: true, + }); + + // Wait for both thumbnails to be fully rendered. + for (let i = 1; i <= 2; i++) { + await page.waitForSelector( + `${getThumbnailSelector(i)} > img[src^="blob:http:"]`, + { visible: true } + ); + } + + // Copy page 1. + await waitAndClick( + page, + `.thumbnail:has(${getThumbnailSelector(1)}) input` + ); + let handlePagesEdited = await waitForPagesEdited(page, "copy"); + await waitAndClick(page, "#viewsManagerStatusActionButton"); + await waitAndClick(page, "#viewsManagerStatusActionCopy"); + await awaitPromise(handlePagesEdited); + + // Paste after page 1 (the pasted copy lands at position 2). + handlePagesEdited = await waitForPagesEdited(page); + await waitAndClick(page, `${getThumbnailSelector(1)}+button`); + await awaitPromise(handlePagesEdited); + + // Select the original page 1 (still at position 1) and delete it. + await waitAndClick( + page, + `.thumbnail:has(${getThumbnailSelector(1)}) input` + ); + handlePagesEdited = await waitForPagesEdited(page); + await waitAndClick(page, "#viewsManagerStatusActionButton"); + await waitAndClick(page, "#viewsManagerStatusActionDelete"); + await awaitPromise(handlePagesEdited); + + await page.waitForSelector("#viewsManagerStatusUndo", { + visible: true, + }); + + // Dismiss the undo bar. Without the fix, this would destroy the + // thumbnail at position 1 (the pasted copy) because both the saved + // state and the current _thumbnails array pointed to the same object. + handlePagesEdited = await waitForPagesEdited(page, "cleanSavedData"); + await waitAndClick(page, "#viewsManagerStatusUndoCloseButton"); + await awaitPromise(handlePagesEdited); + + // The thumbnail at position 1 (the pasted copy) must still be + // rendered and visible with a valid image source. + await page.waitForSelector( + `${getThumbnailSelector(1)} > img[src^="blob:http:"]`, + { visible: true } + ); + }) + ); + }); + }); + + describe("Undo after drag-and-drop move followed by delete restores correct page mapping", () => { + let pages; + + beforeEach(async () => { + pages = await loadAndWait( + "page_with_number.pdf", + "#viewsManagerToggleButton", + "1", + null, + { enableSplitMerge: true } + ); + }); + + afterEach(async () => { + await closePages(pages); + }); + + it("should restore the post-move mapping when undoing a delete", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + await waitForThumbnailVisible(page, 1); + const rect1 = await getRect(page, getThumbnailSelector(1)); + const rect2 = await getRect(page, getThumbnailSelector(2)); + + // Move page 1 after page 2: mapping becomes [2, 1, 3, …, 17]. + let handlePagesEdited = await waitForPagesEdited(page); + await dragAndDrop( + page, + getThumbnailSelector(1), + [[0, rect2.y - rect1.y + rect2.height / 2]], + 10 + ); + let pageIndices = await awaitPromise(handlePagesEdited); + let expected = [ + 2, 1, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, + ]; + expect(pageIndices) + .withContext(`In ${browserName}`) + .toEqual(expected); + + // Select the new first thumbnail (originally page 2) and delete it. + await waitAndClick( + page, + `.thumbnail:has(${getThumbnailSelector(1)}) input` + ); + handlePagesEdited = await waitForPagesEdited(page); + await waitAndClick(page, "#viewsManagerStatusActionButton"); + await waitAndClick(page, "#viewsManagerStatusActionDelete"); + pageIndices = await awaitPromise(handlePagesEdited); + expected = [1, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17]; + expect(pageIndices) + .withContext(`In ${browserName}`) + .toEqual(expected); + + await page.waitForSelector("#viewsManagerStatusUndo", { + visible: true, + }); + + // Undo the delete. Without the fix, cancelDelete() would call the + // pagesNumber setter which triggers #reset(), wiping out the + // just-restored pageNumberToId and idToPageNumber mappings, leaving + // the mapper in identity state despite the prior move. + handlePagesEdited = await waitForPagesEdited(page, "cancelDelete"); + await waitAndClick(page, "#viewsManagerStatusUndoButton"); + pageIndices = await awaitPromise(handlePagesEdited); + expected = [ + 2, 1, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, + ]; + expect(pageIndices) + .withContext(`In ${browserName}`) + .toEqual(expected); + + await waitForHavingContents(page, expected); + }) + ); + }); + }); + + describe("Keyboard Delete is ignored while in paste mode", () => { + let pages; + + beforeEach(async () => { + pages = await loadAndWait( + "page_with_number.pdf", + "#viewsManagerToggleButton", + "1", + null, + { enableSplitMerge: true } + ); + }); + + afterEach(async () => { + await closePages(pages); + }); + + it("should not delete pages when pressing Delete while in paste mode", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + await waitForThumbnailVisible(page, 1); + await page.waitForSelector("#viewsManagerStatusActionButton", { + visible: true, + }); + + // Copy page 1 to enter paste mode. + await waitAndClick( + page, + `.thumbnail:has(${getThumbnailSelector(1)}) input` + ); + const handlePagesEdited = await waitForPagesEdited(page, "copy"); + await waitAndClick(page, "#viewsManagerStatusActionButton"); + await waitAndClick(page, "#viewsManagerStatusActionCopy"); + await awaitPromise(handlePagesEdited); + + // Paste mode must be active (paste buttons visible). + await page.waitForSelector("button.thumbnailPasteButton", { + visible: true, + }); + + // Focus an element inside the thumbnail container so that the + // keydown handler can fire. Checkboxes are hidden in paste mode + // (CSS: pasteMode > .thumbnail > input { display: none }), so we + // click the imageContainer itself instead. + await waitAndClick(page, getThumbnailSelector(1)); + + // Press Delete: must be a no-op while in paste mode. + await kbDelete(page); + await waitForBrowserTrip(page); + + // The page count must remain at 17. + const pageCount = await page.$eval("#pageNumber", el => el.max); + expect(parseInt(pageCount, 10)) + .withContext(`In ${browserName}`) + .toBe(17); + + // Paste buttons must still be present (paste mode not disrupted). + const pasteButtons = await page.$$("button.thumbnailPasteButton"); + expect(pasteButtons.length) + .withContext(`In ${browserName}`) + .toBeGreaterThan(0); + }) + ); + }); + }); + + describe("Clicking Done after two copy+paste rounds must not remove the pasted pages (bug 2023934)", () => { + let pages; + + beforeEach(async () => { + pages = await loadAndWait( + "page_with_number.pdf", + "#viewsManagerToggleButton", + "1", + null, + { enableSplitMerge: true } + ); + }); + + afterEach(async () => { + await closePages(pages); + }); + + it("should keep all pasted pages after clicking Done without pasting", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + await waitForThumbnailVisible(page, 1); + await page.waitForSelector("#viewsManagerStatusActionButton", { + visible: true, + }); + + // Helper: copy page 1, click a paste button, await the paste event. + async function copyAndPaste() { + await waitAndClick( + page, + `.thumbnail:has(${getThumbnailSelector(1)}) input` + ); + let handlePagesEdited = await waitForPagesEdited(page, "copy"); + await waitAndClick(page, "#viewsManagerStatusActionButton"); + await waitAndClick(page, "#viewsManagerStatusActionCopy"); + await awaitPromise(handlePagesEdited); + + handlePagesEdited = await waitForPagesEdited(page); + // Click the paste button that appears after thumbnail 1. + await waitAndClick(page, `${getThumbnailSelector(1)}+button`); + await awaitPromise(handlePagesEdited); + } + + // Round 1: copy page 1 and paste → 18 pages. + await copyAndPaste(); + let pageCount = await page.$eval("#pageNumber", el => el.max); + expect(parseInt(pageCount, 10)) + .withContext(`In ${browserName} after 1st paste`) + .toBe(18); + + // Round 2: copy page 1 and paste → 19 pages. + await copyAndPaste(); + pageCount = await page.$eval("#pageNumber", el => el.max); + expect(parseInt(pageCount, 10)) + .withContext(`In ${browserName} after 2nd paste`) + .toBe(19); + + // STR step 4: select page 1, copy (enter paste mode), then click + // "Done" without pasting. Without the fix, clicking Done also + // triggers a restore of the saved pre-paste thumbnails, which removes + // one of the pasted pages and corrupts the state. + await waitAndClick( + page, + `.thumbnail:has(${getThumbnailSelector(1)}) input` + ); + const handlePagesEdited = await waitForPagesEdited(page, "copy"); + await waitAndClick(page, "#viewsManagerStatusActionButton"); + await waitAndClick(page, "#viewsManagerStatusActionCopy"); + await awaitPromise(handlePagesEdited); + + const handleCancelCopy = await waitForPagesEdited(page, "cancelCopy"); + await waitAndClick(page, "#viewsManagerStatusUndoButton"); + await awaitPromise(handleCancelCopy); + + // Page count must still be 19 – Done should not have removed a page. + pageCount = await page.$eval("#pageNumber", el => el.max); + expect(parseInt(pageCount, 10)) + .withContext(`In ${browserName} after Done`) + .toBe(19); + + // STR step 5: copy page 1 and paste → must reach 20 without crash. + await copyAndPaste(); + pageCount = await page.$eval("#pageNumber", el => el.max); + expect(parseInt(pageCount, 10)) + .withContext(`In ${browserName} after 3rd paste`) + .toBe(20); + + // Sidebar must still show all 20 thumbnails. + const thumbnails = await page.$$( + ".thumbnail .thumbnailImageContainer" + ); + expect(thumbnails.length) + .withContext(`In ${browserName} thumbnail count`) + .toBe(20); + }) + ); + }); + }); + + describe("Selection counter must be cleared after drag-and-drop (bug 2022884)", () => { + let pages; + + beforeEach(async () => { + pages = await loadAndWait( + "page_with_number.pdf", + "#viewsManagerToggleButton", + "1", + null, + { enableSplitMerge: true } + ); + }); + + afterEach(async () => { + await closePages(pages); + }); + + it("should hide the selection counter after dropping selected pages", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + await waitForThumbnailVisible(page, 1); + + // Select page 1 via its checkbox. + await waitAndClick( + page, + `.thumbnail:has(${getThumbnailSelector(1)}) input` + ); + + // The deselect button must now be visible ("1 page selected"). + await page.waitForSelector( + "#viewsManagerStatusActionDeselectButton:not(.hidden)", + { visible: true } + ); + + // Capture rects after the status-bar layout has settled so the + // drag displacement accounts for the correct positions. + const rect1 = await getRect(page, getThumbnailSelector(1)); + const rect2 = await getRect(page, getThumbnailSelector(2)); + + // Drag page 1 to after page 2 (short displacement, stays in + // viewport). + const handlePagesEdited = await waitForPagesEdited(page); + await dragAndDrop( + page, + getThumbnailSelector(1), + [[0, rect2.y - rect1.y + rect2.height / 2]], + 10 + ); + await awaitPromise(handlePagesEdited); + + // After the drop the selection must be cleared: the deselect button + // must be hidden and the label must show the "no selection" state. + // Without the fix, the "2 selected" counter persists. + const deselectHidden = await page.$eval( + "#viewsManagerStatusActionDeselectButton", + el => el.classList.contains("hidden") + ); + expect(deselectHidden) + .withContext(`In ${browserName}: deselect button hidden`) + .toBeTrue(); + + const labelId = await page.$eval( + "#viewsManagerStatusActionLabel", + el => el.getAttribute("data-l10n-id") + ); + expect(labelId) + .withContext(`In ${browserName}: label l10n-id`) + .toBe("pdfjs-views-manager-pages-status-none-action-label"); + }) + ); + }); + }); + + describe("Undo bar must disappear and UI must recover after delete+undo (bug 2022824)", () => { + let pages; + + beforeEach(async () => { + pages = await loadAndWait( + "page_with_number.pdf", + "#viewsManagerToggleButton", + "1", + null, + { enableSplitMerge: true } + ); + }); + + afterEach(async () => { + await closePages(pages); + }); + + it("should hide the undo bar and restore the action bar after undoing a delete", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + await waitForThumbnailVisible(page, 1); + await page.waitForSelector("#viewsManagerStatusActionButton", { + visible: true, + }); + + // Select page 1 and delete it. + await waitAndClick( + page, + `.thumbnail:has(${getThumbnailSelector(1)}) input` + ); + let handlePagesEdited = await waitForPagesEdited(page); + await waitAndClick(page, "#viewsManagerStatusActionButton"); + await waitAndClick(page, "#viewsManagerStatusActionDelete"); + await awaitPromise(handlePagesEdited); + + // Undo bar must be visible with the "Undo" button. + await page.waitForSelector("#viewsManagerStatusUndo", { + visible: true, + }); + + // Click Undo. + handlePagesEdited = await waitForPagesEdited(page, "cancelDelete"); + await waitAndClick(page, "#viewsManagerStatusUndoButton"); + await awaitPromise(handlePagesEdited); + + // Without the fix, the undo bar stays visible and buttons are + // unclickable. With the fix, the undo bar must be gone and the + // normal action bar must be back. + await page.waitForSelector("#viewsManagerStatusUndo", { + hidden: true, + }); + await page.waitForSelector("#viewsManagerStatusAction", { + visible: true, + }); + + // The page count must be back to 17. + const pageCount = await page.$eval("#pageNumber", el => el.max); + expect(parseInt(pageCount, 10)) + .withContext(`In ${browserName}`) + .toBe(17); + + // The action button must be interactable: select a page and delete + // it again to confirm the full state is restored. + await waitAndClick( + page, + `.thumbnail:has(${getThumbnailSelector(1)}) input` + ); + handlePagesEdited = await waitForPagesEdited(page); + await waitAndClick(page, "#viewsManagerStatusActionButton"); + await waitAndClick(page, "#viewsManagerStatusActionDelete"); + await awaitPromise(handlePagesEdited); + + const pageCountAfterSecondDelete = await page.$eval( + "#pageNumber", + el => el.max + ); + expect(parseInt(pageCountAfterSecondDelete, 10)) + .withContext(`In ${browserName} after second delete`) + .toBe(16); + }) + ); + }); + }); }); diff --git a/web/pdf_page_view.js b/web/pdf_page_view.js index 32f234e13..17ba9ad72 100644 --- a/web/pdf_page_view.js +++ b/web/pdf_page_view.js @@ -303,7 +303,7 @@ class PDFPageView extends BasePDFPageView { l10n: this.l10n, clonedFrom: this, }); - clone.setPdfPage(this.pdfPage); + clone.setPdfPage(this.pdfPage.clone(id - 1)); return clone; } diff --git a/web/pdf_thumbnail_view.js b/web/pdf_thumbnail_view.js index c59a7ba93..46d4ad37c 100644 --- a/web/pdf_thumbnail_view.js +++ b/web/pdf_thumbnail_view.js @@ -80,6 +80,8 @@ class TempImageFactory { class PDFThumbnailView extends RenderableView { #renderingState = RenderingStates.INITIAL; + static foo = 0; + /** * @param {PDFThumbnailViewOptions} options */ @@ -97,6 +99,7 @@ class PDFThumbnailView extends RenderableView { enableSplitMerge = false, }) { super(); + this.foo = PDFThumbnailView.foo++; this.id = id; this.renderingId = `thumbnail${id}`; this.pageLabel = null; @@ -166,7 +169,6 @@ class PDFThumbnailView extends RenderableView { pageColors: this.pageColors, enableSplitMerge: !!this.checkbox, }); - thumbnailView.setPdfPage(this.pdfPage); const { imageContainer } = this; if (!imageContainer.classList.contains("missingThumbnailImage")) { thumbnailView.image.replaceWith(this.image.cloneNode(true)); diff --git a/web/pdf_thumbnail_viewer.js b/web/pdf_thumbnail_viewer.js index 3bdf11f8b..c66b246d6 100644 --- a/web/pdf_thumbnail_viewer.js +++ b/web/pdf_thumbnail_viewer.js @@ -157,6 +157,8 @@ class PDFThumbnailViewer { #isInPasteMode = false; + #hasUndoBarVisible = false; + /** * @param {PDFThumbnailViewerOptions} options */ @@ -277,7 +279,7 @@ class PDFThumbnailViewer { this.#undoButton?.addEventListener("click", this.#undo.bind(this)); this.#undoCloseButton?.addEventListener( "click", - this.#dismissUndo.bind(this) + this.#dismissUndo.bind(this, /* mustUpdateStatus = */ true) ); this.#deselectButton?.addEventListener("click", () => { this.#clearSelection(); @@ -693,7 +695,8 @@ class PDFThumbnailViewer { selectedPages.clear(); this.#pageNumberToRemove = NaN; - this.#updateMenuEntries(); + this.#toggleMenuEntries(false); + this.#updateStatus("select"); this.eventBus.dispatch("pagesedited", { source: this, @@ -728,13 +731,13 @@ class PDFThumbnailViewer { } #undo() { + this.#clearSelection(); + this.#toggleMenuEntries(false); + this.#updateStatus("select"); if (this.#copiedThumbnails) { // We undo a copy or a cut. this.#copiedThumbnails = null; this.#pagesMapper.cancelCopy(); - this.#clearSelection(); - this.#toggleMenuEntries(false); - this.#updateStatus("select"); this.#togglePasteMode(false); this.eventBus.dispatch("pagesedited", { @@ -766,17 +769,21 @@ class PDFThumbnailViewer { } } - #dismissUndo() { + #dismissUndo(mustUpdateStatus) { this.#copiedThumbnails = null; if (this.#deletedPageNumbers) { - for (const pageNumber of this.#deletedPageNumbers) { - this.#savedThumbnails[pageNumber - 1].destroy(); + if (this.#savedThumbnails) { + for (const pageNumber of this.#deletedPageNumbers) { + this.#savedThumbnails[pageNumber - 1].destroy(); + } + this.#savedThumbnails = null; } this.#deletedPageNumbers = null; - this.#savedThumbnails = null; } this.#isCut = false; - this.#updateStatus("select"); + if (mustUpdateStatus) { + this.#updateStatus("select"); + } this.#togglePasteMode(false); this.#pagesMapper.cleanSavedData(); @@ -817,6 +824,12 @@ class PDFThumbnailViewer { } #copyPages(clearSelection = true) { + if (!this.#isCut) { + // Entering pure copy mode "commits" any pending paste/delete state so + // that clicking the "Done" button later only cancels the copy and does + // not accidentally restore a previous paste or delete. + this.#savedThumbnails = null; + } this.#updateStatus(this.#isCut ? "cut" : "copy"); const pageNumbersToCopy = (this.#copiedPageNumbers = Uint32Array.from( this.#selectedPages @@ -860,6 +873,7 @@ class PDFThumbnailViewer { pagesMapper.pastePages(index); this.#updateCurrentPage(this.#updateThumbnails(currentPageNumber)); + this.#computeThumbnailsPosition(); this.eventBus.dispatch("pagesedited", { source: this, @@ -944,6 +958,7 @@ class PDFThumbnailViewer { } this.#statusBar.classList.toggle("hidden", false); this.#undoBar.classList.toggle("hidden", true); + this.#hasUndoBarVisible = false; return; } @@ -978,6 +993,7 @@ class PDFThumbnailViewer { this.#statusBar.classList.toggle("hidden", true); this.#undoBar.classList.toggle("hidden", false); + this.#hasUndoBarVisible = true; } #moveDraggedContainer(dx, dy) { @@ -1194,7 +1210,11 @@ class PDFThumbnailViewer { break; case "Delete": case "Backspace": - if (this.#enableSplitMerge && this.#selectedPages?.size) { + if ( + this.#enableSplitMerge && + !this.#isInPasteMode && + this.#selectedPages?.size + ) { this.#deletePages(); stopEvent(e); } @@ -1217,12 +1237,16 @@ class PDFThumbnailViewer { } #selectPage(pageNumber, checked) { + if (this.#hasUndoBarVisible) { + this.#dismissUndo(/* mustUpdateStatus = */ false); + } const set = (this.#selectedPages ??= new Set()); if (checked) { set.add(pageNumber); } else { set.delete(pageNumber); } + this.#updateMenuEntries(); this.#updateStatus("select"); } diff --git a/web/pdf_viewer.js b/web/pdf_viewer.js index 68b568b98..c82121490 100644 --- a/web/pdf_viewer.js +++ b/web/pdf_viewer.js @@ -1202,6 +1202,10 @@ class PDFViewer { } if (type === "cancelDelete") { + this.#deletedPageNumbers = null; + if (!this.#savedPageViews) { + return; + } const viewerElement = this._scrollMode === ScrollMode.PAGE ? null : this.viewer; if (viewerElement) { @@ -1215,16 +1219,19 @@ class PDFViewer { } this._pages = this.#savedPageViews; this.#savedPageViews = null; - this.#deletedPageNumbers = null; return; } if (type === "cleanSavedData") { - for (const pageNum of this.#deletedPageNumbers) { - this.#savedPageViews[pageNum - 1].deleteMe(); + if (this.#deletedPageNumbers) { + if (this.#savedPageViews) { + for (const pageNum of this.#deletedPageNumbers) { + this.#savedPageViews[pageNum - 1].deleteMe(); + } + this.#savedPageViews = null; + } + this.#deletedPageNumbers = null; } - this.#savedPageViews = null; - this.#deletedPageNumbers = null; return; } @@ -1248,7 +1255,7 @@ class PDFViewer { page.updatePageNumber(i); } - if (!isCut) { + if (type === "paste") { this.#copiedPageViews = null; }