1740304 - Print to PDF - Printout doesn't match Print Preview, with each page's content being positioned progressively higher (eventually clipping all of the content)

Status: VERIFIED FIXED

(Core :: Printing: Output, defect)

Description

[Eduardo Reyes]

Attachment: Preview / Printout comparison

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:94.0) Gecko/20100101 Firefox/94.0

Steps to reproduce:

1. Point the browser to https://www.hashicorp.com/privacy
2. File/Print ... or Ctrl-P to open Print dialog
3. Select either Save to PDF or Microsoft Print to PDF
4. Preview is Ok

Actual results:

Printout does not match the Preview (missing contents at page breaks)

Expected results:

Printout matchintg the Preview

Comment 1

[Alice0775 White]

I can reproduce the issue in Nightly96 Windows10

Comment 2

[Alice0775 White]

[Tracking Requested - why for this release]: some lines after pagebreak is truncated in print output.

Comment 3

[Alice0775 White]

Mozregression:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=84a86222614e711f0011a04aa20d49333bd62a3e&tochange=230b74c414e78e44cc0d589ec704285eb67afb99

Before landing Bug 1681052 : 2nd page is completely blank, only header/footer.
After landing Bug 1681052 : Some lines after page break is truncated in print output

Comment 4

[Ciprian Georgiu, Desktop QA]

Removing regressionwindow-wanted keyword and adding the regression one, since it was found in the previous comment.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

:alice0775, since this bug is a regression, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.

Comment 6

[Alice0775 White]

This is not actually a regression. However, Bug 1681052 causes it to be printed incorrectly.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Mats maybe it's an offset that the fragmentation fallback computes wrong for printing, but not on preview, perhaps you have a hunch of what can be going wrong?

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Data loss in print output is bad, setting S2. Redirecting to Miko.

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Alice0775 White from comment #6)

This is not actually a regression. However, Bug 1681052 causes it to be printed incorrectly.

Specifically, if you set layout.display-list.improve-fragmentation to false (i.e. behave as we did before that^ bug landed), then we only print 2 pages -- one with the truncated content, and one blank page.

Note that the page has overflow-x: hidden; on the body element; if I remove that, then it prints just fine (regardless of layout.display-list.improve-fragmentation)

Comment 10

[Daniel Holbert [:dholbert]]

Normally we propagate scrollbars from the body to the root scrollport, which should make that overflow styling on the body shouldn't be able to make a difference.

But in this case, the propagation doesn't seem to happen quite in the way we expect, I think due to the fact that the root element and the body element have different display values. (The body has display:flex.)

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Here's a reduced testcase. The key requirements are display:flex (or display:grid works too) on the body element, as well as overflow:[non-visible] on that element.

Observations:

• If you print-preview this testcase in the default Firefox configuration, then you get several pages of output, possibly with lines of text being "sliced" if they fall on a pagebreak.

• If you print-preview this testcase with the fragmentation fallback disabled, then we just print 1 page, with truncated content at the bottom.

• If you remove either the display or the overflow declaration from the testcase, then it print-previews just fine and doesn't depend on the fragmentation-fallback codepath at all. (Similarly, if you move the overflow styling to the html element, then it works fine.)

Ideally, we should fix this by fragmenting this page properly, probably by making it behave as if overflow had been specified on the root element.

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #11)

Ideally, we should fix this by fragmenting this page properly, probably by making it behave as if overflow had been specified on the root element.

But also: we should ensure that the fragmentation-fallback doesn't completely-lose any content here, as is currently happening per the screenshot in comment 0 (where the line at the top of the second page is different in printout vs. print-preview).

Comment 13

[Eduardo Reyes]

Taking into account the on going work on this bug, I would like to call the developers attention to the fact that sometimes there are missing lines from the bottom of a page printed to PDF. This happens randomly but since it is related to the PDF module perhaps this issue could be also checked. Thanks.

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to Eduardo Reyes from comment #13)

Taking into account the on going work on this bug, I would like to call the developers attention to the fact that sometimes there are missing lines from the bottom of a page printed to PDF

