Set the dimensions of the various layers at their creation #15770

calixteman · 2022-12-01T18:41:03Z

Use a unique helper function in display/display_utils.js;
Move those dimensions in the css' side.

src/display/display_utils.js

Snuffleupagus · 2022-12-02T09:00:50Z

I'm holding off on testing/review this a little bit, since it seems like a good idea to not land this and the textLayer changes in the same mozilla-central update to make finding/handling any potential regressions easier.

Snuffleupagus · 2022-12-06T11:55:07Z

Let's check that all tests pass before I attempt to test/review this in more detail.

/botio test

pdfjsbot · 2022-12-06T11:55:09Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2022-12-06T11:55:09Z

From: Bot.io (Linux m4)

Received

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

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

pdfjsbot · 2022-12-06T12:20:41Z

From: Bot.io (Linux m4)

Failed

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

Total script time: 25.52 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: Passed
Regression tests: FAILED

  different ref/snapshot: 71

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

pdfjsbot · 2022-12-06T12:28:08Z

From: Bot.io (Windows)

Failed

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

Total script time: 32.97 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: FAILED
Regression tests: FAILED

  different ref/snapshot: 47

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

Snuffleupagus · 2022-12-06T12:36:24Z

/botio-linux preview

pdfjsbot · 2022-12-06T12:36:26Z

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

pdfjsbot · 2022-12-06T12:37:43Z

From: Bot.io (Linux m4)

Success

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

Total script time: 1.26 mins

Published

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

Snuffleupagus · 2022-12-06T13:12:03Z

One observation here is that in some PDF documents the page-containers, and all of their layers, are no longer guaranteed to have integer dimensions. Hopefully that's not really an issue anywhere in the viewer, e.g. in the scrolling and visibility-checking code, but it's a sort-of "noticeable change" and one that I cannot immediately tell if it'll always be safe (given the age of this viewer code).
Ninja-edit: Or is the browser perhaps rounding fractional DOM element dimensions internally already, making this entire point moot?

calixteman · 2022-12-06T14:06:11Z

