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

[api-minor] Respect the drawingDelay also when CSS-only zooming is used (issue 18022) #18077

Merged
merged 1 commit into from
May 16, 2024

Conversation

Snuffleupagus
Copy link
Collaborator

If a user manually calls PDFPageView.prototype.update() with a drawingDelay-option then it'll always be necessary to re-call the method without a delay afterwards, regardless of the maxCanvasPixels-value (e.g. even when CSS-only zooming is used).

…used (issue 18022)

If a user manually calls `PDFPageView.prototype.update()` with a `drawingDelay`-option then it'll always be necessary to re-call the method *without* a delay afterwards, regardless of the `maxCanvasPixels`-value (e.g. even when CSS-only zooming is used).
@Snuffleupagus
Copy link
Collaborator Author

/botio-linux preview

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

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

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

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

Total script time: 1.91 mins

Published

@Snuffleupagus
Copy link
Collaborator Author

/botio integrationtest

@moz-tools-bot
Copy link
Collaborator

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/abd20738a5eb7cf/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/266bf72cb5fe464/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

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

Total script time: 7.13 mins

  • Integration Tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/266bf72cb5fe464/output.txt

Total script time: 19.54 mins

  • Integration Tests: FAILED

@timvandermeij
Copy link
Contributor

timvandermeij commented May 14, 2024

The change looks good to me aside from the comment above, and two questions:

  • I assume it's not really easy/possible to add a test for this particular issue, but it might be good to add a more general test that zooms in to above the CSS zooming threshold and asserts that the rendering still works so we have at least hit the CSS zooming feature in the integration tests. However, that doesn't have to be part of this PR per se since it's not directly related to the issue at hand, so I'll leave that up to you to decide.

  • Should this be tagged as [api-minor] since I thought we mainly used that for the library API and not really for the viewer API (since I don't think we really defined a formal viewer/components API)? It is a possible behavior change for viewer consumers, so I'm not against tagging it, but figured I should verify since I'm not really sure what the scope of that tag is.

@nicolo-ribaudo
Copy link
Contributor

I assume it's not really easy/possible to add a test for this particular issue

I believe it is possible to test this by listening to the pagerendered event while zooming, I can try providing a test in the next few days.

@Snuffleupagus
Copy link
Collaborator Author

Snuffleupagus commented May 14, 2024

The change looks good to me aside from the comment above, and two questions:

@timvandermeij
Copy link
Contributor

I believe it is possible to test this by listening to the pagerendered event while zooming, I can try providing a test in the next few days.

If that's possible that'd be great; thanks! It's not a must-have for landing this PR, but if it's possible to have some (basic) testing for either this issue or CSS zooming in general that would increase confidence in the code.

[...] I figured that [api-minor] might not hurt [...]

I agree. Let's keep this as-is then.

Copy link
Contributor

@timvandermeij timvandermeij left a comment

Choose a reason for hiding this comment

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

The change looks good from my point of view, now that the questions above have been addressed. I'd prefer we wait a few days with merging this until we know if we can include a test for this, but otherwise I'm also not opposed to doing that in a separate PR if that turns out to be more complicated than anticipated.

@nicolo-ribaudo
Copy link
Contributor

@timvandermeij Unfortunately I don't think I'll be able to submit a test until next week.

@timvandermeij timvandermeij merged commit 0603d1a into mozilla:master May 16, 2024
7 checks passed
@timvandermeij
Copy link
Contributor

timvandermeij commented May 16, 2024

@nicolo-ribaudo No worries; we're already happy that you're willing to help out with this, so no rush. Let's land this PR so the issue is resolved, and I have opened follow-up issue #18105 for introducing tests for this functionality (since that scope is a bit broader than just this particular patch).

@Snuffleupagus Thank you for the patch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Text layer constantly re-rendered when zooming large PDF (or large zoom)
4 participants