1394249 - Borders of long table do not print after initial page

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Fred Preller]

Attachment: WWII Veteran Pilot War Record

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.

Comment 1

[YF (Yang)]

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3e1dea2784281f3fb022d2c96df48338afc825f5&tochange=4ab3d69a2cf09a0c888c8d3d6bc589118cb223d7

Comment 2

[Fred Preller]

Attachment: This is the stylesheet used by the page having this problem.

I have tried a number of css settings in this file, based on various internet searches, all with no effect.

Comment 3

[Mats Palmgren (inactive)]

Morris, can you take a look?

Comment 4

[Fred Preller]

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.

Comment 5

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Bug 1394249 - SetNeedToCalcHasBCBorders to true when initialize nsTableFrame.

In the printing preview, we create continuous table frame if table is too
long to containing in a page. But the default value of
NeedToCalcHasBCBorders is false which means we don't calculate
HasBCBorders for continuous table frame. Thus, the border collapse is
not shown when printing preview.

Review commit: https://reviewboard.mozilla.org/r/173636/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/173636/

Comment 6

[Mats Palmgren (inactive)]

I'd prefer if you put it in Init instead, since that's where we usually
do these kinds of initializations.

Comment 7

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(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 8

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8902167 [details]
Bug 1394249 - SetNeedToCalcHasBCBorders to true when initialize nsTableFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173636/diff/1-2/

Comment 9

[Matt Woodrow (:mattwoodrow)]

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 11

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8902167 [details]
Bug 1394249 - SetNeedToCalcHasBCBorders to true when initialize nsTableFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173636/diff/2-3/

Comment 12

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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

[Mats Palmgren (inactive)]

> 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.

Comment 14

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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?

Comment 15

[Mats Palmgren (inactive)]

Ah, that makes sense, thanks.

Comment 16

[Mats Palmgren (inactive)]

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

[Mats Palmgren (inactive)]

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?

Comment 18

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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

[Mats Palmgren (inactive)]

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.

Comment 20

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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

[Mats Palmgren (inactive)]

> 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

[Mats Palmgren (inactive)]

... 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 23

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8902167 [details]
Bug 1394249 - SetNeedToCalcHasBCBorders to true when initialize nsTableFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173636/diff/3-4/

Comment 24

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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.

Comment 25

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=60c876b396d6499e56ea59befb5f508fd1b3f551

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/64fa48edf016
SetNeedToCalcHasBCBorders to true when initialize nsTableFrame. r=mats

Comment 27

[Mats Palmgren (inactive)]

Oh, I forgot - we really should add a reftest for this if possible.
Morris, will you take care of that?

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/64fa48edf016

Comment 29

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Sure, since this bug is resolved. I'll create a follow-up bug for adding reftest.

Comment 30

[Ton [:Tonnes]]

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

[Mats Palmgren (inactive)]

> 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

[Ton [:Tonnes]]

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

[Mats Palmgren (inactive)]

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

[Mats Palmgren (inactive)]

We should probably uplift this to v56 given the number of dupes filed.
It's a low-risk patch IMO.

Comment 35

[Mats Palmgren (inactive)]

Might even be worth taking for v55 if we're doing a point-release there
for some (other) reason.

Comment 37

[Julien Cristau [:jcristau]]

Too late for 55, we're entering RC mode for 56.  So please request uplift ASAP if you want this to make 56 rc1.

Comment 38

[Ton [:Tonnes]]

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

[Mats Palmgren (inactive)]

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

Comment 41

[Liz Henry (:lizzard)]

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

[Liz Henry (:lizzard)]

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!

Comment 43

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(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

[jbovey]

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

[jscher2000]

(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

[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.

Comment 51

[jscher2000]

(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

[jbovey]

@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

[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.

Comment 54

[Jonathan Watt [:jwatt]]

(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

[jbovey]

(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

[ovidiu boca[:Ovidiu]]

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?

Comment 57

[Mats Palmgren (inactive)]

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

[Timea Cernea [:tbabos][inactive]]

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

[Mats Palmgren (inactive)]

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

[forkik]

(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