I'd say that the problem of non-integer dimensions exists for a while because of the % unit which like lead to non-integer numbers at the end, so I'm pretty confident.
The only issues I could imagine about rounding errors would be to have some containers with a small black line between them (exactly like the issue fixed by #14853), but in the pdf.js context we don't have such adjacent containers.
@emilio, wdyt ?

Snuffleupagus · 2022-12-06T14:25:18Z

I'd say that the problem of non-integer dimensions exists for a while because of the % unit which like lead to non-integer numbers at the end, so I'm pretty confident.

The difference is that previously it never happened for the page-elements, which means that there's no way it could've affected e.g. scrolling and visibility calculations.
Now you're changing that, hence my worry about this possibly breaking some (old) edge-cases. Note that while methods such as e.g. Element.clientWidth always return integer dimensions, however using the Inspector (in the dev-tools) fractional dimensions are displayed (for some PDF documents).

calixteman · 2022-12-06T14:56:33Z

Ah yes that's true.
I've a wip locally to use an IntersectionObserver for the visibility stuff.
In looking at the code in getVisibleElements, I don't really see what could be wrong (the various clientXXX values are rounded compared to what we have in getComputedStyle(...).width).
Anyway, let's try and we'll see/fix the problems if any (I know I'm usually too much confident).
But as usual, I really rely on your judgement: you're the old wise man here so don't hesitate to try to figure out something which could break the rendering.

Snuffleupagus · 2022-12-06T16:18:15Z

I've a wip locally to use an IntersectionObserver for the visibility stuff.

As far as I understand things, that one is async. Please keep in mind that there's a bunch of situations where we need to access the visible elements synchronously, so I'm not sure how well that'd work in general.
Furthermore given the all of the viewer-related changes recently it seems that making such large, and potentially invasive, re-factoring of old and somewhat poorly tested code carries a way too large risk here. (Since there's just so many different cases to consider...)
Hence, I'm sorry to say that such a change mostly just scares me here :-(

In looking at the code in getVisibleElements, I don't really see what could be wrong (the various clientXXX values are rounded compared to what we have in getComputedStyle(...).width).

Could it not potentially lead to the wrong page being considered as the most visible, thus affecting things such as rendering prioritization in some edge-cases, exactly because of the rounding that you mention!?
I suppose that we can't easily e.g. floor the values computed by the new setLayerDimensions function, since that ought to prevent any issues?

calixteman · 2022-12-06T16:37:51Z

I've a wip locally to use an IntersectionObserver for the visibility stuff.

As far as I understand things, that one is async. Please keep in mind that there's a bunch of situations where we need to access the visible elements synchronously, so I'm not sure how well that'd work in general. Furthermore given the all of the viewer-related changes recently it seems that making such large, and potentially invasive, re-factoring of old and somewhat poorly tested code carries a way too large risk here. (Since there's just so many different cases to consider...) Hence, I'm sorry to say that such a change mostly just scares me here :-(

Yep I know it's risky and I'll try to figure out a way to reduce the risk as much as possible but this patch is not a priority for the moment.

In looking at the code in getVisibleElements, I don't really see what could be wrong (the various clientXXX values are rounded compared to what we have in getComputedStyle(...).width).

Could it not potentially lead to the wrong page being considered as the most visible, thus affecting things such as rendering prioritization in some edge-cases, exactly because of the rounding that you mention!? I suppose that we can't easily e.g. floor the values computed by the new setLayerDimensions function, since that ought to prevent any issues?

In the future, we can use:
https://developer.mozilla.org/en-US/docs/Web/CSS/round

I think Math.floor(x) === Math.round(x - 0.5) so since it appears that clientWidth === round(getComputedStyle(..).width) a way to floor would be to just subtract 0.5 in the calc.

calixteman · 2022-12-06T16:45:12Z

Here is what I get in using the -0.5 trick with two different scale factors: 1.2 and 1.4666666666666668

So the values are really floored to get the clientXXX ones.

Snuffleupagus · 2022-12-06T17:03:40Z

In the future, we can use:
https://developer.mozilla.org/en-US/docs/Web/CSS/round

What's the status of that feature, w.r.t. being enabled by default?
Could we perhaps enable that unconditionally for the Firefox PDF Viewer specifically, since I seem to recall that there's existing cases where we "force-enable" features in that case?

calixteman · 2022-12-06T17:12:00Z

I've a wip locally to use an IntersectionObserver for the visibility stuff.

As far as I understand things, that one is async. Please keep in mind that there's a bunch of situations where we need to access the visible elements synchronously, so I'm not sure how well that'd work in general. Furthermore given the all of the viewer-related changes recently it seems that making such large, and potentially invasive, re-factoring of old and somewhat poorly tested code carries a way too large risk here. (Since there's just so many different cases to consider...) Hence, I'm sorry to say that such a change mostly just scares me here :-(

And to clarify my goal isn't to change just because changing is kind of good/cool, but I just want to simplify things because I want to not be scared by any changes and I want to improve performance in general.
And for example, from my pov, not being able to have this patch:
#15721
is a way more scary: I absolutely don't care about the patch by itself but not being able to have is... scary.
Originally the patch for scrollIntoView was not that bad, I mean it was the way to deal with the scrollIntoView in an iframe issues but nowadays this function is untouchable in the context we use it.
FYI, I wrote #15036 just to write this one #15060 because it was super painful to try to compose the rotations when generating the transform for each annotations.
So this change and few others I made just aim to simplify things, move what we can in the CSS because it's faster (you taught me that) and overall try to improve things.

calixteman · 2022-12-06T18:08:53Z

In the future, we can use:
https://developer.mozilla.org/en-US/docs/Web/CSS/round

What's the status of that feature, w.r.t. being enabled by default? Could we perhaps enable that unconditionally for the Firefox PDF Viewer specifically, since I seem to recall that there's existing cases where we "force-enable" features in that case?

For now it's only in nightly:
https://searchfox.org/mozilla-central/rev/a44deb21744de6269329b05461c55488a147f95e/modules/libpref/init/StaticPrefList.yaml#8028
Considering that the pref is used in the css parser:
https://searchfox.org/mozilla-central/rev/a44deb21744de6269329b05461c55488a147f95e/servo/components/style/values/specified/calc.rs#789
I'm not sure it's possible to force the pref when the principal is the pdf.js one, I'll ask.

src/display/display_utils.js

Snuffleupagus · 2022-12-08T15:22:57Z

In the future, we can use:
https://developer.mozilla.org/en-US/docs/Web/CSS/round

What's the status of that feature, w.r.t. being enabled by default? Could we perhaps enable that unconditionally for the Firefox PDF Viewer specifically, since I seem to recall that there's existing cases where we "force-enable" features in that case?

For now it's only in nightly: https://searchfox.org/mozilla-central/rev/a44deb21744de6269329b05461c55488a147f95e/modules/libpref/init/StaticPrefList.yaml#8028 Considering that the pref is used in the css parser: https://searchfox.org/mozilla-central/rev/a44deb21744de6269329b05461c55488a147f95e/servo/components/style/values/specified/calc.rs#789 I'm not sure it's possible to force the pref when the principal is the pdf.js one, I'll ask.

Did you receive any feedback on this, since it'd likely be the overall "safest" thing we could to do here?

calixteman · 2022-12-08T16:00:43Z

Yep, @emilio told me that we could try to enable it by default in 110.
Anyway the -0.5px does the trick so why not using it if you prefer.
Another thing I thought about: you told that IntersectionObserve is async and that's correct but right now it's the same
Each scroll event just add either a callback in window.requestAnimationFrame and with my setup it seems that it's called every 16ms (likely 60 fps) or just do nothing if a callback is already there.
When the callback is called then PDFViewer::update is called and then getVisibleElements. Between the scrollEvent which triggered the update and the call to getVisibleElements some things could have happened.
So if I understand correctly, some scroll events are just skipped which means that likely few pixels are in the area when we try to compute the visibility.
And even the window.requestAnimationFrame is not exactly called every 16ms: it depends on the event queue...

So my feeling is that this patch will bring a potential new inaccuracy to something which is already inaccurate but it doesn't mean that it'll be more inaccurate (errors can compensate themselves sometimes).
I just tried to dump the hiddenHeight of the first visible element and the values dumped when scrolling are never the same from a run to an other.
Anyway, we can ask to QA to play with their mouse wheel in different configuration to try to catch something wrong.

Snuffleupagus · 2022-12-08T17:04:31Z

Another thing I thought about: you told that IntersectionObserve is async and that's correct but right now it's the same
Each scroll event just add either a callback in window.requestAnimationFrame and with my setup it seems that it's called every 16ms (likely 60 fps) or just do nothing if a callback is already there.
When the callback is called then PDFViewer::update is called and then getVisibleElements.

Please note that my point was/is not actually about that particular code-path, but rather about the various cases throughout the PDFViewer-implementation where we directly call this._getVisiblePages() in order to synchronously find out which pages are currently visible.

calixteman · 2022-12-08T17:38:25Z

Another thing I thought about: you told that IntersectionObserve is async and that's correct but right now it's the same
Each scroll event just add either a callback in window.requestAnimationFrame and with my setup it seems that it's called every 16ms (likely 60 fps) or just do nothing if a callback is already there.
When the callback is called then PDFViewer::update is called and then getVisibleElements.

Please note that my point was/is not actually about that particular code-path, but rather about the various cases throughout the PDFViewer-implementation where we directly call this._getVisiblePages() in order to synchronously find out which pages are currently visible.

Ok, so what could happen ?
Some pages in whatever view could be out of the viewport because of rounding issues, so would it mean that right now we're seeing only 1 pixel of them ? because finally floor vs round is really a matter of 1px.
Will we be wrong in not rendering this newly invisible page ? The only difference I see finally is that when the user will scroll then the page would have to be rendered when right now it's already rendered. But that means that the viewport and the page size (+its border) have the exact dimensions required to exactly have 1px of page 2 in the viewport which is unlikely.

Again, why not using the -0.5px trick in order to have exactly the same behaviour as now ?

calixteman · 2022-12-08T17:52:03Z

And if there are some bugs, we'll fix them as usual :)

Snuffleupagus · 2022-12-08T18:23:23Z

Sorry if it was unclear, #15770 (comment) was only about your previous suggestion to maybe use IntersectionObserve in the viewer (not about this patch as such).

calixteman · 2022-12-08T18:28:25Z

Sorry if it was unclear, #15770 (comment) was only about your previous suggestion to maybe use IntersectionObserve in the viewer (not about this patch as such).

Ok I thought it was about this patch.
I just used the IntersectionObserver thing as an example to show why it's right now inaccurate.
And about the IntersectionObserver thing, we'll see that later (next year), it's absolutely not a priority.

Snuffleupagus

OK let's try this, since I'm assuming that this won't land in mozilla-central until the 110-cycle thus giving us some time to find problems.
But please file a follow-up issue about using CSS round as soon as that one becomes generally available in Firefox!

src/display/editor/annotation_editor_layer.js

calixteman · 2022-12-09T13:28:08Z

OK let's try this, since I'm assuming that this won't land in mozilla-central until the 110-cycle thus giving us some time to find problems. But please file a follow-up issue about using CSS round as soon as that one becomes generally available in Firefox!

Sorry to ask again, but why not use the -0.5px trick ?

Snuffleupagus · 2022-12-09T16:25:46Z

Sorry to ask again, but why not use the -0.5px trick ?

I don't fully understand how that helps, since it still leads to non-integer dimensions for these layers. Hence we have a situation where the actual dimensions differ from what e.g. clientWidth/clientHeight returns, as far as I can tell.

Edit: However it that approach really is better, until CSS round becomes available, let's just use that instead :-)

calixteman · 2022-12-09T16:39:07Z

Sorry to ask again, but why not use the -0.5px trick ?

I don't fully understand how that helps, since it still leads to non-integer dimensions for these layers. Hence we have a situation where the actual dimensions differ from what e.g. clientWidth/clientHeight returns, as far as I can tell.

It'd help to at least have the same values as now for those dimensions.
Do we use somewhere the "real" (e.g. getComputedStyle(div).width or something similar) dimensions of the page div ?
I just noticed that the values pageView.width/pageView.height are potentially non-integers (it depends of the scale factor), so we already have a potential mismatch between clientX and pageView.X.

- Use a unique helper function in display/display_utils.js; - Move those dimensions in css' side.

calixteman · 2022-12-10T13:35:39Z

/botio integrationtest

pdfjsbot · 2022-12-10T13:35:41Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2022-12-10T13:35:41Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/8834b9fea6a4d86/output.txt

pdfjsbot · 2022-12-10T13:39:54Z

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/8834b9fea6a4d86/output.txt

Total script time: 4.21 mins

Integration Tests: Passed

pdfjsbot · 2022-12-10T13:49:06Z

From: Bot.io (Windows)

Failed

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

Total script time: 13.41 mins

Integration Tests: FAILED

Snuffleupagus · 2022-12-11T18:50:31Z

/botio makeref

pdfjsbot · 2022-12-11T18:50:33Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/2b35cd603c8b9d0/output.txt

pdfjsbot · 2022-12-11T18:50:33Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/0091948a878309c/output.txt

pdfjsbot · 2022-12-11T19:12:03Z

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/2b35cd603c8b9d0/output.txt

Total script time: 21.49 mins

Lint: Passed
Make references: Passed
Check references: Passed

pdfjsbot · 2022-12-11T19:14:42Z

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/0091948a878309c/output.txt

Total script time: 24.14 mins

Lint: Passed
Make references: Passed
Check references: Passed

…ts (follow-up of mozilla#15770) In order to move the annotations in the DOM to have something which corresponds to the visual order, we need to have their dimensions/positions which means that the parent must have some dimensions.

The annotation layer dimensions must be set before adding some elements (follow-up of #15770)

calixteman added the viewer label Dec 1, 2022

calixteman requested a review from Snuffleupagus December 1, 2022 18:41

github-advanced-security bot found potential problems Dec 1, 2022

View reviewed changes

src/display/display_utils.js Fixed Show fixed Hide fixed

calixteman force-pushed the set_dims branch 2 times, most recently from 7679888 to f74123d Compare December 1, 2022 18:58

Snuffleupagus reviewed Dec 1, 2022

View reviewed changes

src/display/display_utils.js Outdated Show resolved Hide resolved

Snuffleupagus added the core label Dec 1, 2022

calixteman force-pushed the set_dims branch from f74123d to cd3ff21 Compare December 1, 2022 19:47

calixteman force-pushed the set_dims branch from cd3ff21 to 9107b6a Compare December 6, 2022 10:55

calixteman force-pushed the set_dims branch from 9107b6a to 982c7db Compare December 6, 2022 18:11

Snuffleupagus reviewed Dec 6, 2022

View reviewed changes

src/display/display_utils.js Outdated Show resolved Hide resolved

calixteman force-pushed the set_dims branch from da4c2a5 to 79f1bd3 Compare December 8, 2022 17:52

Snuffleupagus mentioned this pull request Dec 9, 2022

Fix page-switching for landscape documents with SpreadModes and PresentationMode (PR 14877 follow-up) #15798

Merged

Snuffleupagus approved these changes Dec 9, 2022

View reviewed changes

src/display/editor/annotation_editor_layer.js Outdated Show resolved Hide resolved

Set the dimensions of the various layers at their creation

a989b5a

- Use a unique helper function in display/display_utils.js; - Move those dimensions in css' side.

calixteman force-pushed the set_dims branch from 79f1bd3 to a989b5a Compare December 10, 2022 13:35

calixteman merged commit d9f1355 into mozilla:master Dec 11, 2022

calixteman added a commit that referenced this pull request Dec 13, 2022

Merge pull request #15820 from calixteman/fix_visual_order

64786b4

The annotation layer dimensions must be set before adding some elements (follow-up of #15770)

Snuffleupagus mentioned this pull request Aug 5, 2023

Use the round CSS function in the setLayerDimensions helper function #16794

Merged

Set the dimensions of the various layers at their creation #15770

Set the dimensions of the various layers at their creation #15770

Conversation

calixteman commented Dec 1, 2022

Snuffleupagus commented Dec 2, 2022

Snuffleupagus commented Dec 6, 2022

pdfjsbot commented Dec 6, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Dec 6, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Dec 6, 2022

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Dec 6, 2022

From: Bot.io (Windows)

Failed

Snuffleupagus commented Dec 6, 2022

pdfjsbot commented Dec 6, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Dec 6, 2022

From: Bot.io (Linux m4)

Success

Published

Snuffleupagus commented Dec 6, 2022 • edited Loading

calixteman commented Dec 6, 2022

Snuffleupagus commented Dec 6, 2022 • edited Loading

calixteman commented Dec 6, 2022

Snuffleupagus commented Dec 6, 2022

calixteman commented Dec 6, 2022

calixteman commented Dec 6, 2022

Snuffleupagus commented Dec 6, 2022

calixteman commented Dec 6, 2022

calixteman commented Dec 6, 2022

Snuffleupagus commented Dec 8, 2022

calixteman commented Dec 8, 2022

Snuffleupagus commented Dec 8, 2022

calixteman commented Dec 8, 2022

calixteman commented Dec 8, 2022

Snuffleupagus commented Dec 8, 2022

calixteman commented Dec 8, 2022

Snuffleupagus left a comment

Choose a reason for hiding this comment

calixteman commented Dec 9, 2022

Snuffleupagus commented Dec 9, 2022 • edited Loading

calixteman commented Dec 9, 2022

calixteman commented Dec 10, 2022

pdfjsbot commented Dec 10, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Dec 10, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Dec 10, 2022

From: Bot.io (Linux m4)

Success

pdfjsbot commented Dec 10, 2022

From: Bot.io (Windows)

Failed

Snuffleupagus commented Dec 11, 2022

pdfjsbot commented Dec 11, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Dec 11, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Dec 11, 2022

From: Bot.io (Linux m4)

Success

pdfjsbot commented Dec 11, 2022

From: Bot.io (Windows)

Success

Snuffleupagus commented Dec 6, 2022 •

edited

Loading

Snuffleupagus commented Dec 6, 2022 •

edited

Loading

Snuffleupagus commented Dec 9, 2022 •

edited

Loading