Set page size via @page + size #5857

Rob--W · 2015-03-18T22:41:20Z

In Blink-based browsers, there is a mismatch between document size and paper size. Even if exactly the same values and unit are used, it is possible that the printed results takes more pages than expected.

To solve the issue, the page size is set via @page size, and the canvas and ancestor nodes are assigned a width+height of 100% (=relative to the page). This change resolves bugs such as blank pages and split pages.

To test the result, open a PDF with A4 size, such as the ones in #5718 and open a print preview. No blanks pages should appear between the pages (in Firefox, a blank page shows up at the end, but this was already present before my patch).

Fixes #5718 (no blank pages, and the correct paper size is automatically selected if needed; only in Chrome, Firefox does not support @page size yet - https://bugzilla.mozilla.org/show_bug.cgi?id=851937).

Snuffleupagus · 2015-03-19T19:49:13Z

web/ui_utils.js

+  // To make sure that the page fits on printed paper, points are converted to
+  // pixels, the width is rounded UP, while the height is rounded DOWN.
+
+  var cssPixelsPerPoint = 96 / 72;


Since this is a constant, please change the name to CSS_PIXELS_PER_POINT.
Also, since the value is identical to CSS_UNITS, could we just use that one instead?

Rob--W · 2015-03-20T14:28:28Z

Please do not merge this PR yet. I have received a test page of size (240x440 pt = 320.00 x 586.67px) from someone else by mail, which did render on two pages instead of one in Chrome's (41.0.2272.77) print preview after I set @page { size: 240pt 440pt; }.

I'll try to improve my understanding of how the page sizes are determined/rounded/scaled in relation to the paper size.

Rob--W · 2015-03-21T11:50:24Z

I have completely rewritten my patch, and verified that it works as expected (and also with the PDF that someone mailed me in private).

@Snuffleupagus Could you review my code? See initial post for test steps.

Snuffleupagus · 2015-03-23T10:00:02Z

@Snuffleupagus Could you review my code? See initial post for test steps.

I'm sorry, but since I've never touched the printing code I don't feel comfortable doing the final review.

Snuffleupagus · 2015-03-23T10:02:46Z

web/viewer.js

@@ -1311,12 +1340,30 @@ var PDFViewerApplication = {
 //#endif
  },

+  // Whether all pages of the PDF have the same width and height.
+  hasEqualPageSizes: function pdfViewHasEqualPageSizes() {


I'd suggest changing this to a getter instead, since you're not passing any parameters to the function and it only returns a boolean.

@page

In Blink-based browsers, there is a mismatch between document size and paper size. Even if exactly the same values and unit are used, it is possible that the printed results takes more pages than expected. To solve the issue, the page size is set via @page size, and the canvas and ancestor nodes are assigned a width+height of 100% (=relative to the page). This change resolves bugs such as blank pages and split pages.

Rob--W · 2015-03-23T11:32:42Z

@Snuffleupagus Done.

@brendandahl Could you review this PR?

The code change is relatively small, you just need to make sure that I did not introduce regressions. Build PDF.js and run the following tests

Test 1

Open http://kniel.de/download/96141656.02.pdf
Click on the print button to generate a print preview in Chrome and/or print to a PDF file.
Verify that two pages are previewed without blank pages, and without cut-off (A4 size).

Test 2

Open https://dl.dropboxusercontent.com/u/7246095/example_RTL.pdf
Repeat step 2-3 of test 1 (expect 5 pages).

Test 3

Open the tracemonkey paper (not A4, probably US letter?)
Repeat step 2-3 of test 1 (expect 14 pages).

Rob--W · 2015-04-28T09:57:20Z

Ping for review? See my previous comment for steps to test.

Hengjie · 2015-05-04T04:46:48Z

web/viewer.js

+    }
+
+    // Insert a @page + size rule to make sure that the page size is correctly
+    // set. Note that we assume that all pages have the same size, because


Could you please explain more about why different page sizes aren't supported yet? Is it because we need to set different @page on a per page basis and that may cause performance issues?

Variable page sizes are not supported by PDF.js because browsers do not offer the means to implement this (named pages). This is explained on the next few lines, and the relevant browser bugs have been referenced.

Gotcha. I understand that variable pages won't work but what about PDFs with pages that have landscape and portrait dimensions that other than a simple 90deg rotation will have the same dimensions? Anecdotally, I feel that these types of PDFs are common.

You could create a separate feature request for that, and link to some PDFs for testing. pages cannot be targeted separately, but your scenario could be addressed by rotating the pages before printing.

If I'm not mistaken, pages with alternating landscape/portrait modes are not printed correctly, right? If so, then the acceptance of this patch won't be a regression.

Yeah it isn't a regression, would you prefer a PR to your Rob--W:print-page-size or to Mozilla?

I suggest a new PR that refers to this discussion for context.

I have PR the patch on #6103

Rob--W · 2015-05-09T17:06:42Z

Hey, could this PR be merged? The Notable PDF Chrome extension has already included integrated this patch in their product and a user told me that their extensions prints just fine (contrary to the unpatched PDF.js PDF Viewer).

