Bonus extra blank page, when printing A4-formatted PDF to A4-sized paper in Firefox, with layout.display-list.improve-fragmentation enabled
Categories
(Core :: Printing: Output, defect)
Tracking
()
Tracking | Status | |
---|---|---|
relnote-firefox | --- | 85+ |
firefox-esr78 | --- | unaffected |
firefox85 | --- | verified disabled |
firefox86 | --- | verified |
firefox87 | --- | verified |
People
(Reporter: k.fatz, Assigned: MatsPalmgren_bugz)
References
(Regression)
Details
(Keywords: regression, Whiteboard: [print2020_v86])
Attachments
(6 files)
4.53 KB,
application/pdf
|
Details | |
9.03 KB,
application/pdf
|
Details | |
4.13 KB,
text/plain
|
Details | |
48 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details | Review |
48 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details | Review |
48 bytes,
text/x-phabricator-request
|
jcristau
:
approval-mozilla-release+
|
Details | Review |
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:85.0) Gecko/20100101 Firefox/85.0
Steps to reproduce:
Open a website with integrated pdf in FF84 oder in FF85.
https://www.jena.de/fm/41/test.pdf
Actual results:
Print it in FF84 and you get one 1 printed Page.
Do the same in FF85 and you get the 1 Page with content and 1 other emty page without content.
Expected results:
In both Versions, FF84 and FF85 you should get only the ONE PAGE with content as it is shown in the browser!
Comment 2•3 years ago
|
||
Bugbug thinks this bug should belong to this component, but please revert this change in case of error.
(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #2)
Bugbug thinks this bug should belong to this component, but please revert this change in case of error.
???
Comment 4•3 years ago
|
||
(In reply to Kai from comment #3)
???
(You can disregard that Bugbug comment -- that's just a bot, auto-classifying the bug into the appropriate Bugzilla component, and documenting the fact that it's doing so.)
I can reproduce the described behavior (unexpected blank page of output, shown in print preview), if I choose A4 as the paper size in the print dialog.
Comment 5•3 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #4)
I can reproduce the described behavior (unexpected blank page of output, shown in print preview), if I choose A4 as the paper size in the print dialog.
...and after selecting A4 to make the bug reproduce, I can then make it go away by toggling the about:config pref layout.display-list.improve-fragmentation
to false
.
So, that pref-tweak (from bug 1681052) is probably the relevant change in Firefox 85 that caused this issue to start happening.
Also notable: the PDF testcase itself is in A4 format, according to evince
(the default Ubuntu PDF viewer).
Updated•3 years ago
|
Comment 6•3 years ago
|
||
ni=mats to investigate this fragmentation-fallback regression, if he has cycles to do so.
Updated•3 years ago
|
Comment 7•3 years ago
|
||
Here's the reporter's testcase, as a local attachment in Bugzilla, BTW. (originally hosted at https://www.jena.de/fm/41/test.pdf )
Assignee | ||
Comment 8•3 years ago
|
||
The fragmentation-fallback is working as intended. The problem is that the content actually overflows.
We have code to scale down pdf.sj content to fit the page here:
https://searchfox.org/mozilla-central/rev/0a489e67575540f5aeb968208ae03ff17eb71e94/layout/generic/nsPageContentFrame.cpp#98-112
I guess it's not working correctly... Hmm, doesn't mShrinkToFitRatio
affect the frame tree at some point?
Assignee | ||
Comment 10•3 years ago
|
||
So... for this document we calculate mShrinkToFitRatio = 0.999406
but then:
https://searchfox.org/mozilla-central/rev/8d290159a6f80f5e33e2bd35c0e4b2df283a18c5/layout/printing/nsPrintJob.cpp#1290
ROFL
If I change that constant to 0.99999f
then the extra page disappears...
Assignee | ||
Comment 11•3 years ago
|
||
The root cause of the mShrinkToFitRatio not being 1 is this:
maxSize.height=67280 (2.966861e+01 centimeter), heightToFit=67320 (2.968625e+01 centimeter)
(where maxSize.height is the Print Preview canvas size, heightToFit is the size of the pdf.js content)
The size of an A4 is 210 by 297 millimeters. So it appears that we have setup a Print Preview canvas size that is too small to fit an A4. (The document is 296.8625mm so it really should fit without scaling.)
67280 above is in our internal AU units (60 per CSS pixel). To be at least 297mm we would need 67352 (2.970036e+01 centimeter) (the next lower value 67351 is 2.969992e+01 centimeter). The difference between 67280 and 67352 (72) is much more than a rounding error though, it's even more than 1px, so I wonder where this bogus size came from...
Assignee | ||
Comment 12•3 years ago
|
||
So, nsPageFrame::ReflowPageContent sets up the canvas size. It gets its size from aPresContext->GetPageSize()
, and that size came from nsDeviceContext::CalcPrintingSize(), which has:
(rr) p this->mPrintTarget.mRawPtr->mSize
$27 = {
<mozilla::gfx::BaseSize<int, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> >> = {
{
{
width = 595,
height = 841
},
(rr) p this->mAppUnitsPerPhysicalInch
$28 = 5760
and POINTS_PER_INCH_FLOAT = 72.
The mPrintTarget->GetSize()
got its size from a gfxImageSurface, which got its size from nsDeviceContextSpecProxy::MakePrintTarget which got its size from mPrintSettings->GetEffectiveSheetSize which leads to nsPrintSettingsGTK::GetEffectivePageSize which got its height from gtk_paper_size_get_height. The value it returned is 11.6929131 inch, which is 29.699999274 centimeters. So it appears we start with a slightly too small value and then have compounding rounding errors through various unit conversions after that...
Fwiw, if I call gtk_paper_size_get_height(paperSize, GTK_UNIT_MM)
there I get the correct 297mm size. gtk_paper_size_get_name(paperSize)
returns "iso_a4".
Assignee | ||
Comment 13•3 years ago
|
||
Assignee | ||
Comment 14•3 years ago
|
||
Also, for GTK, request the paper size in millimeter for
millimeter based paper units to avoid depending on GTK's
rounding behavior.
Depends on D103608
Assignee | ||
Comment 15•3 years ago
|
||
Note that these two parts both fixes the problem for this given PDF by themselves. I think it's worth taking both.
I'd be happy to add a test for this but I have no clue how to write a test for Print Preview of PDF documents.
Assignee | ||
Comment 16•3 years ago
|
||
FTR, the 0.998f
number comes from this patch (it used to be 1.0
):
https://searchfox.org/mozilla-central/diff/b1cb2953ae82a8d5ca453930b739de76ada00c18/layout/base/nsDocumentViewer.cpp#4464
There's no explanation why it was chosen, nor does it seem related to the rest of that patch... the commit message is "Add the new pluggable dialog work, in build, not used", without any bug number. smh
Comment 17•3 years ago
|
||
FWIW I think it should probably be 1.0...
Comment 18•3 years ago
|
||
Set release status flags based on info from the regressing bug 1681052
Assignee | ||
Comment 19•3 years ago
•
|
||
FWIW I think it should probably be 1.0...
Yeah, but it is kind of silly to re-reflow just to scale it up 1px or less too. After digging through all the conversion steps the paper size goes through I think it probably is a good idea to have some tolerance for rounding errors etc. But yes, 0.998 is rather random...
I'll file a follow-up bug on it... (EDIT: filed bug 1690028)
Assignee | ||
Comment 23•3 years ago
|
||
We should probably uplift this fix since this bug has multiple dupes already.
Updated•3 years ago
|
Comment 24•3 years ago
|
||
Comment 25•3 years ago
|
||
For us after version 85 not only blank pages appeared but also the orientation got flipped.
Imgur album:
https://imgur.com/a/3UVFdwn
Comment 26•3 years ago
|
||
(In reply to serpher from comment #25)
For us after version 85 not only blank pages appeared but also the orientation got flipped.
Imgur album:
https://imgur.com/a/3UVFdwn
That is extremely unexpected. Would you mind filing a new bug on that?
(You can use the "new/clone" button at top right of this page, and choose "...in this component")
Comment 27•3 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #5)
(In reply to Daniel Holbert [:dholbert] from comment #4)
I can reproduce the described behavior (unexpected blank page of output, shown in print preview), if I choose A4 as the paper size in the print dialog.
...and after selecting A4 to make the bug reproduce, I can then make it go away by toggling the about:config pref
layout.display-list.improve-fragmentation
tofalse
.So, that pref-tweak (from bug 1681052) is probably the relevant change in Firefox 85 that caused this issue to start happening.
Also notable: the PDF testcase itself is in A4 format, according to
evince
(the default Ubuntu PDF viewer).
This solved (more like a workaround) the problem for now. But they need to fix it.
Comment 28•3 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #5)
So, that pref-tweak (from bug 1681052) is probably the relevant change in Firefox 85 that caused this issue to start happening.
Is this something we could (or would want to) flip back, or would that be worse?
Comment 29•3 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #26)
(In reply to serpher from comment #25)
For us after version 85 not only blank pages appeared but also the orientation got flipped.
Imgur album:
https://imgur.com/a/3UVFdwnThat is extremely unexpected. Would you mind filing a new bug on that?
(You can use the "new/clone" button at top right of this page, and choose "...in this component")
I hope I did that properly.
https://bugzilla.mozilla.org/show_bug.cgi?id=1690335
Comment 30•3 years ago
|
||
(In reply to Julien Cristau [:jcristau] from comment #28)
(In reply to Daniel Holbert [:dholbert] from comment #5)
So, that pref-tweak (from bug 1681052) is probably the relevant change in Firefox 85 that caused this issue to start happening.
Is this something we could (or would want to) flip back, or would that be worse?
Yeah, it's probably worth turning off the pref. The pref doesn't require a restart, for what it's worth (which I think gives us some flexibility about how we flip it).
(There are long-standing bugs that are mitigated by the pref's current setting, which would obviously become re-broken if we do turn off the pref; but on the other hand, we've been living with those bugs for quite some time vs. this bug seems to be a new & fairly-obvious issue for lots of users, and new bugs are more jarring than long-standing bugs, so it probably makes sense to turn the pref off & accept those old bugs for a few weeks more, and let the pref turn back on with the "real" fix in the next release.)
(In reply to serpher from comment #29)
I hope I did that properly.
Yup, thank you!
Assignee | ||
Comment 31•3 years ago
|
||
I chatted with @dholbert and I agree it's probably worth turning the pref off for now until the fixes in this bug are released.
To turn it off, set layout.display-list.improve-fragmentation
to false
.
Comment 32•3 years ago
•
|
||
After some discussion with jcristau and mythmon: It sounds like our best bet to turn off the pref would be via a traditional patch-uplift, which we could ship as a ridealong if there's a dot release to fix some other bug. (Normandy is also sort-of an option that doesn't involve shipping an update, but it turns out that approach isn't currently well-suited to turning off a pref on just a single release.)
So: if we don't end up having a dot release, then we'll have to live with this bug for a few weeks until Firefox 86 ships to release, which will hopefully include the real fix (mats' attached patches).
Updated•3 years ago
|
Comment 35•3 years ago
|
||
Should this really wait? In my experience 5 duplicates is a lot.
Assignee | ||
Comment 36•3 years ago
|
||
Assignee | ||
Comment 37•3 years ago
•
|
||
Comment on attachment 9200953 [details]
Bug 1689789 - Disable the fragmentation fallback feature on the RELEASE channel. r=dholbert
Beta/Release Uplift Approval Request
- User impact if declined: An extra blank page is generated in Print/Print Preview for some PDF files (A4 at least).
Note that this will also bring back bugs that were fixed by said feature (missing content in Print/Print Preview in some cases). - Is this code covered by automated tests?: No
- Has the fix been verified in Nightly?: No
- Needs manual test from QE?: Yes
- If yes, steps to reproduce: see bug description and duplicates
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): just a pref flip to revert to earlier behavior
Note that this patch should ONLY land on the RELEASE channel. It's mostly intended as a ride-along for any dot-release; judgement call for release managers.
(The other two patches will fix the underlying bug without disabling the pref for Nightly/Beta)
- String changes made/needed:
Assignee | ||
Updated•3 years ago
|
Comment 38•3 years ago
|
||
Comment on attachment 9200953 [details]
Bug 1689789 - Disable the fragmentation fallback feature on the RELEASE channel. r=dholbert
approved for 85.0.1
Comment 40•3 years ago
|
||
bugherder uplift |
Comment 41•3 years ago
|
||
Toggled layout.display-list.improve-fragmentation to false. No extra page any more.
Comment 42•3 years ago
|
||
Hello,
Confirming this issue as verified fixed on 85.0.1(20210204182252) with Win10x64, macOS 10.15.7 and Ubuntu 20.
layout.display-list.improve-fragmentation is switched to false and no extra pages are displayed in Print Preview.
Updated•3 years ago
|
Comment 43•3 years ago
|
||
Adding to the 85.0.1 release notes:
"Avoid printing an extra blank page at the end of some documents (bug 1689789)."
Comment 44•3 years ago
|
||
Reopening this, as the patch that landed (comment 40) didn't really fix the underlying issue, it just disabled the problematic feature on the release channel.
Comment 45•3 years ago
|
||
Pushed by mpalmgren@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/25d8b783c514 part 1 - Ensure we don't have any overflow areas for pdf.js content. r=jfkthame
Comment 46•3 years ago
|
||
Pushed by mpalmgren@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ba7cdc0b2d91 part 2 - Use Ceil instead of Truncate the ensure the print surface is large enough for the requested paper size. r=jfkthame
Comment 47•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/25d8b783c514
https://hg.mozilla.org/mozilla-central/rev/ba7cdc0b2d91
Comment 48•3 years ago
|
||
The patch landed in nightly and beta is affected.
:mats, is this bug important enough to require an uplift?
If not please set status_beta
to wontfix
.
For more information, please visit auto_nag documentation.
Assignee | ||
Comment 49•3 years ago
|
||
Comment on attachment 9200347 [details]
Bug 1689789 part 2 - Use Ceil instead of Truncate the ensure the print surface is large enough for the requested paper size. r=jfkthame
Beta/Release Uplift Approval Request
- User impact if declined: An extra blank page is generated in Print/Print Preview for some PDF files (A4 at least).
- Is this code covered by automated tests?: No
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: Yes
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): The changes are limited to print media, and are fairly non-invasive.
AFAIK, we don't have any automated regression tests for print rendering of PDF though, which adds risk. - String changes made/needed:
Assignee | ||
Updated•3 years ago
|
Comment 50•3 years ago
|
||
Comment on attachment 9200347 [details]
Bug 1689789 part 2 - Use Ceil instead of Truncate the ensure the print surface is large enough for the requested paper size. r=jfkthame
We already have 7 bugs filed for this issue and the patch is evaluated by the author as low risk, let's take it in 86 beta 9.
Updated•3 years ago
|
Comment 51•3 years ago
|
||
bugherder uplift |
Updated•3 years ago
|
Comment 52•3 years ago
|
||
Hello,
Confirming this issue as verified fixed on 86.0b9(20210211195159) and 87.0a1(20210211213143).
Verified using macOS 10.15.7, Win10x64 and Ubuntu 20x64.
Updated•3 years ago
|
Description
•