Closed Bug 1394249 Opened 7 years ago Closed 7 years ago

Borders of long table do not print after initial page

Categories

(Core :: Printing: Output, defect)

55 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 - wontfix
firefox56 - wontfix
firefox57 --- fixed

People

(Reporter: f3red, Assigned: mtseng)

References

(Blocks 3 open bugs)

Details

(Keywords: regression)

Attachments

(3 files)

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
Blocks: 1367747
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Printing: Output
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
I have tried a number of css settings in this file, based on various internet searches, all with no effect.
Morris, can you take a look?
Flags: needinfo?(mtseng)
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Assignee: nobody → mtseng
Flags: needinfo?(mtseng)
I'd prefer if you put it in Init instead, since that's where we usually
do these kinds of initializations.
(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 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?
Blocks: 1396250
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.
> 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.
Flags: needinfo?(mtseng)
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?
Flags: needinfo?(mtseng) → needinfo?(mats)
Ah, that makes sense, thanks.
Flags: needinfo?(mats)
Attachment #8902167 - Flags: review?(matt.woodrow) → review?(mats)
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.
Attachment #8902167 - Flags: review?(mats) → review+
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?
Flags: needinfo?(mtseng)
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?
Flags: needinfo?(mtseng) → needinfo?(mats)
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.
Flags: needinfo?(mats) → needinfo?(mtseng)
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()?
Flags: needinfo?(mtseng) → needinfo?(mats)
> 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?
Flags: needinfo?(mats) → needinfo?(mtseng)
... 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 on attachment 8902167 [details]
Bug 1394249 - SetNeedToCalcHasBCBorders to true when initialize nsTableFrame.

Looks good to me. Could you review the patch again? Thank you.
Flags: needinfo?(mtseng)
Attachment #8902167 - Flags: review+ → review?(mats)
Attachment #8902167 - Flags: review?(mats) → review+
Pushed by mtseng@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/64fa48edf016
SetNeedToCalcHasBCBorders to true when initialize nsTableFrame. r=mats
Blocks: 1399282
Oh, I forgot - we really should add a reftest for this if possible.
Morris, will you take care of that?
Flags: needinfo?(mtseng)
Flags: in-testsuite?
https://hg.mozilla.org/mozilla-central/rev/64fa48edf016
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Sure, since this bug is resolved. I'll create a follow-up bug for adding reftest.
Flags: needinfo?(mtseng)
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).
Blocks: 1392028
> 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.
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.
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.
Flags: needinfo?(mtseng)
We should probably uplift this to v56 given the number of dupes filed.
It's a low-risk patch IMO.
Might even be worth taking for v55 if we're doing a point-release there
for some (other) reason.
No longer blocks: 1396250
Too late for 55, we're entering RC mode for 56.  So please request uplift ASAP if you want this to make 56 rc1.
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 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
Attachment #8902167 - Flags: approval-mozilla-beta?
Attachment #8902167 - Flags: approval-mozilla-beta? → approval-mozilla-release?
This is too late for the 56 release, without more time for testing/automated tests. Looks like it is fixed in 57 though.
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!
Attachment #8902167 - Flags: approval-mozilla-release? → approval-mozilla-release-
(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.
Flags: needinfo?(mtseng)
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.
(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.
Flags: needinfo?(jbovey)
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.
Flags: needinfo?(jbovey)
(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.
@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.
Flags: needinfo?(jscher2000)
(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.
(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.
(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!
Flags: needinfo?(jscher2000)
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?
Flags: needinfo?(mats)
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.
Flags: needinfo?(mats)

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?

Flags: needinfo?(mats)

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!

Flags: needinfo?(mats)

(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

You need to log in before you can comment on or make changes to this bug.