Closed Bug 963441 Opened 6 years ago Closed 6 years ago

web page is not completely printed, with "float:left" and "clear" and <table>

Categories

(Core :: Printing: Output, defect, P4)

defect

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: bug.tracker, Assigned: mats)

References

(Blocks 1 open bug, )

Details

(Keywords: dataloss, regression)

Attachments

(4 files, 1 obsolete file)

Attached image printpreview.png
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131205075310

Steps to reproduce:

I loaded https://en.comparis.ch/banken/vorsorge/saeule3a-vergleich.aspx , then I pressed 'Continue' (the form is sent with default data). (-> The table I get is longer than one (print) page)

Then print or print preview the page.


Actual results:

Two pages were print: The first with (incomplete) table data, the second with the page contens below the table.
The table data is cut on the bottom of page 1. The reminding data is not print.

See the attachement. The last table row ('Zürcher Kantonalbank Saving 3 Accounts') on the screen does not appears on the printout.

FF 24 ESR has the same bug.


Expected results:

The reminding table data should be printed on a second table. 

I think there is a problem with tables transferred via https. Tables via http seems to be printed out correctly.
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:29.0) Gecko/20100101 Firefox/29.0

Reproduced with latest Nightly 29 (Build ID: 20140128031716): the pages are not entirely displayed in print preview nor completely printed.
Component: Untriaged → Printing: Output
Product: Firefox → Core
Confirmed in Linux Nightly as well. (29.0a1 (2014-01-27))
Blocks: 521204
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: dataloss
OS: Windows 7 → All
Priority: -- → P4
Hardware: x86_64 → All
Version: 26 Branch → Trunk
Summary: web page is not completely printed → web page is not completely printed, with "float:left" and "clear" and <table>
Attached file testcase 1
Here's a reduced testcase.

