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

Status: RESOLVED FIXED

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

Description

[Andi]

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

Comment 1

[Ada [:adalucinet]]

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.

Comment 2

[Daniel Holbert [:dholbert]]

Confirmed in Linux Nightly as well. (29.0a1 (2014-01-27))

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: 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)

Comment 4

[Ada [:adalucinet]]

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

Comment 5

[Daniel Holbert [:dholbert]]

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)

Comment 6

[Daniel Holbert [:dholbert]]

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.

Comment 7

[Mats Palmgren (inactive)]

Yeah, I'm debugging this at the moment... more in a bit.

Comment 8

[Mats Palmgren (inactive)]

Attachment: some frame trees from table reflow

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.

Comment 9

[Mats Palmgren (inactive)]

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 :-)

Comment 10

[Mats Palmgren (inactive)]

This is a good point:
https://bugzilla.mozilla.org/show_bug.cgi?id=162691#c7

Comment 11

[Mats Palmgren (inactive)]

Attachment: fix+reftest

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?

Comment 12

[Daniel Holbert [: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)

Comment 13

[Daniel Holbert [:dholbert]]

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

[Daniel Holbert [:dholbert]]

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)

Comment 15

[Mats Palmgren (inactive)]

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

Comment 16

[Mats Palmgren (inactive)]

Attachment: fix+reftest

(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

Comment 17

[Daniel Holbert [:dholbert]]

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

Comment 18

[Daniel Holbert [:dholbert]]

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

Comment 19

[Mats Palmgren (inactive)]

Oh, I just missed that, I'll remove it before landing.

Comment 20

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b717389c3f94

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b717389c3f94