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.

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

Fix various zooming regressions (PR 15812 follow-up) #16227

Merged
merged 2 commits into from
Mar 29, 2023

Conversation

Snuffleupagus
Copy link
Collaborator

  • Fix rotation of the zoomLayer (PR 15812 follow-up)

    Currently the zoomLayer isn't rotated correctly in all cases. To reproduce:

    The easiest solution, as far as I can tell, is that we always set the transform just as we did (for years) prior to the changes in PR Only redraw after zooming is finished (bug 1661253) #15812.

  • Fix CSS-only zooming in the viewer (PR 15812 follow-up)

    Currently if you e.g. enable the useOnlyCssZoom option rendering may no longer finish as intended. To reproduce:

    In this case the document will never finish rendering, since the postponeDrawing-functionality will (here incorrectly) abort rendering and with CSS-only zooming rendering is only expected to happen once per page.
    To fix this we'll simply ignore any drawingDelay when CSS-only zooming is used (regardless if it's triggered via the option or the zoom-level being very large).

Currently the `zoomLayer` isn't rotated correctly in all cases. To reproduce:
 - Load https://github.com/mozilla/pdf.js/files/1522715/wuppertal_2012.pdf
 - Let the document render.
 - Rotate the document *four* times, such that the original rotation is restored.

The easiest solution, as far as I can tell, is that we always set the `transform` just as we did (for years) prior to the changes in PR 15812.
Currently if you e.g. enable the `useOnlyCssZoom` option rendering may no longer finish as intended. To reproduce:
 - Enable the `useOnlyCssZoom` option.
 - Load https://github.com/mozilla/pdf.js/files/1522715/wuppertal_2012.pdf (in the development viewer).
 - When rendering starts, *immediately* change the zoom-level.

In this case the document will never finish rendering, since the `postponeDrawing`-functionality will (here incorrectly) abort rendering and with CSS-only zooming rendering is only expected to happen once per page.
To fix this we'll simply ignore any `drawingDelay` when CSS-only zooming is used (regardless if it's triggered via the option or the zoom-level being very large).
@Snuffleupagus Snuffleupagus added viewer regression release-blocker Blocker for the upcoming release labels Mar 29, 2023
@Snuffleupagus Snuffleupagus changed the title Zooming regressions Fix various zooming regressions (PR 15812 follow-up) Mar 29, 2023
@Snuffleupagus
Copy link
Collaborator Author

/botio integrationtest

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.241.84.105:8877/0d8fabed06ef935/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

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

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/0d8fabed06ef935/output.txt

Total script time: 4.38 mins

  • Integration Tests: Passed

Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you.

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

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

Total script time: 17.00 mins

  • Integration Tests: FAILED

@Snuffleupagus Snuffleupagus merged commit 4391779 into mozilla:master Mar 29, 2023
@Snuffleupagus Snuffleupagus deleted the zooming-regressions branch March 29, 2023 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression release-blocker Blocker for the upcoming release viewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants