From aa90f9023d3081c83eadc7389f3412366d81b3ce Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sun, 16 Aug 2026 15:06:52 +0200 Subject: [PATCH 1/4] Remove unnecessary `!lzwState` check in `LZWStream.prototype.readBlock` According to the coverage data this is now dead code; see https://app.codecov.io/gh/mozilla/pdf.js/commit/6694c0eca8c80e3b0aaefa1ad3b76d9317ab2466/blob/src/core/lzw_stream.js?dropdown=coverage#L69 The reason that this check isn't necessary is that the `DecodeStream` class always checks that `!this.eof` holds *before* invoking the `readBlock` method. Looking at the other `DecodeStream` sub-classes they generally rely on this fact, since there's no other `readBlock` implementation with a similar check. Finally, note that `this.lzwState` is only removed in a single case where `this.eof = true;` is also being set; see https://github.com/mozilla/pdf.js/blob/6694c0eca8c80e3b0aaefa1ad3b76d9317ab2466/src/core/lzw_stream.js#L108-L112 --- src/core/lzw_stream.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/core/lzw_stream.js b/src/core/lzw_stream.js index 465cffddfcf30..7b5322358ab93 100644 --- a/src/core/lzw_stream.js +++ b/src/core/lzw_stream.js @@ -65,11 +65,7 @@ class LZWStream extends DecodeStream { let estimatedDecodedSize = blockSize * 2; let i, j, q; - const lzwState = this.lzwState; - if (!lzwState) { - return; // eof was found - } - + const { lzwState } = this; const earlyChange = lzwState.earlyChange; let nextCode = lzwState.nextCode; const dictionaryValues = lzwState.dictionaryValues; From c19ff5f9815f51059007d7f28b9dc16ca92f61a4 Mon Sep 17 00:00:00 2001 From: calixteman Date: Sat, 15 Aug 2026 18:09:22 +0200 Subject: [PATCH 2/4] Fix intermittent reorganize pages integration tests Avoid rendering every page for content assertions and read text through the reordered page-view proxies. Wait for the views manager to finish opening, target a stable drag slot, synchronize current-page checks, and handle cloned annotation storage IDs. --- test/integration/reorganize_pages_spec.mjs | 97 ++++++++++++++-------- test/integration/test_utils.mjs | 27 +++--- 2 files changed, 77 insertions(+), 47 deletions(-) diff --git a/test/integration/reorganize_pages_spec.mjs b/test/integration/reorganize_pages_spec.mjs index 065c9cbb91850..4e91df22f0c36 100644 --- a/test/integration/reorganize_pages_spec.mjs +++ b/test/integration/reorganize_pages_spec.mjs @@ -140,27 +140,33 @@ async function drawInkLine(page, pageNumber) { } async function waitForHavingContents(page, expected) { - await page.evaluate(() => { - // Make sure all the pages will be visible. - window.PDFViewerApplication.pdfViewer.scrollMode = 2; /* = ScrollMode.WRAPPED = */ - window.PDFViewerApplication.pdfViewer.updateScale({ - drawingDelay: 0, - scaleFactor: 0.01, - }); - }); - return page.waitForFunction( - ex => { - const textLayers = document.querySelectorAll(".textLayer"); - const buffer = []; - for (const [i, textLayer] of textLayers.entries()) { - const text = textLayer.textContent.trim(); - buffer.push(typeof ex[i] === "string" ? text : parseInt(text, 10)); + await page.waitForFunction( + length => { + const { pdfViewer } = window.PDFViewerApplication; + for (let i = 0; i < length; i++) { + if (!pdfViewer.getPageView(i)?.pdfPage) { + return false; + } } - return ex.length === buffer.length && ex.every((v, i) => v === buffer[i]); + return true; }, {}, - expected + expected.length ); + const actual = await page.evaluate(async ex => { + const { pdfViewer } = window.PDFViewerApplication; + const contents = []; + for (let i = 0, ii = ex.length; i < ii; i++) { + const { items } = await pdfViewer.getPageView(i).pdfPage.getTextContent(); + const text = items + .map(item => item.str ?? "") + .join("") + .trim(); + contents.push(typeof ex[i] === "string" ? text : parseInt(text, 10)); + } + return contents; + }, expected); + expect(actual).toEqual(expected); } async function waitForPageCanvasToHaveImage(page, pageNumber) { @@ -2470,13 +2476,17 @@ describe("Reorganize Pages View", () => { await waitForThumbnailVisible(page, 1); const rect1 = await getRect(page, getThumbnailSelector(1)); const rect2 = await getRect(page, getThumbnailSelector(2)); + // Stay clear of the bottom edge, where dragging can auto-scroll and + // move the page one position too far. + const yTranslation = + rect2.y + rect2.height / 2 - (rect1.y + rect1.height / 2) + 1; // 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]], + [[0, yTranslation]], 10 ); let pageIndices = await awaitPromise(handlePagesEdited); @@ -2897,27 +2907,40 @@ describe("Reorganize Pages View", () => { // Both the original and the cloned annotation must now be in storage. await waitForStorageEntries(page, 2); - const editorIds = await page.evaluate(() => { + // When its layer is rendered, a clone replaces its serialized storage + // entry with a real editor having a new id. The original id doesn't + // change, hence use it to tell the two entries apart. + const originalEditorId = await page.evaluate(() => { const entries = Array.from( window.PDFViewerApplication.pdfDocument.annotationStorage ); - return { - original: entries.find(([, editor]) => editor.pageIndex === 0)[0], - clone: entries.find(([, editor]) => editor.pageIndex === 2)[0], - }; + return entries.find(([, editor]) => editor.pageIndex === 0)[0]; }); // Move the pasted copy before the original and verify that both // stored page indices follow the new page order. await movePages(page, [3], 0); - const editorPageIndices = await page.evaluate(ids => { - const storage = - window.PDFViewerApplication.pdfDocument.annotationStorage; - return { - original: storage.getRawValue(ids.original).pageIndex, - clone: storage.getRawValue(ids.clone).pageIndex, - }; - }, editorIds); + const editorPageIndicesHandle = await page.waitForFunction( + id => { + const storage = + window.PDFViewerApplication.pdfDocument.annotationStorage; + const original = storage.getRawValue(id); + const clone = Array.from(storage).find( + ([editorId]) => editorId !== id + )?.[1]; + if (!original || !clone) { + return false; + } + return { + original: original.pageIndex, + clone: clone.pageIndex, + }; + }, + {}, + originalEditorId + ); + const editorPageIndices = await editorPageIndicesHandle.jsonValue(); + await editorPageIndicesHandle.dispose(); expect(editorPageIndices) .withContext(`In ${browserName}`) .toEqual({ original: 1, clone: 0 }); @@ -3117,12 +3140,12 @@ describe("Reorganize Pages View", () => { visible: true, }); + const currentThumbnailSelector = + '.thumbnailImageContainer[aria-current="page"]'; const countCurrentThumbnails = () => - page.evaluate( - () => - document.querySelectorAll( - '.thumbnailImageContainer[aria-current="page"]' - ).length + page.$$eval( + currentThumbnailSelector, + thumbnails => thumbnails.length ); // Copy page 1 and paste it after page 3. @@ -3151,6 +3174,7 @@ describe("Reorganize Pages View", () => { await waitAndClick(page, "#viewsManagerStatusActionCut"); await awaitPromise(handlePagesEdited); + await page.waitForSelector(currentThumbnailSelector); expect(await countCurrentThumbnails()) .withContext(`In ${browserName}, after cut #${i + 1}`) .toBe(1); @@ -3162,6 +3186,7 @@ describe("Reorganize Pages View", () => { await waitAndClick(page, "#viewsManagerStatusUndoButton"); await awaitPromise(handlePagesEdited); + await page.waitForSelector(currentThumbnailSelector); expect(await countCurrentThumbnails()) .withContext(`In ${browserName}, after undo #${i + 1}`) .toBe(1); diff --git a/test/integration/test_utils.mjs b/test/integration/test_utils.mjs index 2ba8a7e5a62b6..5999c202d2ef8 100644 --- a/test/integration/test_utils.mjs +++ b/test/integration/test_utils.mjs @@ -1133,18 +1133,23 @@ async function highlightSpan( } async function showViewsManager(page) { - const hasAnimations = await page.evaluate( - () => !window.matchMedia("(prefers-reduced-motion: reduce)").matches - ); - const movingPromise = hasAnimations - ? page.waitForSelector("#outerContainer.viewsManagerMoving", { - visible: true, - }) - : Promise.resolve(); + // Opening dispatches this event synchronously when animations are disabled, + // so install the listener before clicking the toggle button. With animations, + // it's dispatched once the transition has ended and the moving class has + // been removed. + const openedHandle = await createPromise(page, resolve => { + const { eventBus, viewsManager } = window.PDFViewerApplication; + const onResize = ({ source }) => { + if (source !== viewsManager) { + return; + } + eventBus.off("resize", onResize); + resolve(); + }; + eventBus.on("resize", onResize); + }); await page.click("#viewsManagerToggleButton"); - if (hasAnimations) { - await movingPromise; - } + await awaitPromise(openedHandle); await page.waitForSelector("#viewsManager", { visible: true }); await page.waitForSelector( "#outerContainer:not(.viewsManagerMoving).viewsManagerOpen", From ca39f5a85f2c449253cf686ef4731d26fc64897c Mon Sep 17 00:00:00 2001 From: calixteman Date: Sun, 16 Aug 2026 19:13:13 +0200 Subject: [PATCH 3/4] Fix the RTL layout of the views manager header (bug 2060033) The chevron and the header padding used physical properties, so they didn't mirror in RTL: the chevron ended up flush against the pages icon and the selector and trailing buttons had their insets swapped. The menu check mark is a "V" shaped glyph, hence it must be mirrored too, as it's already done for the check mark of the signature properties. --- web/menu.css | 2 +- web/views_manager.css | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/web/menu.css b/web/menu.css index 3d225ed5c3f65..6ad2dc06a4c9c 100644 --- a/web/menu.css +++ b/web/menu.css @@ -159,7 +159,7 @@ button.hasPopupMenu { position: absolute; inset-inline-start: var(--menuitem-gap); top: 50%; - transform: translateY(-50%); + transform: translateY(-50%) scaleX(var(--dir-factor)); } &:disabled { diff --git a/web/views_manager.css b/web/views_manager.css index 1ad2c2bf14bc8..37c15b9868e66 100644 --- a/web/views_manager.css +++ b/web/views_manager.css @@ -334,7 +334,8 @@ align-self: stretch; justify-content: space-between; width: auto; - padding: 12px 16px 12px 8px; + padding-block: 12px; + padding-inline: 8px 16px; &:not(:has(#viewsManagerHeaderLabel ~ button:not([hidden])))::after { /* If one of the following buttons is visible, hide the placeholder @@ -372,7 +373,7 @@ display: inline-block; width: 12px; height: 12px; - margin-left: 8px; + margin-inline-start: 8px; mask-repeat: no-repeat; mask-position: center; mask-image: var(--views-manager-button-arrow-icon); From 13b68c0bdca95dce1be9d8de656a54dc1f4c3d50 Mon Sep 17 00:00:00 2001 From: calixteman Date: Sun, 16 Aug 2026 19:39:52 +0200 Subject: [PATCH 4/4] Set the direction of the floating toolbar in RTL locales (bug 2060032) The toolbar is appended to the text layer, which is always LTR (see `.pdfViewer .page`), hence the `inset-inline-end` used to position it always resolved to `right` while in RTL the anchor point is the left edge of the selection: the toolbar ended up on the other side of the page. The annotation editor layer already sets its own direction, so just do the same here. --- src/display/editor/toolbar.js | 4 +++ test/integration/highlight_editor_spec.mjs | 42 ++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/src/display/editor/toolbar.js b/src/display/editor/toolbar.js index 563ad965e01e2..8196089a6fb14 100644 --- a/src/display/editor/toolbar.js +++ b/src/display/editor/toolbar.js @@ -277,6 +277,10 @@ class FloatingToolbar { const editToolbar = (this.#toolbar = document.createElement("div")); editToolbar.className = "editToolbar"; editToolbar.setAttribute("role", "toolbar"); + // The toolbar is inserted in the text layer which is always LTR (see + // `.pdfViewer .page`), hence the direction must be set here in order to + // have `inset-inline-end` (see `show`) resolved against the UI direction. + editToolbar.dir = this.#uiManager.direction; const signal = this.#uiManager._signal; if (signal instanceof AbortSignal && !signal.aborted) { diff --git a/test/integration/highlight_editor_spec.mjs b/test/integration/highlight_editor_spec.mjs index a729cee12306f..f13ad2d8ced8e 100644 --- a/test/integration/highlight_editor_spec.mjs +++ b/test/integration/highlight_editor_spec.mjs @@ -1437,6 +1437,48 @@ describe("Highlight Editor", () => { }); }); + describe("Floating highlight button in a RTL locale", () => { + let pages; + + beforeEach(async () => { + pages = await loadAndWait( + "tracemonkey.pdf", + ".annotationEditorLayer", + null, + null, + { locale: "ar" } + ); + }); + + afterEach(async () => { + await closePages(pages); + }); + + it("must check that the floating toolbar is next to the selected text", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + const { x, y, width, height } = await getSpanRectFromText( + page, + 1, + "Abstract" + ); + await page.mouse.click(x + width / 2, y + height / 2, { + count: 2, + delay: 100, + }); + + const toolbarRect = await getRect(page, ".textLayer .editToolbar"); + + // In RTL, the left edge of the toolbar is aligned on the left edge + // of the selection (bug 2060032). + expect(toolbarRect.x) + .withContext(`In ${browserName}`) + .toBeCloseTo(x, 0); + }) + ); + }); + }); + describe("Text layer must have the focus before highlights", () => { let pages;