Thanks! If you can come up with a specific site and/or steps to trigger that issue, would you mind filing them in a new bug? (possibly using "new/clone" button at the top right of this bug, and choosing "as a clone of this bug")

That will copy over the appropriate metadata, while still letting us track/investigate that issue independently (since it's likely a different-but-related underlying cause).

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: screenshot of testcase 1 in print-preview (on left) vs. resulting PDF output (on right)

In this bug here, it seems like the missing content is always at the top of a page, when printed to PDF. (it's fine in print preview).

This can be seen when previewing vs. printing-to-pdf with the attached testcase. Here's a screenshot showing that -- when printed to PDF, lines 49-55 seem to have been truncated, due to having been positioned "off the top" of page 2.

It looks like we're just painting each page's content at the wrong position, and we're undershooting each page's position by ~5 lines (which makes each page's content progressively higher and higher, and eventually pages end up completely blank because we're positioning their intended content at the wrong spot, more than 1 page-height away).

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: broken PDF printout of testcase 1

Here's how testcase 1 looks when printed to PDF. Note each page's content is printed progressively higher and higher (with some content lost due to having been shoved off the top of the page).

This leaves the bottom portion of page 2 and onward having some blank area (more on each page). This might be what Eduardo was referring to in comment 13 (if so, then never mind about the request-to-file-a-separate-bug -- I think that's still part of this same underlying issue).

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (static copy of the original site)

For archival/fix-validation purposes, here's a static copy of the original site that was triggering this issue. (simplified using devtools a bit, and then unified into a standalone file using the "Testcase Reducer" add-on).

Comment 18

[Eduardo Reyes]

(In reply to Daniel Holbert [:dholbert] from comment #16)

Created attachment 9253985 [details]
broken PDF printout of testcase 1

Here's how testcase 1 looks when printed to PDF. Note each page's content is printed progressively higher and higher (with some content lost due to having been shoved off the top of the page).

This leaves the bottom portion of page 2 and onward having some blank area (more on each page). This might be what Eduardo was referring to in comment 13 (if so, then never mind about the request-to-file-a-separate-bug -- I think that's still part of this same underlying issue).

You're correct Daniel! That's what I was referring to.

Comment 20

[Daniel Holbert [:dholbert]]

Mats is no longer active; canceling his needinfo-request.

miko, perhaps you could take a look if you have cycles?

Comment 22

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (like testcase 1 but with margins set to 0)

Here's a modified version of my reduced 'testcase 1' where I've specified 0 margins on all the relevant things (the page, the body, and the ol element).

With this version, each page's content seems to be positioned about 1-1.5 line-heights higher than it should be (as compared to testcase 1 where we're off by 5-6 line heights). So we're much closer to correct, with this version.

This leads me to think:
(1) the margins (in particular the page-margin) are responsible for part of our mis-positioning here.
(2) I wonder if the remaining offset in this case has something to do with the distance-between-pages in our Print Preview presentation -- that distance is roughly 1-1.5 line-heights, which is suspiciously close to the distance that we're off by here. e.g. maybe we're improperly accounting for that distance (shifting content up by that much) when we're doing an actual print operation, or something along those lines.

Comment 23

[Daniel Holbert [:dholbert]]

Attachment: broken PDF printout of testcase 3

Comment 24

[Daniel Holbert [:dholbert]]

Also notable: if I print with multiple pages-per-sheet, then this bug isn't reproducible; we produce the correct output. This is only a problem with 1-page-per-sheet (the default).

EDIT: More specifically, we produce the correct output on the first sheet. Subsequent (if more than one sheet is required) are affected by this bug and seem to have entirely blank page-content areas.

Comment 25

[Daniel Holbert [:dholbert]]

Attachment: testcase 4 (just a tall inline-block)

Here's a further-reduced testcase, which just uses an inline-block, and which reproduces the bug.

This demonstrates that the requirements described in comment 11 (display:flex/grid and nondefault overflow on body) aren't actually strict requirements -- this is unfortunately broken whenever the fragmentation-ballback codepath is triggered (for actual printed/printed-to-PDF output, not for print-preview).

Also, I confirmed that this was already-broken at the time that we landed the fragmentation-fallback codepath (in bug 1640197), but I think we didn't notice at the time because print-preview looked correct. But I just confirmed that this was even broken back then, using testcase #3 from that bug, with Nightly 2020-10-29 and layout.display-list.improve-fragmentation set to true, printing to PDF.

Comment 26

[Daniel Holbert [:dholbert]]

I suspect this has something to do with the offsetToCurrentPageBStart math in nsPageContentFrame.cpp's BuildPreviousPageOverflow() function. Right now, that just adds up the BSizes of the previous nsPageContentFrame continuations, which perhaps doesn't quite give us the right offset.

(Intuitively it feels like we should be using nsIFrame::OffsetTo() there, but that doesn't quite work either; in particular, when we have multiple pages-per-sheet, they all have a layout-position of 0,0 on the sheet, which means OffsetTo() would give the wrong position. Though we could conceivably change that if we needed to & if it made things less-wacky here.)

Comment 27

[Daniel Holbert [:dholbert]]

A few other interesting things:
(1) If I set the -moz-printed-sheet vertical-margin to 0 (which visually smooshes the pages together in the print-preview layout), and I print the zero-margins-testcase like testcase 3 to PDF, then it prints just fine. No content is pushed off the top, and no content is clipped at the bottom.

(2) Conversely: if I set the -moz-printed-sheet vertical margin to something large like 2in, then the bug gets more-severe; it pushes ~2in of content off the top of page 2, and clips ~2in at the bottom.

Conclusion: the issue here is related to that distance (the conceptual distance between pages -- really, the distance between page-content-frames -- as represented in the frame tree).

(3) I think we've got a function-call that's correcting for this^ distance, which is having an effect in print-preview but is not having an effect in printed output. Specifically this call to buildingForChild.SetAdditionalOffset():
https://searchfox.org/mozilla-central/rev/d5113f9c616b75b7919916f5fe4b4f8f78191ac9/layout/generic/nsPageContentFrame.cpp#253-254
If I comment that call out, it makes no difference in the printed output (!!) but it does make a difference to the print-preview rendering -- in particular, it makes the print-preview rendering break in exactly the same way that the printed output is broken!

Conclusion: it seems that this buildingForChild.SetAdditionalOffset() call is correcting for something in print-preview but failing to have an effect in printing.

This SetAdditionalOffset API is only called in this one place, and the API was added as part of this fragmentation-fallback implementation, so it's not-too-surprising that it's involved in the brokenness here.

Comment 28

[Daniel Holbert [:dholbert]]

Yeah, SetAdditionalOffset gets consumed in nsDisplayListBuilder::FindReferenceFrameFor and impacts the aOffset outparam there (via the MaybeApplyAdditionalOffset lamda), during print-preview.

But during print-to-PDF, we don't enter the loop that calls MaybeApplyAdditionalOffset, and we fall down to a lower clause which does not cause MaybeApplyAdditionalOffset but probably should. If I add a MaybeApplyAdditionalOffset call there (after we set the aOffset outparam), then that seems to fix this bug, with all attached testcases here.

Patch coming up. Still need to find a way to test this -- I think Web Platform Tests' "print" tests do exercise our PDF print backend, so hopefully those might work? Anyway, missing tests for now but coming soon.

Comment 29

[Daniel Holbert [:dholbert]]

Attachment: Bug 1740304: Apply the print fragmentation-fallback "additional offset" in one case where it was being missed. r?miko

We call MaybeApplyAdditionalOffset a few lines up when setting *aOffset, but it
looks like we missed this particular codepath. This was causing content to
render properly during print-preview (which takes the upper codepath) but not
during actual printing (which takes the lower codepath, the one where I'm
adding the new call).

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/073faa92644b
Apply the print fragmentation-fallback "additional offset" in one case where it was being missed. r=miko

Comment 31

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/32605 for changes under testing/web-platform/tests

Comment 32

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/073faa92644b

Comment 33

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 34

[Daniel Holbert [:dholbert]]

Comment on attachment 9261126 [details]
Bug 1740304: Apply the print fragmentation-fallback "additional offset" in one case where it was being missed. r?miko

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: dataloss bug (content missing at page breaks)
• User impact if declined: Dataloss (missing content, incorrectly-fully-blank pages) in certain printed documents. See comment 15 for more details. This can result in us e.g. losing entire steps from printed bonappetit.com recipes as in bug 1748050, or other missing content.
• Fix Landed on Version: 98
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a targeted one-line fix, which makes us more internally consistent (making our print codepath more-closely match our print-preview codepath), and which only makes a difference for a narrow category of printed content (stuff that we can't properly break across pages, which then causes us to set up the offset in question here as part of a "slicing" fallback strategy).

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9261126 [details]
Bug 1740304: Apply the print fragmentation-fallback "additional offset" in one case where it was being missed. r?miko

One-line fix w/ tests to fix a papercut dataloss bug. Approved for 97.0rc1 and 91.6esr.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3ab3e457bee4

Comment 37

[Daniel Holbert [:dholbert]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #35)

One-line fix w/ tests to fix a papercut dataloss bug. Approved for 97.0rc1 and 91.6esr.

Thanks! Agreed, nice to have this in 97.0rc1, too. (I would have explicitly requested uplift, except I misremembered that today was already merge day - I was off by a week.)

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/d8affed56ce5

Comment 39

[Catalin Sasca, Desktop QA [:csasca]]

Reproduced the issue on Firefox 96.0a1 (2021-11-09) under macOS 11.6.3 by using both tc 1 and tc 2.

The issue is fixed on Firefox 97.0, 98.0a1 (2022-01-31) and Firefox ESR 91.6.0. Tests were performed on macOS 11.6.3, Windows 11 and Ubuntu 20.04.

Comment 40

[paul]

To all:
This is great news to hear bug is fixed. However, I do need to ask if the bug fix has been explicitly tested against prior related bug 1748050 (my original bug report) and what I think I see as related (yet later) bug 1752421 (spun off by [e-mail] as an explicit test case of my original bug report).

And I do want to thank the firefox team for pushing this to esr91 ... helps one whole lot !!!

Paul

Comment 41

[paul]

to all:

I see that after I submitted my comment that bug 1748050 is automatically considered fixed ... fair enough but I would like a confirm on bug 1752421 and web site https://www.bonappetit.com/recipe/slow-cooker-beer-braised-brisket .... really want to print this recipe so it can be tried (smile)

Thanks for understanding,
Paul

Comment 42

[Daniel Holbert [:dholbert]]

(In reply to paul from comment #40)

To all:
This is great news to hear bug is fixed. However, I do need to ask if the bug fix has been explicitly tested against prior related bug 1748050 (my original bug report)

Yes, I tested this bug's patch against on your site there, per bug 1748050 comment 15. (For good measure, I just retested using current Nightly, and confirmed that the issue discussed there [content missing at pagebreaks in printed output] is indeed fixed).

and what I think I see as related (yet later) bug 1752421

That bug is about a different issue with a different root-cause, and it's not fixed by the patch here. So indeed, your recipe may not print properly yet, though it's less broken than it was. :)

And I do want to thank the firefox team for pushing this to esr91 ... helps one whole lot !!!

Thanks!

Comment 43

[Eduardo Reyes]

I would like to thank everybody for your effort in fixing this bug.

On a technical note, I just tested the same document included in the bug report and confirmed not only that the bug is fixed, but also that the output of Firefox's Save to PDF is considerable smaller that the output with the Microsoft print to PDF and Adobe PDF as shown below:

2022-02-16  03:04 PM         3,439,041 HashiCorp Privacy Policy-Adobe.pdf
2022-02-16  03:04 PM           141,526 HashiCorp Privacy Policy-Firefox.pdf
2022-02-16  03:07 PM         3,771,128 HashiCorp Privacy Policy-Microsoft.pdf

I tried another document and although not in the same order of reduction, Firefox's output is close to 50% smaller.

2022-02-16  03:33 PM           747,793 robotic fish powered by human heart cells-Adobe.pdf
2022-02-16  03:32 PM           355,778 robotic fish powered by human heart cells-Firefox.pdf
2022-02-16  03:32 PM           786,932 robotic fish powered by human heart cells-Microsoft.pdf

It seems that Firefox algorithm is quite efficient!