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.

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

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

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)

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.

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

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

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

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

(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

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