Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Respect maxCanvasPixels when computing canvas dimensions #18218

Merged
merged 2 commits into from
Jun 19, 2024
Merged

Respect maxCanvasPixels when computing canvas dimensions #18218

merged 2 commits into from
Jun 19, 2024

Conversation

nicolo-ribaudo
Copy link
Contributor

Closes #18105

Review this PR disabling whitespace changes, since one of the existing tests is indented and it makes the diff quite confusing.

@nicolo-ribaudo nicolo-ribaudo changed the title Add test for CSS-only zoom above maxCanvasPixels Respect maxCanvasPixels when computing camvas dimensions Jun 7, 2024
@nicolo-ribaudo nicolo-ribaudo force-pushed the test-maxCanvasPixels branch 2 times, most recently from 9d4881e to a4e865c Compare June 7, 2024 14:27
@marco-c marco-c changed the title Respect maxCanvasPixels when computing camvas dimensions Respect maxCanvasPixels when computing canvas dimensions Jun 7, 2024
@timvandermeij
Copy link
Contributor

/botio-linux preview

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/504be7417e49d80/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/504be7417e49d80/output.txt

Total script time: 1.08 mins

Published

@timvandermeij
Copy link
Contributor

/botio integrationtest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/44aeb66a25858b3/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.193.163.58:8877/dcdf1f861b1fcba/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/44aeb66a25858b3/output.txt

Total script time: 7.77 mins

  • Integration Tests: FAILED

@nicolo-ribaudo
Copy link
Contributor Author

The "PDF viewer CSS-only zoom triggers when going bigger than maxCanvasPixels test correctly configured" failure means that the numbers I hard-coded don't work for these tests on the CI machine.

I picked them based on how big the Firefox/Chrome windows were on my machine when running the tests. Do you know:

  • how big the viewport is in the bot
  • if I can hard-coded a viewport size?

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/dcdf1f861b1fcba/output.txt

Total script time: 19.96 mins

  • Integration Tests: FAILED

@timvandermeij
Copy link
Contributor

I'm not sure how big the viewport is on the bots, but in general I think we should probably set a fixed viewport size for the integration tests to eliminate any possible differences between configurations (since we also want everything to work correctly locally where window sizes might differ). Fortunately Puppeteer already seems to support this in the launch() call, refer to https://pptr.dev/api/puppeteer.browserconnectoptions#properties, and this should not cause other tests to fail (and if they do, we found an implicit dependency on the viewport size which we should then fix).

@timvandermeij
Copy link
Contributor

timvandermeij commented Jun 9, 2024 •
edited
Loading

The default viewport property for the launch() call turns out to already be set at

pdf.js/test/test.mjs

Line 888 in bb73d2a

defaultViewport: null,
but it's set to null instead of a fixed value. That dates back to my initial commit that introduced Puppeteer, but I don't really know anymore why I apparently explicitly set it to that 😅

I would say that we can pick a standard value (maybe 1024x768 or 1280x1024) here which should be fine for any modern configuration.

@Snuffleupagus
Copy link
Collaborator

That dates back to my initial commit that introduced Puppeteer, but I don't really know anymore why I apparently explicitly set it to that 😅

That's done to address #11807 (review), so please don't change that unconditionally since it'd make working with browsertest results more "annoying" locally.

I would say that we can pick a standard value (maybe 1024x768 or 1280x1024) here which should be fine for any modern configuration.

If so, please limit that to only the integration tests.

@timvandermeij
Copy link
Contributor

timvandermeij commented Jun 9, 2024 •
edited
Loading

Good point. Alternatively, instead of setting a fixed viewport, it might also be possible to dynamically determine the values in this PR based on the current viewport size (perhaps a certain fraction/percentage of it) which can be fetched using e.g. the approach from https://stackoverflow.com/questions/1248081/how-to-get-the-browser-viewport-dimensions? That avoids changing the existing viewport behavior while also having a way to deal with different viewport sizes, and it avoids having to change the test if we would later decide to increase the fixed viewport size if we went with that approach.

Snuffleupagus
Snuffleupagus reviewed Jun 18, 2024
View reviewed changes
web/ui_utils.js Outdated Show resolved Hide resolved
@nicolo-ribaudo
Copy link
Contributor Author

It turns out that my test was not depending on the viewport dimensions :) The PDF size is guaranteed to be consistent, given that it's always reset to "50%" in beforeEach. However, given a fixed PDF size, the canvas size depends on the pixel ratio.

I changed the options to pass a maxCanvasPixels based on the pixel ratio, which solves this problem.

@timvandermeij
Copy link
Contributor

/botio integrationtest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/971890288bbb4c2/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.193.163.58:8877/91894c56e5e5e04/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/971890288bbb4c2/output.txt

Total script time: 7.86 mins

  • Integration Tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/91894c56e5e5e04/output.txt

Total script time: 18.51 mins

  • Integration Tests: Passed

@nicolo-ribaudo
Add test for CSS-only zoom above maxCanvasPixels
12692d2 
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.
@nicolo-ribaudo
Respect maxCanvasPixels when computing canvas dimensions
de23bb9 
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.
@timvandermeij
Copy link
Contributor

/botio-linux preview

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/76d3b7a02ce2390/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/76d3b7a02ce2390/output.txt

Total script time: 1.04 mins

Published

@timvandermeij
Copy link
Contributor

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/a829ef111d362fb/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.193.163.58:8877/b65656c11c97246/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/a829ef111d362fb/output.txt

Total script time: 28.74 mins

  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 16

Image differences available at: http://54.241.84.105:8877/a829ef111d362fb/reftest-analyzer.html#web=eq.log

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/b65656c11c97246/output.txt

Total script time: 44.07 mins

  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 5

Image differences available at: http://54.193.163.58:8877/b65656c11c97246/reftest-analyzer.html#web=eq.log

@timvandermeij timvandermeij merged commit 3e1d779 into mozilla:master Jun 19, 2024
7 checks passed
@timvandermeij
Copy link
Contributor

Thank you for improving this!

@nicolo-ribaudo nicolo-ribaudo deleted the test-maxCanvasPixels branch June 19, 2024 14:47
@ZeroXClem ZeroXClem mentioned this pull request Aug 12, 2024
Closed
@Snuffleupagus Snuffleupagus mentioned this pull request Sep 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Snuffleupagus Snuffleupagus Snuffleupagus left review comments

@calixteman calixteman calixteman left review comments

@timvandermeij timvandermeij timvandermeij approved these changes

Assignees
No one assigned
Labels
test viewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Introduce tests for CSS-only zooming
5 participants
@nicolo-ribaudo @timvandermeij @moz-tools-bot @Snuffleupagus @calixteman