[api-minor] Simplify API to implement zoom in custom viewers #18179

nicolo-ribaudo · 2024-05-28T10:18:35Z

This PR extends the zoom/scale API to simplify the logic in custom viewer wrappers:

the first commit provides an updateScale function that does both increaseScale/decreaseScale. Ad you can see from the commit itself, it significantly reduces code duplication both in embedders (see the changes in web/app.js) and in web/pdf_viewer.js itself
the second commits moves the "preserve zoom origin when zooming" logic from the app to the viewer, since embedders that want to implement pinch-to-zoom or zoom-around-cursor would need to also implement it (and figuring out exactly what the logic needs to be can be challenging). Part of the "scroll after zoom" logic was already in the viewer, now all of it is roughly in the same place.

Note that this PR increases the net number of lines because it adds a test, but ignoring the new test it would be +56 -95.

@Snuffleupagus In #18098 (comment) you said that you probably do not want updateScale, but I still uploaded it since it de-duplicates a significant amount of code. What do you think about it? (the second commit could be done independently from the first one)

Commits:

Unify increaseScale/decreaseScale logic as updateScale

updateScale receives a drawingDelay, a scaleFactor and/or a number of steps.
If scaleFactor is a positive number different from 1 the current scale is multiplied by
that number. Otherwise, if steps if a positive integer the current scale is multiplied by
DEFAULT_SCALE_DELTA steps times. Finally, if steps is a negative integer, the
current scale is divided by DEFAULT_SCALE_DELTA abs(steps) times.

Add origin parameter to updateScale

This parameter allows defining which point should remain
fixed while scaling the document. It can be used, for example,
to implement "zoom around the cursor" or "zoom around
pinch center".

The logic was previously implemented in web/app.js, but
moving it to the viewer scaling utilities themselves makes it
easier to implement similar zooming functionalities in
other embedders.

test/integration/viewer_spec.mjs

web/pdf_viewer.js

Snuffleupagus · 2024-05-28T11:24:22Z

In #18098 (comment) you said that you probably do not want updateScale, but I still uploaded it since it de-duplicates a significant amount of code. What do you think about it?

Looking at a working patch, written by an actual human, I'm happy to retract that statement since it's clear that it avoids a bunch of code duplication :-)

Snuffleupagus · 2024-05-28T11:25:49Z

/botio integrationtest

moz-tools-bot · 2024-05-28T11:25:52Z

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

moz-tools-bot · 2024-05-28T11:25:52Z

From: Bot.io (Windows)

Received

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

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

moz-tools-bot · 2024-05-28T11:33:43Z

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/41d5c1423b09441/output.txt

Total script time: 7.85 mins

Integration Tests: Passed

moz-tools-bot · 2024-05-28T11:42:29Z

From: Bot.io (Windows)

Failed

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

Total script time: 16.62 mins

Integration Tests: FAILED

nicolo-ribaudo · 2024-05-28T11:53:20Z

This is the test failing on windows (line 727 is throwing):

pdf.js/test/integration/stamp_editor_spec.mjs