(Pretty self-explanatory; view in print preview, scroll to bottom, check for the "IS THIS TEXT VISIBLE" text. It's missing for me.)

The bug goes away if I do any of the following...
 - remove the "float" styling
 - remove the "clear" styling
 - remove the table wrapper (so that only blocks are involved)
Found the following regression range: 

Last good revision: c8a1314aa449 (2012-12-17)
First bad revision: 2e70b718903a (2012-12-18)

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c8a1314aa449&tochange=2e70b718903a
Keywords: regression
Thanks!

Regression range on inbound (which turns out to be a strict subset of the changesets from comment 4's regression range):
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3e4e600adc3b&tochange=ff251b3e073f

I'm guessing it's a regression from this cset:
{
http://hg.mozilla.org/integration/mozilla-inbound/rev/1ac4491232e5
Bug 696640 - Don't put an existing next-in-flow for the kid into 'rowGroups' - it's either already there or it's not our child. r=roc
}
...since it looks like it was modifying the behavior of fragmented things inside tables. (The patch is touching nsTableFrame.cpp)
Blocks: 696640
Yup, confirmed (with targeted builds) that cset 1ac4491232e5 exhibits this bug, and its parent (9431f3821542) does not.

So, this was a regression from 1ac4491232e5 / Bug 696640.
Yeah, I'm debugging this at the moment... more in a bit.
Assignee: nobody → matspal
So we start [1] reflowing our table (blue), and find that its
row-group (green) is incomplete - so we create [2] a NIF for it (red),
insert that into 'rowGroups' [3] and then call PushChildren which
puts the entries into the table overflow list [4].

We then, for some unknown reason, decide to reflow the same table
again (instead of its NIF).  This time MoveOverflowToChildList
moves the row-group (red) back in our child list before starting
to reflow the children [5].  We again find that (green) is incomplete
but this time we already have a NIF for it[6], and it's in our child
list.  What's NOT expected though is that it's NOT in 'rowGroups' [7]
even though it's in our child list, so this time PushChildren
fails to put it on the overflow list...

The frame tree at the end is what we print, note the remaining
overflow list.

The reason for the unexpected 'rowGroups' state is this:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/tables/nsTableFrame.cpp&rev=3.505&root=/cvsroot#3170
OrderRowGroups explicitly ignores siblings that are also next-in-flows.
So if I make OrderRowGroups actually put all row-groups into 'rowGroups'
then it works.  Alternatively, if I deal with it like so:

  if (kidNextInFlow->GetParent() == this && !rowGroups.Contains(kidNextInFlow)) {
    // Oops 'kidNextInFlow' is one of our kids that's not in rowGroups,
    // make sure we push it.
    rowGroups.InsertElementAt(childX + 1,
      static_cast <nsTableRowGroupFrame*>(kidNextInFlow));
  }

then it also works as expected...  I'll sleep on it :-)
Attached patch fix+reftest (obsolete) — Splinter Review
We could change the "else if" in this patch to "if" and then
remove the "rowGroups.InsertElementAt" statement above it so
that would use this block too, but I think I prefer to separate
this block as an exception due to the way OrderRowGroups works.
What do you think?
Attachment #8367949 - Flags: review?(dholbert)
Comment on attachment 8367949 [details] [diff] [review]
fix+reftest

Brief nit on the reftest: Looks like the reference case uses a multi-page floated table (albeit one with two <tbody> elements).  That seems potentially fragile, as a means of testing this - I imagine (perhaps naively) that there would be ways for us to accidentally break both the reference case and testcase simultaneously.

Could we instead make the reference just use three divs, with no floating/clearing/tables? Say, a 3-in tall div, followed by a div with "IS THIS TEXT..", followed by the "clear" div (but without any 'clear' styling, since I don't think we'd need it.)


(Will look at the code parts soon)
(In reply to Mats Palmgren (:mats) from comment #9)
> So if I make OrderRowGroups actually put all row-groups into 'rowGroups'
> then it works.

Do you know why we can't just do that (i.e. why does OrderRowGroups need the NIF check)? Do NIFs end up throwing off the row numbering, or something?

In any case, the attached patch does look like a reasonable iteration, based on the patch that regressed this.  (I'm assuming that in bug 696640, the NIF was *not* the next sibling? Otherwise, it looks like this would be effectively reverting that bug's patch as far as that bug is concerned, and we'd be open to that bug again.)
Comment on attachment 8367949 [details] [diff] [review]
fix+reftest

>+++ b/layout/tables/nsTableFrame.cpp
>@@ -2957,16 +2957,23 @@ nsTableFrame::ReflowChildren(nsTableRefl
>           rowGroups.InsertElementAt(childX + 1,
>                       static_cast <nsTableRowGroupFrame*>(kidNextInFlow));
[...]
>+          rowGroups.InsertElementAt(childX + 1,
>+            static_cast <nsTableRowGroupFrame*>(kidNextInFlow));

Nit: Can you reindent your InsertElementAt() call to match the one in the contextual code above it?

(Or alternately, reindent the contextual code to match your new one)

Just bringing that up because they're doing the exact same thing and are only a few lines apart, so we might as well make them consistent.

r=me (though I'm still interested in the questions at the beginning & end of comment 13)
Attachment #8367949 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #13)
> Do you know why we can't just do that (i.e. why does OrderRowGroups need the
> NIF check)? Do NIFs end up throwing off the row numbering, or something?

Yes, I think Bernd's comment in bug 162691 comment 7 is correct.
nsContainerFrame::ReflowChild will destroy next-in-flows when
the reflow status is COMPLETE, so if we have those in 'rowGroups'
bad stuff could happen.  (having a local array with child frames
that you're going to reflow in the first place is *insane* of course)

> (I'm assuming that in bug 696640, the NIF
> was *not* the next sibling?

Yes, the NIF was not our child at all, it was a child of table's NIF IIRC.

BTW, how does flexbox deal with reordering frames?  does it reverse the
child list?
Attached patch fix+reftestSplinter Review
(In reply to Daniel Holbert [:dholbert] from comment #12)
> Brief nit on the reftest: Looks like the reference case uses a multi-page
> floated table (albeit one with two <tbody> elements).  That seems
> potentially fragile, as a means of testing this - I imagine (perhaps
> naively) that there would be ways for us to accidentally break both the
> reference case and testcase simultaneously.

That's a good point.  I've simplified the -ref as suggested.

Fixed the indentation and removed the space in "static_cast <".

https://tbpl.mozilla.org/?tree=Try&rev=419b3ebac7cf
Attachment #8367949 - Attachment is obsolete: true
Attachment #8368304 - Flags: review+
(In reply to Mats Palmgren (:mats) from comment #15)
> BTW, how does flexbox deal with reordering frames?  does it reverse the
> child list?

(I take it you're talking about the "order" property)

We sort the actual "mFrames" list, according to "order" (and secondarily by DOM position), yeah.
(In reply to Mats Palmgren (:mats) from comment #16)
> That's a good point.  I've simplified the -ref as suggested.

Thanks!

(Looks like it still has style="clear:left" on the final div, which is technically unnecessary (doesn't affect the rendering at all), but doesn't really matter I suppose.)
Oh, I just missed that, I'll remove it before landing.
https://hg.mozilla.org/mozilla-central/rev/b717389c3f94
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.