Borders of long table do not print after initial page
Categories
(Core :: Printing: Output, defect)
Tracking
()
People
(Reporter: f3red, Assigned: mtseng)
References
(Blocks 3 open bugs)
Details
(Keywords: regression)
Attachments
(3 files)
738.20 KB,
application/pdf
|
Details | |
2.39 KB,
text/css
|
Details | |
59 bytes,
text/x-review-board-request
|
MatsPalmgren_bugz
:
review+
lizzard
:
approval-mozilla-release-
|
Details |
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0 Build ID: 20170824053622 Steps to reproduce: Visit page http://www.384thbombgroup.com/_content/_pages/PersonalWarRecord-Beta.php?xID7uGt4=75 and select Print Preview in Firefox 55.0.3 (and other recent versions). Actual results: Table starting o page 4 of print preview has full borders as intended; remaining pages of table (5, 6, 7) have no borders. This also persists when I use Microsoft print to PDF (see attached file). Expected results: Entire table from page 4 thru 7 should have full borders.
Regression range: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3e1dea2784281f3fb022d2c96df48338afc825f5&tochange=4ab3d69a2cf09a0c888c8d3d6bc589118cb223d7
Reporter | ||
Comment 2•7 years ago
|
||
I have tried a number of css settings in this file, based on various internet searches, all with no effect.
Reporter | ||
Comment 4•7 years ago
|
||
This does not appear to be a problem with Chrome or Edge - which do not provide the (otherwise) high-quality, and standards-conforming (IMHO) print formatting that FF does.
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Comment 6•7 years ago
|
||
I'd prefer if you put it in Init instead, since that's where we usually do these kinds of initializations.
Assignee | ||
Comment 7•7 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #6) > I'd prefer if you put it in Init instead, since that's where we usually > do these kinds of initializations. Sure, I'll update the patch.
Comment hidden (mozreview-request) |
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8902167 [details] Bug 1394249 - SetNeedToCalcHasBCBorders to true when initialize nsTableFrame. https://reviewboard.mozilla.org/r/173636/#review179282 ::: layout/tables/nsTableFrame.cpp:193 (Diff revision 2) > // code in nsTableWrapperFrame depends on this being set. > WritingMode wm = GetWritingMode(); > SetSize(LogicalSize(wm, aPrevInFlow->ISize(wm), BSize(wm))); > } > + > + SetNeedToCalcHasBCBorders(true); I'm not sure I'm the best person to review this, I don't understand the continuation code all that well. It seems like we are trying to avoid calculating if we need border-collapse if we're sure we won't need it, and making this unconditional would break that. Can we only do this if we're a continuation (aPrevInFlow != nullptr), and possibly check state on the original frame?
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8902167 [details] Bug 1394249 - SetNeedToCalcHasBCBorders to true when initialize nsTableFrame. https://reviewboard.mozilla.org/r/173636/#review179282 > I'm not sure I'm the best person to review this, I don't understand the continuation code all that well. > > It seems like we are trying to avoid calculating if we need border-collapse if we're sure we won't need it, and making this unconditional would break that. > > Can we only do this if we're a continuation (aPrevInFlow != nullptr), and possibly check state on the original frame? Fixed. Could you review it again? Thanks.
Comment 13•7 years ago
|
||
> Can we only do this if we're a continuation (aPrevInFlow != nullptr), and possibly check state on the original frame? Hmm, I suspect that we can't actually. Looking at nsTableFrame::CalcHasBCBorders() it only iterates the row-groups of the current table frame, not next-in-flows, so copying the table first-in-flow state to its nif's doesn't seem correct to me. http://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/layout/tables/nsTableFrame.cpp#1459 You should be able to test that by making a table with two or more row-groups that fragments between first and second row-group. The table itself should *not* have a CSS border defined and neither should the first row-group, but the second (or later) row-group should have a border. In this case, CalcHasBCBorders() will be false for the table first-in-flow, but its next-in-flow should have that bit true since it has borders from the 2nd row-group to account for.
Assignee | ||
Comment 14•7 years ago
|
||
I didn't copy the result of CalcHasBCBorders(). I actually copy the dirty flag which is NeedToCalcBCBorders(). So this patch means if first-in-flow table need to call CalcHasBCBorders(), then we also set the dirty flag to its nif. Then, each nif also call CalcHasBCBorders() because the dirty flag is set. Does this make sense?
Assignee | ||
Updated•7 years ago
|
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8902167 [details] Bug 1394249 - SetNeedToCalcHasBCBorders to true when initialize nsTableFrame. https://reviewboard.mozilla.org/r/173636/#review182448 ::: layout/tables/nsTableFrame.cpp:192 (Diff revision 3) > // Set my isize, because all frames in a table flow are the same isize and > // code in nsTableWrapperFrame depends on this being set. > WritingMode wm = GetWritingMode(); > SetSize(LogicalSize(wm, aPrevInFlow->ISize(wm), BSize(wm))); > + > + if (aPrevInFlow->IsTableFrame()) { The continuation chain should all be of the same type so this check is redundant. Put it in a MOZ_ASSERT instead.
Comment 17•7 years ago
|
||
I still think there's something fishy about how this bit is copied from the prev-in-flow. Let's say we have a frame tree like this: table rg1 row1 rg2 row2 Let's assume only row2 has some borders, nothing else. Initially, 'table' has mNeedToCalcBCBorders true, so when HasBCBorders() is called we do CalcHasBCBorders() which sets mHasBCBorders true (based on row2) and then we do SetNeedToCalcBCBorders(false). Now, let's assume the parent of 'table' changes size causing it to fragment into: table (first-in-flow) rg1 row1 table (next-in-flow) rg2 row2 when the "table (next-in-flow)" is created, mNeedToCalcBCBorders is false and that value is copied in Init(), so CalcHasBCBorders() is never called. (also, mHasBCBorders in "first-in-flow" is now lying since that frame subtree has no borders) Am I missing something?
Assignee | ||
Comment 18•7 years ago
|
||
I think when table is split, in your example, we will remove rg2 and row2 from fif table. And the children change in the table will reset the mNeedToCalcBCBorders flag (https://searchfox.org/mozilla-central/search?q=symbol:_ZN12nsTableFrame15AddBCDamageAreaERKN7mozilla9TableAreaE&redirect=false). So we should always have dirty flag with true when table is split. Is this right?
Comment 19•7 years ago
|
||
I don't think splitting a frame uses the RemoveFrame/InsertFrame/AppendFrame methods (see nsTableFrame::PushChildren). We probably call SetInitialChildList on the table continuation though, even though the child list is probably empty at that point (I'd expect rg2 to be in the first-in-flow table frame's overflow list at that point). nsTableFrame::SetInitialChildList does call SetFullBCDamageArea() though, which calls SetNeedToCalcHasBCBorders(true). Hmm, so why is this patch needed then? Can you debug this a bit and see what's actually happens after we split the table? Setting a breakpoint in nsCSSFrameConstructor::CreateContinuingFrame is probably a good starting point.
Assignee | ||
Comment 20•7 years ago
|
||
nsTableFrame::SetInitialChildList() would call SetFullBCDamageArea() only if !PrevInFlow() (https://searchfox.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#345). So in the continuing table frame, we don't call SetFullBCDamageArea. Since the HasBCBorder state would change after continuing frames are constructed. How about we set the NeedToCalcHasBCBorder flag for current frame and prev-in-flow frame in the nsTableFrame::SetInitialChildList()?
Comment 21•7 years ago
|
||
> nsTableFrame::SetInitialChildList() would call SetFullBCDamageArea() only if !PrevInFlow() Ah, good point. I'd slightly prefer we set the bit to true in Init() instead, after this line (so we can use IsBorderCollapse()): https://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/layout/tables/nsTableFrame.cpp#175 (SetFullBCDamageArea() will do that again in SetInitialChildList() for the first-in-flow, but that doesn't seem like a big deal) WDYT?
Comment 22•7 years ago
|
||
... and to explain why I'd prefer that: setting NeedToCalcHasBCBorder to true doesn't depend on the child list, which is what SetInitialChildList is for.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 24•7 years ago
|
||
Comment on attachment 8902167 [details] Bug 1394249 - SetNeedToCalcHasBCBorders to true when initialize nsTableFrame. Looks good to me. Could you review the patch again? Thank you.
Updated•7 years ago
|
Assignee | ||
Comment 25•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=60c876b396d6499e56ea59befb5f508fd1b3f551
Comment 26•7 years ago
|
||
Pushed by mtseng@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/64fa48edf016 SetNeedToCalcHasBCBorders to true when initialize nsTableFrame. r=mats
Comment 27•7 years ago
|
||
Oh, I forgot - we really should add a reftest for this if possible. Morris, will you take care of that?
Comment 28•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/64fa48edf016
Assignee | ||
Comment 29•7 years ago
|
||
Sure, since this bug is resolved. I'll create a follow-up bug for adding reftest.
Comment 30•7 years ago
|
||
Last 2 nightly builds show the issue seems to be fixed for the test case in bug 1392028, but not for the sample page mentioned in bug 1396250 (right border on page 1 is missing).
Comment 31•7 years ago
|
||
> but not for the sample page mentioned in bug 1396250 (right border on page 1 is missing).
It looks fixed to me - I could reproduce it in an older build, but in
the latest Nightly I see all the borders (on Linux).
The borders have varying width on different pages though, some are 1px,
others are 2px. I think that's an (unrelated) older "rounding error" bug.
I think in some cases that bug manifests as missing borders (i.e. we
rounded to zero), on some platforms. So if the remaining issue for you
is that just the right border is missing then it's likely that older bug
you're seeing. (We have several bugs on file for that bug so no need to
file another one.)
BTW, thanks for testing Nightly! That's very appreciated, especially
for v57 which has a lot of changes.
Comment 32•7 years ago
|
||
I tested on Win7, 32-bit. I would like to say you’re right and believe you, but when testing the June 6 build, the current page 1 issue does not occur - both test cases display fine there. That made me believe this bug does not entirely fix the bug 1367747 regression, unless any existing "rounding error" bug started to suffer another regression meanwhile, or it affects the current fix (and only on page 1) where the previous code wasn’t affected at all.
Comment 33•7 years ago
|
||
OK, maybe Morris can have a look at the test in bug 1396250 and see what's up with the change in behavior there? BTW, the border rounding error bug is a bit volatile, sometimes just resizing the window will make the borders change size/disappear. It only affects border-collapse tables with row/column spans though, IIRC.
Comment 34•7 years ago
|
||
We should probably uplift this to v56 given the number of dupes filed. It's a low-risk patch IMO.
Comment 35•7 years ago
|
||
Might even be worth taking for v55 if we're doing a point-release there for some (other) reason.
Comment 37•7 years ago
|
||
Too late for 55, we're entering RC mode for 56. So please request uplift ASAP if you want this to make 56 rc1.
Updated•7 years ago
|
Comment 38•7 years ago
|
||
FWIW: in current Nightly, the missing right-border can also occur on page 2 instead of page 1 when changing zoom level in print preview. The same however can also happen on smaller than 100% views in pre-June 6 builds, though it seems to happen less often, and not in 100% view. Guess it’s the random rounding behavior after all. I cannot verify what happens on a real printer though, but would expect users want to see a matching preview in all cases and/or at least in 100% view. Is that included in the rounding bug(s)? And are you sure it only occurs for previews? Regardless and given the number of users suffering the bug according to sumo, uplifting for 56 will probably be appreciated.
Comment 40•7 years ago
|
||
Comment on attachment 8902167 [details] Bug 1394249 - SetNeedToCalcHasBCBorders to true when initialize nsTableFrame. Approval Request Comment [Feature/Bug causing the regression]: bug 1367747 [User impact if declined]: missing table borders in Print Preview/Print that affects a lot of users [Is this code covered by automated tests?]:no (bug 1399698 filed on adding it) [Has the fix been verified in Nightly?]:yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]:no [Is the change risky?]:no [Why is the change risky/not risky?]:trivial patch [String changes made/needed]:no
Updated•7 years ago
|
Comment 41•7 years ago
|
||
This is too late for the 56 release, without more time for testing/automated tests. Looks like it is fixed in 57 though.
Comment 42•7 years ago
|
||
Comment on attachment 8902167 [details] Bug 1394249 - SetNeedToCalcHasBCBorders to true when initialize nsTableFrame. RC 56 was build yesterday, we can't take this so late in the beta 56 cycle. Sorry!
Assignee | ||
Comment 43•7 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #33) > OK, maybe Morris can have a look at the test in bug 1396250 and see > what's up with the change in behavior there? > > BTW, the border rounding error bug is a bit volatile, sometimes just > resizing the window will make the borders change size/disappear. > It only affects border-collapse tables with row/column spans though, IIRC. I think my patch didn't change rounding behavior. Might be another issue.
Comment 48•6 years ago
|
||
This bug persists into v.58. I see that it has been marked as fixed in v.57, however I believe that to be false. Please advise.
Comment 49•6 years ago
|
||
(In reply to jbovey from comment #48) > This bug persists into v.58. There are numerous subtly different table printing bugs. You need to provide a link to a page where you encounter the problem so it can be identified as this bug or possibly a different bug.
Comment 50•6 years ago
|
||
Here's a test link: http://liveonlinemath.com/printing_issue.html If you print preview, then fiddle with various combinations of zoom/scale, you should see what I mean. For example... * 100%: Doesn't show the right border on pgs. 2 and 3, and the bottom border on pg. 3 is missing. * 104%: Missing the top border on pg. 1, and left and bottom borders on pgs. 2 and 3. There's also a missing border between problem #14 and #15. * 98%: Missing left and right borders on pg. 1, but pgs. 2 and 3 seem fine. * 98% with setting the width of the table to 95% with CSS: Missing left border on pg. 1 and missing right border on pgs. 2 and 3. * 102% with setting the width of the table to 95% with CSS: Everything is good except a missing border between problem #16 and #17. * 125%: Everything seems to be good here whether the CSS table width is 100% or 95%. As you can see, the results vary by table width and zoom/scale combination. Also, these results are relevant for this particular document, but getting the right combination varies by content. I'm running 58.0.1 as of this testing.
Comment 51•6 years ago
|
||
(In reply to jbovey from comment #50) > Here's a test link: http://liveonlinemath.com/printing_issue.html In the actual printed output (via print-to-PDF conversion), I get both left and right borders: https://www.jeffersonscher.com/temp/liveonlinemath.com_printing_issue.html.pdf https://www.jeffersonscher.com/temp/Fx58-table-borders-03-actual.png (viewed in Acrobat) > If you print preview, then fiddle with various combinations of zoom/scale, > you should see what I mean. For example... Yes, the Print Preview display sometimes does not display borders that will appear normally in actual output. It's especially noticeable when I am using a maximized window (which is most of the time): https://www.jeffersonscher.com/temp/Fx58-table-borders-01-maximized.png When using a resizable window, I see both left and right borders as they will print: https://www.jeffersonscher.com/temp/Fx58-table-borders-02-resizable.png Sounds like a different bug, not this one.
Comment 52•6 years ago
|
||
@jscher2000: It seems that resizing the window does have some effect. However, from what I've seen, the problem just changes based on the size of the window, as opposed to being fixed. Like, in full screen the right side is missing, but upon changing the window size now the left side is missing be the right side is there. The "end game" for my use case is that I use Bullzip .pdf printer to output .pdfs from print preview. I don't actually print (at least in this use case). Should I create a new bug report, or is there another thread for this somewhere? I've seen many bugs related to the borders spanning past the first page thing.
Comment 53•6 years ago
|
||
(In reply to jbovey from comment #52) > It seems that resizing the window does have some effect. However, from what > I've seen, the problem just changes based on the size of the window, as > opposed to being fixed. Like, in full screen the right side is missing, but > upon changing the window size now the left side is missing be the right side > is there. There is a problem with print preview, akin to a rounding error, where you can see the border flicker on and off as you slowly adjust the window width. > The "end game" for my use case is that I use Bullzip .pdf printer to output > .pdfs from print preview. I don't actually print (at least in this use > case). Should I create a new bug report, or is there another thread for > this somewhere? I've seen many bugs related to the borders spanning past > the first page thing. You need to view the PDF. When I print your test page, both left and right borders appear using three different PDF printer drivers: PDFCreator (free version), pdfFactory Pro, and Adobe PDF. If Bullzip doesn't get both borders into the PDF, that could be a different issue.
Comment 54•6 years ago
|
||
(In reply to jbovey from comment #48) > This bug persists into v.58. I see that it has been marked as fixed in > v.57, however I believe that to be false. Please advise. Please open another bug. This bug as originally reported was for a problem with: http://www.384thbombgroup.com/_content/_pages/PersonalWarRecord-Beta.php?xID7uGt4=75 That is fixed. Even if you find a problem with another page that has symptoms that seem similar the underlying issue in the code must be different.
Comment 55•6 years ago
|
||
(In reply to jscher2000 from comment #53) > (In reply to jbovey from comment #52) > > It seems that resizing the window does have some effect. However, from what > > I've seen, the problem just changes based on the size of the window, as > > opposed to being fixed. Like, in full screen the right side is missing, but > > upon changing the window size now the left side is missing be the right side > > is there. > > There is a problem with print preview, akin to a rounding error, where you > can see the border flicker on and off as you slowly adjust the window width. > > > The "end game" for my use case is that I use Bullzip .pdf printer to output > > .pdfs from print preview. I don't actually print (at least in this use > > case). Should I create a new bug report, or is there another thread for > > this somewhere? I've seen many bugs related to the borders spanning past > > the first page thing. > > You need to view the PDF. When I print your test page, both left and right > borders appear using three different PDF printer drivers: PDFCreator (free > version), pdfFactory Pro, and Adobe PDF. If Bullzip doesn't get both borders > into the PDF, that could be a different issue. You're right! It was just print preview messing up. Actually "printing" it with Bullzip worked and all borders showed properly on the outputted,pdf. Thanks for your help on this!
Comment 56•6 years ago
|
||
I tested this issue on Mac OS X 10.13, Windows 10 and Ubuntu 16.04 with the latest Nightly 63.0a1(2018-08-28) and I can reproduce this issue, but only the upper line of the table from pages 5-6-7 is different. Here is a print screen with the issue: https://imgur.com/a/J33pViR Please tell if I need to open another issue or it will be ok to reopen this bug?
Updated•6 years ago
|
Comment 57•6 years ago
|
||
I suspect that what you're seeing might be an already known issue with border rendering for 'border-collapse' tables. Search for "collapse" in the Layout:Tables component. Feel free to make a reduced testcase though and file a new bug if you think it's not covered already.
Comment 58•5 years ago
|
||
Tested on latest Nightly 66 on Windows 7 x64 and the borders are still not rendered in print preview. Used this reduced testcase from Bug 1392028 : https://bug1392028.bmoattachments.org/attachment.cgi?id=8906309
Should we re-open this issue or submit a new one specifically for Print Preview since the actual output is as expected?
Comment 59•5 years ago
|
||
Fwiw, that testcase works fine for me in Nightly on Linux.
I've reopened bug 1392028 to track the remaining issue on Windows, thanks Timea!
Comment 60•4 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #59)
Fwiw, that testcase works fine for me in Nightly on Linux.
I've reopened bug 1392028 to track the remaining issue on Windows, thanks Timea!
On Firefox 71.0 (64 bitų) on Ubuntu 18.04 the top borders are still not rendered after initial page in print preview (also on actual print). Used this reduced testcase from Bug 1392028 : https://bug1392028.bmoattachments.org/attachment.cgi?id=8906309
Description
•