timvandermeij · 2015-05-09T18:25:50Z

@yurydelendik Could you review this?

Rob--W · 2015-05-14T10:58:31Z

The huge latency in reviewing is very disappointing... What can I do to speed up this process?

I've since received multiple messages and feedback from users, who confirmed that this patch is working as intended. Therefore I'll be publishing an update of the Chrome extension of the master branch with this patch applied on top of it in a few minutes.

timvandermeij · 2015-05-14T11:26:15Z

I'm really sorry for the review delay. I have been very busy and only since yesterday I have had more time to do reviews (which resulted in four PRs landing already). I had hoped that @yurydelendik or @brendandahl would have time to review this PR in the meantime, but they have also been very busy on other Mozilla projects (for instance Shumway).

I'm going to review and test this today and merge if everything is indeed working correctly. I agree with you that this needs to be fixed and that is has taken to long for the review to happen. Again, my apologies for that.

Rob--W · 2015-05-14T11:46:52Z

@timvandermeij Thanks for your quick reply. Would it be okay if I review and merge PRs from others, and my own ones if they have been verified independently by others? I already have commit access to the PDF.js repository.

timvandermeij · 2015-05-14T11:51:42Z

@Rob--W You should discuss that with @yurydelendik and @brendandahl on IRC since they are the main developers. They are on IRC (#pdfjs on irc.mozilla.org) practically every day.

@page

Set page size via @page + size

timvandermeij · 2015-05-14T14:53:19Z

I agree with the other people: this patch looks really good, solves the issues in Chrome and has no effect for the other browsers. Thank you very much!

Rob--W added viewer printing chrome-specific labels Mar 18, 2015

Snuffleupagus reviewed Mar 19, 2015
View reviewed changes

Rob--W force-pushed the print-page-size branch from e9a2831 to b1f293d Compare March 21, 2015 11:43

Rob--W changed the title ~~Use pixels instead of points for page size.~~ Set page size via @page + size Mar 21, 2015

Snuffleupagus reviewed Mar 23, 2015
View reviewed changes

Rob--W force-pushed the print-page-size branch from b1f293d to d34e6dd Compare March 23, 2015 10:20

Hengjie reviewed May 4, 2015
View reviewed changes

This was referenced May 7, 2015

PDF display blank in chrome but display correctly in firefox. #5960

Closed

Some PDF's incorrectly shown when printing #5958

Closed

timvandermeij assigned yurydelendik May 9, 2015

timvandermeij assigned timvandermeij and unassigned yurydelendik May 14, 2015

timvandermeij added a commit that referenced this pull request May 14, 2015

Merge pull request #5857 from Rob--W/print-page-size

d7aa95d

Set page size via @page + size

timvandermeij merged commit d7aa95d into mozilla:master May 14, 2015

timvandermeij mentioned this pull request Jun 8, 2015

PDFs print with an additional blank pages after each page with print #6095

Closed

xlc mentioned this pull request Jun 10, 2015

Support printing page with different rotation #6103

Closed

timvandermeij mentioned this pull request Jul 30, 2015

Print Preview is not in proper format , in Chrome Browser. #6279

Closed

xlc mentioned this pull request Aug 12, 2015

Print on Chrome 45 prints extra blank pages #6345

Closed

timvandermeij mentioned this pull request Sep 22, 2015

Extra blank page printed after each page #6461

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set page size via @page + size #5857

Set page size via @page + size #5857

Rob--W commented Mar 18, 2015

Snuffleupagus Mar 19, 2015

Rob--W commented Mar 20, 2015

Rob--W commented Mar 21, 2015

Snuffleupagus commented Mar 23, 2015

Snuffleupagus Mar 23, 2015

Rob--W commented Mar 23, 2015

Rob--W commented Apr 28, 2015

Hengjie May 4, 2015

Rob--W May 4, 2015

Hengjie May 4, 2015

Rob--W May 5, 2015

Hengjie May 7, 2015

Rob--W May 7, 2015

xlc Jun 10, 2015

Rob--W commented May 9, 2015

timvandermeij commented May 9, 2015

Rob--W commented May 14, 2015

timvandermeij commented May 14, 2015

Rob--W commented May 14, 2015

timvandermeij commented May 14, 2015

timvandermeij commented May 14, 2015

Set page size via @page + size #5857

Set page size via @page + size #5857

Conversation

Rob--W commented Mar 18, 2015

Choose a reason for hiding this comment

Rob--W commented Mar 20, 2015

Rob--W commented Mar 21, 2015

Snuffleupagus commented Mar 23, 2015

Choose a reason for hiding this comment

Rob--W commented Mar 23, 2015

Rob--W commented Apr 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rob--W commented May 9, 2015

timvandermeij commented May 9, 2015

Rob--W commented May 14, 2015

timvandermeij commented May 14, 2015

Rob--W commented May 14, 2015

timvandermeij commented May 14, 2015

timvandermeij commented May 14, 2015