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

Workaround WebKit canvas rendering bug when zooming PDFs #17571

Closed

Conversation

nicolo-ribaudo
Copy link
Contributor

@nicolo-ribaudo nicolo-ribaudo commented Jan 24, 2024

WebKit has a bug when rendering canvases that have their width defined as relative to their container, and their parent is resized. This causes a This causes a "tearing" effect when zooming in PDFs past a certain threshold, which is device-dependent but tends to be between 120% and 200%.

The DOM structure around the canvas is as follows:

div.pdfViewer [ --scale-factor: ...; ]
  > div.page [ width: round(var(--scale-factor) * ..., 1px); ]
    > div.canvasWrapper
      > canvas

Setting width: inherit on the div.canvasWrapper and on the canvas is equivalent to explicitly specifying the same width property in pixels as in div.page, thus making Safari properly redraw the canvas on resize.

See https://bugs.webkit.org/show_bug.cgi?id=267986 for more details on the WebKit bug.

Fixes #16155, fixes #16329, fixes #17459, fixes #17713

#17459 shows a good video of what the rendering bug fixed by this PR is.


Note that it's very easy to fix this for downstream consumers of pdf.js. I think adding this CSS to their page is enough:

.pdfViewer .canvasWrapper,
.pdfViewer .page canvas[zooming] {
  width: inherit !important;
}

however, I'm opening this PR because

  1. I noticed that multiple people submitted issues about it
  2. the fix is very minimal and does not impact other browser, given than in this case width: 100% and width: inherit should have the same behavior.

Marking as a draft because I tested it on an iPad but I'm waiting for something with an iPhone to confirm that this indeed works for them too :)

WebKit has a bug when rendering canvases that have their width defined as
relative to their container, and their parent is resized. This causes a
"tearing" effect when zooming in PDFs past a certain threshold, which
is device-dependent but tends to be between 120% and 200%.

The DOM structure around the canvas is as follows:
  div.pdfViewer [ --scale-factor: ...; ]
    > div.page [ width: round(var(--scale-factor) * ..., 1px); ]
      > div.canvasWrapper
        > canvas

Setting `width: inherit` on the div.canvasWrapper and on the canvas is
equivalent to explicitly specifying the same `width` property in pixels
as in div.page, thus making Safari properly redraw the canvas on resize.

See https://bugs.webkit.org/show_bug.cgi?id=267986 for more details
on the WebKit bug.

Fixes mozilla#16155, fixes mozilla#16329, fixes mozilla#17459
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.

Sorry, but why is this not a WebKit bug that ought to be addressed in those browsers rather than worked-around here?

2. the fix is very minimal and does not impact other browser, given than in this case width: 100% and width: inherit should have the same behavior.

Please note that we really want to avoid introducing any browser-specific compatibility hacks in the main code-base.
The reasons for this is to not negatively impact readability/maintainability of the code, and to avoid having to deal with any potential regressions later (given the "should"-part this just seems like unnecessary risk).

Please keep in mind that the built-in Firefox PDF Viewer is the primary development target for the PDF.js library.

@box10
Copy link

box10 commented Jan 25, 2024

thanks so much. this seems to work fine on iPad and from my quick test, also on iPhone

@extremegf
Copy link

I can confirm, that adding css

.pdfViewer .canvasWrapper,
.pdfViewer .page canvas[zooming] {
  width: inherit !important;
}

to viewer.css completely resolves the issue on iPhone iOS 16.7.4.

@Snuffleupagus
Copy link
Collaborator

As mentioned in #17571 (review) we generally don't want to add browser-specific compatibility hacks in the main code base, even more so when the browser in question is only listed as "Mostly" supported; please note https://github.com/mozilla/pdf.js/wiki/Frequently-Asked-Questions#legacy-build

@nicolo-ribaudo nicolo-ribaudo deleted the safari-zoom-fix branch April 10, 2024 09:53
@calixteman
Copy link
Contributor

I had a look and removing the display property:

display: block;

(or setting it to inline) fixes the issue for me with Safari on mac.
I don't see any reason why it's block, as far as I can tell it comes from:
#136
Since we add some svg elements in the canvas wrapper when highlighting, I checked if everything is ok and it seems to be.
So I guess we can remove it.
And tbh, I wasn't super excited by the original patch, especially because it's hard to test and because I had the feeling that even if it was small it could have led to easily introduce some regressions in the future.
@nicolo-ribaudo, could you check that it fixes the issue on ipad/ios (I don't have such devices) ? and if it's ok for you, please make an other PR.

@Snuffleupagus
Copy link
Collaborator

Re #17571 (comment), please let's make a new release first (as mentioned elsewhere) before we consider hacking around a Safari-only bug.

@calixteman
Copy link
Contributor

Re #17571 (comment), please let's make a new release first (as mentioned elsewhere) before we consider hacking around a Safari-only bug.

Yes of course, there is no rush here.

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