Lines 713 to 730 in 95a7de9

    
           it("must check that a resized stamp has its canvas at the right position", async () => { 
        
             await Promise.all( 
        
               pages.map(async ([browserName, page]) => { 
        
                 await switchToStamp(page); 
        
                 await copyImage(page, "../images/firefox_logo.png", 0); 
        
                 await page.waitForSelector(getEditorSelector(0)); 
        
                 await waitForSerialized(page, 1); 
        
                 const serializedRect = await getFirstSerialized(page, x => x.rect); 
        
                 const rect = await getRect(page, ".resizer.bottomRight"); 
        
                 const centerX = rect.x + rect.width / 2; 
        
                 const centerY = rect.y + rect.height / 2; 
        
                 await page.mouse.move(centerX, centerY); 
        
                 await page.mouse.down(); 
        
                 await page.mouse.move(centerX - 500, centerY - 500); 
        
                 await page.mouse.up();

It is failing because the .move() on line 727 is moving to outside of the viewport. It seems unrelated, because that test doesn't change zoom in any way, however looking at recent CI bot I couldn't find any similar failures 🤔

web/pdf_viewer.js

`updateScale` receives a `drawingDelay`, a `scaleFactor` and/or a number of `steps`. If `scaleFactor` is a positive number different from `1` the current scale is multiplied by that number. Otherwise, if `steps` if a positive integer the current scale is multiplied by `DEFAULT_SCALE_DELTA` `steps` times. Finally, if `steps` is a negative integer, the current scale is divided by `DEFAULT_SCALE_DELTA` `abs(steps)` times.

Snuffleupagus · 2024-05-28T12:58:02Z

/botio integrationtest

moz-tools-bot · 2024-05-28T12:58:05Z

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

moz-tools-bot · 2024-05-28T12:58:05Z

From: Bot.io (Windows)

Received

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

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

moz-tools-bot · 2024-05-28T13:05:55Z

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/1a80676073ff22b/output.txt

Total script time: 7.82 mins

Integration Tests: Passed

moz-tools-bot · 2024-05-28T13:17:08Z

From: Bot.io (Windows)

Success

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

Total script time: 19.04 mins

Integration Tests: Passed

Snuffleupagus · 2024-05-28T13:17:39Z

/botio-linux preview

moz-tools-bot · 2024-05-28T13:17:42Z

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

moz-tools-bot · 2024-05-28T13:18:53Z

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/55892e089776b04/output.txt

Total script time: 1.17 mins

Published

Viewer: http://54.241.84.105:8877/55892e089776b04/web/viewer.html
Viewer (legacy): http://54.241.84.105:8877/55892e089776b04/legacy/web/viewer.html

Snuffleupagus · 2024-05-28T13:20:51Z

/botio integrationtest

moz-tools-bot · 2024-05-28T13:20:53Z

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

moz-tools-bot · 2024-05-28T13:20:53Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/1724685ff947a26/output.txt

moz-tools-bot · 2024-05-28T13:28:39Z

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/8b3e402acee8dcf/output.txt

Total script time: 7.75 mins

Integration Tests: Passed

moz-tools-bot · 2024-05-28T13:39:53Z

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/1724685ff947a26/output.txt

Total script time: 18.98 mins

Integration Tests: Passed

Snuffleupagus

r=me, with a couple of final comments; thank you!

web/pdf_viewer.js

This parameter allows defining which point should remain fixed while scaling the document. It can be used, for example, to implement "zoom around the cursor" or "zoom around pinch center". The logic was previously implemented in `web/app.js`, but moving it to the viewer scaling utilities themselves makes it easier to implement similar zooming functionalities in other embedders.

Snuffleupagus · 2024-05-28T14:30:14Z

/botio-linux integrationtest

moz-tools-bot · 2024-05-28T14:30:18Z

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

moz-tools-bot · 2024-05-28T14:38:08Z

From: Bot.io (Linux m4)

Success

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

Total script time: 7.82 mins

Integration Tests: Passed

nicolo-ribaudo · 2024-05-28T14:44:17Z

Thanks for the quick review!

nicolo-ribaudo changed the title ~~Simplify API to implement zoom in custom viewers~~ [api-minor] Simplify API to implement zoom in custom viewers May 28, 2024

Snuffleupagus reviewed May 28, 2024

View reviewed changes

test/integration/viewer_spec.mjs Outdated Show resolved Hide resolved

Snuffleupagus reviewed May 28, 2024

View reviewed changes

web/pdf_viewer.js Outdated Show resolved Hide resolved

nicolo-ribaudo force-pushed the zooming-utilities branch 2 times, most recently from 3d6ce52 to b65ba0d Compare May 28, 2024 11:15

timvandermeij added the viewer label May 28, 2024

Snuffleupagus reviewed May 28, 2024

View reviewed changes

web/pdf_viewer.js Outdated Show resolved Hide resolved

nicolo-ribaudo force-pushed the zooming-utilities branch from b65ba0d to 9d334fc Compare May 28, 2024 12:02

Snuffleupagus approved these changes May 28, 2024

View reviewed changes

web/pdf_viewer.js Outdated Show resolved Hide resolved

web/pdf_viewer.js Outdated Show resolved Hide resolved

nicolo-ribaudo force-pushed the zooming-utilities branch from 9d334fc to b7933d8 Compare May 28, 2024 14:19

Snuffleupagus merged commit 0cec644 into mozilla:master May 28, 2024
7 checks passed

nicolo-ribaudo deleted the zooming-utilities branch May 28, 2024 14:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[api-minor] Simplify API to implement zoom in custom viewers #18179

[api-minor] Simplify API to implement zoom in custom viewers #18179

nicolo-ribaudo commented May 28, 2024 •

edited

Loading

Snuffleupagus commented May 28, 2024

Snuffleupagus commented May 28, 2024

moz-tools-bot commented May 28, 2024

moz-tools-bot commented May 28, 2024

moz-tools-bot commented May 28, 2024

moz-tools-bot commented May 28, 2024

nicolo-ribaudo commented May 28, 2024 •

edited

Loading

Snuffleupagus commented May 28, 2024

moz-tools-bot commented May 28, 2024

moz-tools-bot commented May 28, 2024

moz-tools-bot commented May 28, 2024

moz-tools-bot commented May 28, 2024

Snuffleupagus commented May 28, 2024

moz-tools-bot commented May 28, 2024

moz-tools-bot commented May 28, 2024

Snuffleupagus commented May 28, 2024

moz-tools-bot commented May 28, 2024

moz-tools-bot commented May 28, 2024

moz-tools-bot commented May 28, 2024

moz-tools-bot commented May 28, 2024

Snuffleupagus left a comment

Snuffleupagus commented May 28, 2024

moz-tools-bot commented May 28, 2024

moz-tools-bot commented May 28, 2024

nicolo-ribaudo commented May 28, 2024

[api-minor] Simplify API to implement zoom in custom viewers #18179

[api-minor] Simplify API to implement zoom in custom viewers #18179

Conversation

nicolo-ribaudo commented May 28, 2024 • edited Loading

Snuffleupagus commented May 28, 2024

Snuffleupagus commented May 28, 2024

moz-tools-bot commented May 28, 2024

From: Bot.io (Linux m4)

Received

moz-tools-bot commented May 28, 2024

From: Bot.io (Windows)

Received

moz-tools-bot commented May 28, 2024

From: Bot.io (Linux m4)

Success

moz-tools-bot commented May 28, 2024

From: Bot.io (Windows)

Failed

nicolo-ribaudo commented May 28, 2024 • edited Loading

Snuffleupagus commented May 28, 2024

moz-tools-bot commented May 28, 2024

From: Bot.io (Linux m4)

Received

moz-tools-bot commented May 28, 2024

From: Bot.io (Windows)

Received

moz-tools-bot commented May 28, 2024

From: Bot.io (Linux m4)

Success

moz-tools-bot commented May 28, 2024

From: Bot.io (Windows)

Success

Snuffleupagus commented May 28, 2024

moz-tools-bot commented May 28, 2024

From: Bot.io (Linux m4)

Received

moz-tools-bot commented May 28, 2024

From: Bot.io (Linux m4)

Success

Published

Snuffleupagus commented May 28, 2024

moz-tools-bot commented May 28, 2024

From: Bot.io (Linux m4)

Received

moz-tools-bot commented May 28, 2024

From: Bot.io (Windows)

Received

moz-tools-bot commented May 28, 2024

From: Bot.io (Linux m4)

Success

moz-tools-bot commented May 28, 2024

From: Bot.io (Windows)

Success

Snuffleupagus left a comment

Choose a reason for hiding this comment

Snuffleupagus commented May 28, 2024

moz-tools-bot commented May 28, 2024

From: Bot.io (Linux m4)

Received

moz-tools-bot commented May 28, 2024

From: Bot.io (Linux m4)

Success

nicolo-ribaudo commented May 28, 2024

nicolo-ribaudo commented May 28, 2024 •

edited

Loading

nicolo-ribaudo commented May 28, 2024 •

edited

Loading