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

[Bug]: White border in viewer for PDFs with background color #18694

Closed
roland-strasser opened this issue Sep 5, 2024 · 8 comments · Fixed by #18698
Closed

[Bug]: White border in viewer for PDFs with background color #18694

roland-strasser opened this issue Sep 5, 2024 · 8 comments · Fixed by #18698

Comments

@roland-strasser
Copy link

Attach (recommended) or Link to PDF file

test.pdf

Web browser and its version

130.0 (64-Bit)

Operating system and its version

Windows 10

PDF.js version

Stable (v4.6.82)

Is the bug present in the latest PDF.js version?

Yes

Is a browser extension

No

Steps to reproduce the problem

1.) Create a PDF with a background color other than plain white
2.) Open the PDF in the viewer
3.) Increase/Decrease the zoom level until a white border appears on the right/bottom edge of the page.

What is the expected behavior?

There is no white line between the PDFs background and the background of the viewer.

grafik

What went wrong?

grafik

Link to a viewer

No response

Additional context

It looks like there is a rounding error for the height and width of the #viewer .page element. My workaround was to set background-color: var(--body-bg-color); in the .pdfViewer .page CSS selector.

Maybe related: #18680

@roland-strasser roland-strasser changed the title [Bug]: [Bug]: White border in viewer for PDFs with background color Sep 5, 2024
@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Sep 5, 2024

This appears to be a regression from PR #18218, and more specifically the roundToDivide -> floorToDivide helper function change since reverting only that part seems to improve things.

Edit: The purpose of the roundToDivide helper was that the canvas-element should always be as large or slightly larger than the page-container, to prevent exactly the problem reported in this issue.

/cc @nicolo-ribaudo

@nicolo-ribaudo
Copy link
Contributor

I'll take a look

@calixteman
Copy link
Contributor

I wonder if the right fix for this bug could be to:

  • set the css width and height to 100% for the canvas
  • set the canvas to whatever width and height but recompute the scale factor accordingly to the new values and use it when scaling the canvas before drawing.

If the css style & width are too small then we see the page background.
And if the scale factor isn't recomputed (which is the case right now iirc), we can have a canvas slightly too big and it could lead to a more or less gray line around the canvas (around but inside).

@calixteman
Copy link
Contributor

I just tested this patch:

diff --git a/web/pdf_page_view.js b/web/pdf_page_view.js
index 3193e3785..87b3afade 100644
--- a/web/pdf_page_view.js
+++ b/web/pdf_page_view.js
@@ -1036,19 +1036,22 @@ class PDFPageView {
     const sfx = approximateFraction(outputScale.sx);
     const sfy = approximateFraction(outputScale.sy);
 
-    canvas.width = floorToDivide(width * outputScale.sx, sfx[0]);
-    canvas.height = floorToDivide(height * outputScale.sy, sfy[0]);
-    const { style } = canvas;
-    style.width = floorToDivide(width, sfx[1]) + "px";
-    style.height = floorToDivide(height, sfy[1]) + "px";
+    const newWidth = (canvas.width = floorToDivide(
+      width * outputScale.sx,
+      sfx[0]
+    ));
+    const newHeight = (canvas.height = floorToDivide(
+      height * outputScale.sy,
+      sfy[0]
+    ));
+    const scaleX = newWidth / width;
+    const scaleY = newHeight / height;
 
     // Add the viewport so it's known what it was originally drawn with.
     this.#viewportMap.set(canvas, viewport);
 
     // Rendering area
-    const transform = outputScale.scaled
-      ? [outputScale.sx, 0, 0, outputScale.sy, 0, 0]
-      : null;
+    const transform = outputScale.scaled ? [scaleX, 0, 0, scaleY, 0, 0] : null;
     const renderContext = {
       canvasContext: ctx,
       transform,
diff --git a/web/pdf_viewer.css b/web/pdf_viewer.css
index d65173b81..854eb298f 100644
--- a/web/pdf_viewer.css
+++ b/web/pdf_viewer.css
@@ -83,6 +83,8 @@
     canvas {
       margin: 0;
       display: block;
+      width: 100%;
+      height: 100%;
 
       &[hidden] {
         display: none;

and it seems to work pretty well, especially it fixes an issue I noticed a while ago: when zooming a pdf (e.g. tracemonkey.pdf) we can see a very small movement when the canvas is redrawing.

@Snuffleupagus, @nicolo-ribaudo wdyt ?

@nicolo-ribaudo
Copy link
Contributor

nicolo-ribaudo commented Sep 5, 2024

In practice that solution is stretching the canvas a bit. With the test PDF linked above and zoom as 70% I see the canvas as having width/height of 555px/785px, and the container is 556px/786px, so setting the canvas CSS dimensions to 100% would stretch it by 1px. I wonder whether this could cause problems with anti-aliasing?

The alternative solution I was thinking about is to use round(down, var(--scale-factor) * ..., 1px) instead of round(var(--scale-factor) * ..., 1px) for the .page size, so that we also use floor in the CSS and not just in the JS.

@calixteman
Copy link
Contributor

Using down wfm locally, here's a draft patch to play with:
#18698

@Snuffleupagus
Copy link
Collaborator

The alternative solution I was thinking about is to use round(down, var(--scale-factor) * ..., 1px) instead of round(var(--scale-factor) * ..., 1px) for the .page size, so that we also use floor in the CSS and not just in the JS.

As mentioned in #18698 (comment), if that's enough to address this issue I believe we should do that first.

@timvandermeij timvandermeij linked a pull request Sep 6, 2024 that will close this issue
@roland-strasser
Copy link
Author

thank you!

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

Successfully merging a pull request may close this issue.

4 participants