Skip to content
New issue

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

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

Already on GitHub? # to your account

Don't hide the different layers when zooming #19186

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

calixteman
Copy link
Contributor

When hiding a layer we set de display to none and from time to time time, if the layer is focused, it causes a focus loss.
It's especially annoying when the user is zooming while in the middle of a drawing session on mobile (it's pretty common to zoom in/out on such devices).
In considering, that few years ago, a scaling was causing a complete re-rendering of a page, it made sense to just hide the layer during the scaling in order to avoid a mismatch between element dimensions in different layers.
But nowadays, things are different because only the canvas is re-rendered the other layers are just scaled. So this patch shouldn't hurt.

When hiding a layer we set de display to none and from time to time time, if the layer is focused, it causes
a focus loss.
It's especially annoying when the user is zooming while in the middle of a drawing session on mobile (it's
pretty common to zoom in/out on such devices).
In considering, that few years ago, a scaling was causing a complete re-rendering of a page, it made sense
to just hide the layer during the scaling in order to avoid a mismatch between element dimensions in
different layers.
But nowadays, things are different because only the canvas is re-rendered the other layers are just scaled.
So this patch shouldn't hurt.
@calixteman
Copy link
Contributor Author

/botio integrationtest

@moz-tools-bot
Copy link
Collaborator

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

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

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/92790fbc735c96e/output.txt

Total script time: 10.28 mins

  • Integration Tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

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

Total script time: 23.53 mins

  • Integration Tests: FAILED

@Snuffleupagus
Copy link
Collaborator

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/3181c7b4725b710/output.txt

Total script time: 0.99 mins

Published

@Snuffleupagus
Copy link
Collaborator

A couple of questions, about code just "outside" of this patch:

  • Is it still possible for the structTree to be attached to a previous canvas when we reach the following code?

    pdf.js/web/pdf_page_view.js

    Lines 496 to 499 in 7d51d9b

    if (this.canvas && treeDom.parentNode !== this.canvas) {
    // Pause translation when inserting the structTree in the DOM.
    this.canvas.append(treeDom);
    }

    If not, can we simplify this by re-writing it as below instead?

    // Pause translation when inserting the structTree in the DOM. 
    this.canvas?.append(treeDom); 
  • The pausing of translation added in the patch made me realize that we might have forgotten one case previously?

    this.#addLayer(canvasWrapper, "canvasWrapper");

    Should that also be updated as follows?

    // Pause translation when inserting the canvasWrapper in the DOM.
    this.l10n.pause();
    this.#addLayer(canvasWrapper, "canvasWrapper"); 
    this.l10n.resume();

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

It seems that there was good reason for these layers being hidden, since there's unfortunately various problems with this patch in its current form.

Comment on lines -574 to -578
if (xfaLayerNode) {
// Hide the XFA layer until all elements are resized
// so they are not displayed on the already resized page.
this.xfaLayer.hide();
}
Copy link
Collaborator

@Snuffleupagus Snuffleupagus Dec 7, 2024

Choose a reason for hiding this comment

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

Given that the xfaLayer is a bit special, is keeping it visible actually correct and safe in general?

Ninja-edit: Testing the preview the answer unfortunately seems to be NO, since scrolling to the end of an XFA document and then rotating it triggers intermittent rendering glitches (since pages that are not visible will not be updated until they are scrolled into view again).

Here's a screen-shot of what this looks like:
xfa

Comment on lines -566 to -573
if (annotationLayerNode) {
// Hide the annotation layer until all elements are resized
// so they are not displayed on the already resized page.
this.annotationLayer.hide();
}
if (annotationEditorLayerNode) {
this.annotationEditorLayer.hide();
}
Copy link
Collaborator

@Snuffleupagus Snuffleupagus Dec 7, 2024

Choose a reason for hiding this comment

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

What about pages that are still active in the PDFPageViewBuffer buffer, but not currently visible?

These annotation-related layers won't have e.g. their rotation updated until they become visible again, and it's not clear to me if that could cause them to overflow their respective page-containers (similar to the xfaLayer) and thus potentially "mess" with layout?

@calixteman calixteman marked this pull request as draft December 9, 2024 18:00
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants