From 12692d2e393bff3946131cc07fb89786ff59cd15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Ribaudo?= Date: Fri, 7 Jun 2024 13:44:22 +0200 Subject: [PATCH 1/2] Add test for CSS-only zoom above maxCanvasPixels This commit introduces a test to ensure that: - When zooming below the maxCanvasPixels limit, the canvas is rendered with the correct size (the two sides are multiplied by the zoom factor). - When zooming above the maxCanvasPixels limit, the canvas size is capped. --- test/integration/test_utils.mjs | 7 +- test/integration/viewer_spec.mjs | 219 +++++++++++++++++++++++-------- 2 files changed, 173 insertions(+), 53 deletions(-) diff --git a/test/integration/test_utils.mjs b/test/integration/test_utils.mjs index 2113c4ed224b1..a4f34bd0fec55 100644 --- a/test/integration/test_utils.mjs +++ b/test/integration/test_utils.mjs @@ -38,8 +38,13 @@ function loadAndWait(filename, selector, zoom, pageSetup, options) { let app_options = ""; if (options) { + const optionsObject = + typeof options === "function" + ? await options(page, session.name) + : options; + // Options must be handled in app.js::_parseHashParams. - for (const [key, value] of Object.entries(options)) { + for (const [key, value] of Object.entries(optionsObject)) { app_options += `&${key}=${encodeURIComponent(value)}`; } } diff --git a/test/integration/viewer_spec.mjs b/test/integration/viewer_spec.mjs index d2f2f847fb215..31ba9e1951cbe 100644 --- a/test/integration/viewer_spec.mjs +++ b/test/integration/viewer_spec.mjs @@ -94,24 +94,6 @@ describe("PDF viewer", () => { }); describe("CSS-only zoom", () => { - let pages; - - beforeAll(async () => { - pages = await loadAndWait( - "tracemonkey.pdf", - ".textLayer .endOfContent", - null, - null, - { - maxCanvasPixels: 0, - } - ); - }); - - afterAll(async () => { - await closePages(pages); - }); - function createPromiseForFirstPageRendered(page) { return createPromise(page, (resolve, reject) => { const controller = new AbortController(); @@ -129,50 +111,183 @@ describe("PDF viewer", () => { }); } - it("respects drawing delay when zooming out", async () => { - await Promise.all( - pages.map(async ([browserName, page]) => { - const promise = await createPromiseForFirstPageRendered(page); + describe("forced (maxCanvasPixels: 0)", () => { + let pages; + + beforeAll(async () => { + pages = await loadAndWait( + "tracemonkey.pdf", + ".textLayer .endOfContent", + null, + null, + { maxCanvasPixels: 0 } + ); + }); + + afterAll(async () => { + await closePages(pages); + }); - const start = await page.evaluate(() => { - const startTime = performance.now(); - window.PDFViewerApplication.pdfViewer.decreaseScale({ - drawingDelay: 100, - scaleFactor: 0.9, + it("respects drawing delay when zooming out", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + const promise = await createPromiseForFirstPageRendered(page); + + const start = await page.evaluate(() => { + const startTime = performance.now(); + window.PDFViewerApplication.pdfViewer.decreaseScale({ + drawingDelay: 100, + scaleFactor: 0.9, + }); + return startTime; }); - return startTime; - }); - const end = await awaitPromise(promise); + const end = await awaitPromise(promise); - expect(end - start) - .withContext(`In ${browserName}`) - .toBeGreaterThanOrEqual(100); - }) - ); + expect(end - start) + .withContext(`In ${browserName}`) + .toBeGreaterThanOrEqual(100); + }) + ); + }); + + it("respects drawing delay when zooming in", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + const promise = await createPromiseForFirstPageRendered(page); + + const start = await page.evaluate(() => { + const startTime = performance.now(); + window.PDFViewerApplication.pdfViewer.increaseScale({ + drawingDelay: 100, + scaleFactor: 1.1, + }); + return startTime; + }); + + const end = await awaitPromise(promise); + + expect(end - start) + .withContext(`In ${browserName}`) + .toBeGreaterThanOrEqual(100); + }) + ); + }); }); - it("respects drawing delay when zooming in", async () => { - await Promise.all( - pages.map(async ([browserName, page]) => { - const promise = await createPromiseForFirstPageRendered(page); + describe("triggers when going bigger than maxCanvasPixels", () => { + let pages; - const start = await page.evaluate(() => { - const startTime = performance.now(); - window.PDFViewerApplication.pdfViewer.increaseScale({ - drawingDelay: 100, - scaleFactor: 1.1, + const MAX_CANVAS_PIXELS = new Map(); + + beforeAll(async () => { + pages = await loadAndWait( + "tracemonkey.pdf", + ".textLayer .endOfContent", + null, + null, + async (page, browserName) => { + const ratio = await page.evaluate(() => window.devicePixelRatio); + const maxCanvasPixels = 1_000_000 * ratio ** 2; + MAX_CANVAS_PIXELS.set(browserName, maxCanvasPixels); + + return { maxCanvasPixels }; + } + ); + }); + + beforeEach(async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + await page.evaluate(() => { + window.PDFViewerApplication.pdfViewer.currentScale = 0.5; }); - return startTime; - }); + }) + ); + }); - const end = await awaitPromise(promise); + afterAll(async () => { + await closePages(pages); + }); - expect(end - start) - .withContext(`In ${browserName}`) - .toBeGreaterThanOrEqual(100); - }) - ); + function getCanvasSize(page) { + return page.evaluate(() => { + const canvas = window.document.querySelector(".canvasWrapper canvas"); + return canvas.width * canvas.height; + }); + } + + // MAX_CANVAS_PIXELS must be big enough that the originally rendered + // canvas still has enough space to grow before triggering CSS-only zoom + it("test correctly configured", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + const canvasSize = await getCanvasSize(page); + + expect(canvasSize) + .withContext(`In ${browserName}`) + .toBeLessThan(MAX_CANVAS_PIXELS.get(browserName) / 4); + expect(canvasSize) + .withContext(`In ${browserName}`) + .toBeGreaterThan(MAX_CANVAS_PIXELS.get(browserName) / 16); + }) + ); + }); + + it("does not trigger CSS-only zoom below maxCanvasPixels", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + const originalCanvasSize = await getCanvasSize(page); + const factor = 2; + + await page.evaluate(scaleFactor => { + window.PDFViewerApplication.pdfViewer.increaseScale({ + drawingDelay: 0, + scaleFactor, + }); + }, factor); + + const canvasSize = await getCanvasSize(page); + + expect(canvasSize) + .withContext(`In ${browserName}`) + .toBe(originalCanvasSize * factor ** 2); + + expect(canvasSize) + .withContext(`In ${browserName}, MAX_CANVAS_PIXELS`) + .toBeLessThan(MAX_CANVAS_PIXELS.get(browserName)); + }) + ); + }); + + it("triggers CSS-only zoom above maxCanvasPixels", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + const originalCanvasSize = await getCanvasSize(page); + const factor = 4; + + await page.evaluate(scaleFactor => { + window.PDFViewerApplication.pdfViewer.increaseScale({ + drawingDelay: 0, + scaleFactor, + }); + }, factor); + + const canvasSize = await getCanvasSize(page); + + expect(canvasSize) + .withContext(`In ${browserName}`) + .toBeLessThan(originalCanvasSize * factor ** 2); + + // Disabled because `canvasSize` is `4_012_800`, which is + // close to the limit but somehow a bit more. + // + // expect(canvasSize) + // .withContext(`In ${browserName}, MAX_CANVAS_PIXELS`) + // .toBeLessThan(MAX_CANVAS_PIXELS.get(browserName)); + }) + ); + }); }); }); }); From de23bb9b82b5d63cd591caeddbc96ed13e4827ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Ribaudo?= Date: Fri, 7 Jun 2024 14:08:43 +0200 Subject: [PATCH 2/2] Respect `maxCanvasPixels` when computing canvas dimensions Ensure that we never round the canvas dimensions above `maxCanvasPixels` by rounding them to the preceeding multiple of the display ratio rather than the succeeding one. --- test/integration/viewer_spec.mjs | 13 +++++++------ web/pdf_page_view.js | 10 +++++----- web/ui_utils.js | 12 ++++++++---- 3 files changed, 20 insertions(+), 15 deletions(-) diff --git a/test/integration/viewer_spec.mjs b/test/integration/viewer_spec.mjs index 31ba9e1951cbe..f8e7fda365d09 100644 --- a/test/integration/viewer_spec.mjs +++ b/test/integration/viewer_spec.mjs @@ -279,12 +279,13 @@ describe("PDF viewer", () => { .withContext(`In ${browserName}`) .toBeLessThan(originalCanvasSize * factor ** 2); - // Disabled because `canvasSize` is `4_012_800`, which is - // close to the limit but somehow a bit more. - // - // expect(canvasSize) - // .withContext(`In ${browserName}, MAX_CANVAS_PIXELS`) - // .toBeLessThan(MAX_CANVAS_PIXELS.get(browserName)); + expect(canvasSize) + .withContext(`In ${browserName}, <= MAX_CANVAS_PIXELS`) + .toBeLessThanOrEqual(MAX_CANVAS_PIXELS.get(browserName)); + + expect(canvasSize) + .withContext(`In ${browserName}, > MAX_CANVAS_PIXELS * 0.99`) + .toBeGreaterThan(MAX_CANVAS_PIXELS.get(browserName) * 0.99); }) ); }); diff --git a/web/pdf_page_view.js b/web/pdf_page_view.js index 915f566f97813..fde5d7dc5f6fd 100644 --- a/web/pdf_page_view.js +++ b/web/pdf_page_view.js @@ -34,9 +34,9 @@ import { import { approximateFraction, DEFAULT_SCALE, + floorToDivide, OutputScale, RenderingStates, - roundToDivide, TextLayerMode, } from "./ui_utils.js"; import { AnnotationEditorLayerBuilder } from "./annotation_editor_layer_builder.js"; @@ -1025,11 +1025,11 @@ class PDFPageView { const sfx = approximateFraction(outputScale.sx); const sfy = approximateFraction(outputScale.sy); - canvas.width = roundToDivide(width * outputScale.sx, sfx[0]); - canvas.height = roundToDivide(height * outputScale.sy, sfy[0]); + canvas.width = floorToDivide(width * outputScale.sx, sfx[0]); + canvas.height = floorToDivide(height * outputScale.sy, sfy[0]); const { style } = canvas; - style.width = roundToDivide(width, sfx[1]) + "px"; - style.height = roundToDivide(height, sfy[1]) + "px"; + style.width = floorToDivide(width, sfx[1]) + "px"; + style.height = floorToDivide(height, sfy[1]) + "px"; // Add the viewport so it's known what it was originally drawn with. this.#viewportMap.set(canvas, viewport); diff --git a/web/ui_utils.js b/web/ui_utils.js index a6c0bc7f15fab..f2a30d3613bf2 100644 --- a/web/ui_utils.js +++ b/web/ui_utils.js @@ -260,6 +260,7 @@ function binarySearchFirstItem(items, condition, start = 0) { * @param {number} x - Positive float number. * @returns {Array} Estimated fraction: the first array item is a numerator, * the second one is a denominator. + * They are both natural numbers. */ function approximateFraction(x) { // Fast paths for int numbers or their inversions. @@ -306,9 +307,12 @@ function approximateFraction(x) { return result; } -function roundToDivide(x, div) { - const r = x % div; - return r === 0 ? x : Math.round(x - r + div); +/** + * @param {number} x - A positive number to round to a multiple of `div`. + * @param {number} div - A natural number. + */ +function floorToDivide(x, div) { + return x - (x % div); } /** @@ -863,6 +867,7 @@ export { DEFAULT_SCALE_DELTA, DEFAULT_SCALE_VALUE, docStyle, + floorToDivide, getActiveOrFocusedElement, getPageSizeInches, getVisibleElements, @@ -881,7 +886,6 @@ export { ProgressBar, removeNullCharacters, RenderingStates, - roundToDivide, SCROLLBAR_PADDING, scrollIntoView, ScrollMode,