531200 - Table split across page is missing most of its contents in print-preview (with rowspan, large spacer div)

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase for Linux

STEPS TO REPRODUCE:
 - Print-preview testcase in Firefox. (should produce 2 pages)
 - Examine the bottom of the first page & top of the second page.

ACTUAL RESULTS: The top of the second page has cell "d", but the rest of the table (cells a, b, c) are completely missing.

NOTE: This testcase starts with an 8.59 inch-high spacer-div, which pushes the table to the exact right point with Ubuntu Linux's default "US Letter" page-size. On other platforms, the spacer-div's height may need some tweaking in order to get the table positioned correctly such that it reproduces the bug.  (You want to position it so that "d" ends up on the second page, but nothing else does.  As soon as you hit that threshold, it should reproduce the bug.)

NOTE: The testcase here is reduced from bug 513799's URL.  That bug's URL is pretty hairy (nested tables-within-tables, with colspans / rowspans up the wazoo). So I'm filing this separate bug for my reduced testcase, on the assumption that there could be additional problems (beyond this one) interacting to cause bug 513799.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 for Linux

This testcase just adds a border to the table, and tweaks the spacer div slightly.
This makes "a" and "b" **initially** visible, and the very top of "c" is visible.

Interestingly: if you scroll all the way down (so the first page isn't visible) and then *slowly* scroll upwards, the top three letters (a/b/c) aren't repainted!! But if at that point I alt-tab (or click Firefox's title bar), they repaint. (still showing only the top chunk of "c")

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: printout of testcase 1

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: printout of testcase 2

Comment 4

[Jean-Marc Desperrier]

Could not repro under Windows. My default is A4, but I tried to change to US letter without sucess. Do you believe this is Ubuntu/Linux specific ? 
Can you test under another OS ?

Comment 5

[Bernd]

Daniel is this still an issue after bug 642088 has landed?

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to comment #4)
> Could not repro under Windows.
[...]
> Do you believe this is Ubuntu/Linux specific ? 

No, I don't believe it to be Linux-specific. However, the testcase *is* specifically tailored to font-sizes on Ubuntu, as noted in this chunk of comment 0:
> NOTE: This testcase starts with an 8.59 inch-high spacer-div, which pushes
> the table to the exact right point with Ubuntu Linux's default "US Letter"
> page-size. On other platforms, the spacer-div's height may need some
> tweaking in order to get the table positioned correctly such that it
> reproduces the bug.  (You want to position it so that "d" ends up on the
> second page, but nothing else does.  As soon as you hit that threshold, it
> should reproduce the bug.)


(In reply to comment #5)
> Daniel is this still an issue after bug 642088 has landed?

I can still reproduce in latest nightly
Mozilla/5.0 (X11; Linux i686; rv:8.0a1) Gecko/20110730 Firefox/8.0a1

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to comment #6)
> No, I don't believe it to be Linux-specific. However, the testcase *is*
> specifically tailored to font-sizes on Ubuntu
(er, I meant "font sizes & margin amounts")

Comment 8

[Daniel Holbert [:dholbert]]

One caveat: with up-to-date nightlies in a fresh profile, the attached testcases don't reproduce the bug for me until I've "crystalized" my page-margin settings as described in bug 675457.  (since the attached testcases depend on a specific amount of space being available on the page)

Comment 9

[Bernd]

with a custom scale I could reproduce the problem on testcase 1 with a letter format

Comment 10

[Bernd]

and my hope that 467444 would fix this does not hold

Comment 11

[Bernd]

Attachment: patch

Comment 12

[Bernd]

https://tbpl.mozilla.org/?tree=Try&rev=fda92d9c3338

Comment 13

[Bernd]

patch context showing that we only reset the height if we do not split a spanning cell But as we know from bug 467444 the caller just tries do this.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableRowGroupFrame.cpp&rev=3.411&mark=992,996,1005,1010,1020#990

Comment 14

[Daniel Holbert [:dholbert]]

Sorry for delay on this -- have looked at it briefly, but I need to page in this chunk of code a little more thoroughly before I understand what's going on / the implications of the change. I'm aiming to have it reviewed in the next 24 hours.

(From applying the patch locally & running the included reftest, I can confirm that it fails without the code-change but passes with the code-change, so that's a good sign. :) )

Comment 15

[Bernd]

>it fails without the code-change but passes with the code-change, so that's a good sign. :) )
This is not a coincidence ;-). However the bonsai links demonstrate that the code is wrong since it was written in 2002 it took only 7 years to file the bug. The testcase makes it obvious that our code coverage for this section is weak.

Just for reference 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableRowGroupFrame.cpp&rev=3.411&mark=1237,1253,1267,1277#1231

The patch tries to make SplitSpanningCells more robust and to full fill the callers expectations.

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 579263 [details] [diff] [review]
patch

>+<title>a, b, and c are all missing when printed to US Letter in Ubuntu</title>

Drop (or tweak) this ^ from the testcase & reference case. (Looks like that's left over from the testcase attached to this bug -- but the 'a, b, and c' no longer exist in your reftest.)

>diff --git a/layout/reftests/bugs/531200-1-ref.html b/layout/reftests/bugs/531200-1-ref.html
>new file mode 100644
>--- /dev/null
>+++ b/layout/reftests/bugs/531200-1-ref.html
>@@ -0,0 +1,38 @@
>+    <tr style="page-break-after:always">
>+     <td>
>+       <img  src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABkAAAAZCAMAAADzN3VRAAAAGXRFWHRTb2Z0d2FyZQBBZG9iZSBJbWFnZVJlYWR5ccllPAAAAAZQTFRF/wAAAAAAQaMSAwAAABJJREFUeNpiYBgFo2AwAIAAAwACigABtnCV2AAAAABJRU5ErkJggg==">

>diff --git a/layout/reftests/bugs/531200-1.html b/layout/reftests/bugs/531200-1.html
>+    <tr>
>+     <td>
>+      <img  src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABkAAAAZCAMAAADzN3VRAAAAGXRFWHRTb2Z0d2FyZQBBZG9iZSBJbWFnZVJlYWR5ccllPAAAAAZQTFRF/wAAAAAAQaMSAwAAABJJREFUeNpiYBgFo2AwAIAAAwACigABtnCV2AAAAABJRU5ErkJggg==">

Extreme nit:
Looks like the testcase and reference case are nearly identical (which is great) except for (a) the |style="page-break-after:always"|, and (b) an extra space before the second <img> in the reference case (quoted above in reference case vs. testcase)

Could you drop the extra space in the reference case (or add a matching one in the testcase) so that the "page-break-after" thing is the only difference between the two?

Comment 17

[Daniel Holbert [:dholbert]]

So I still don't completely grok what's going on with the code (sorry).

One question: Previously, aDesiredHeight started at 0 and *only* grew from there (with NS_MAX(aDesiredHeight, ...) -- whereas now, we reset it (possibly to a smaller value!) on each row.  Is that bad?

And to clarify -- is the bug that we have rows with & without rowspanning cells, and we ignore the height of the non-rowspanning cells?

If you could add a comment above your added code, explaining why we want to set (and stomp on the existing value of...?) aDesiredHeight, that would definitely be helpful.

Comment 18

[Bernd]

Attachment: alternative proposal

>So I still don't completely grok what's going on with the code (sorry).

That is a bad sign. alternative approach, gives the same result and shows that the previous code fails if don't have a rowspan where we can split. 

And the testcase is fixed.

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to Bernd from comment #18)
> That is a bad sign. alternative approach, gives the same result and shows
> that the previous code fails if don't have a rowspan where we can split.

Yup, this alternative makes more sense to me.

Could you add a testcase where the previous patch fails but the alternative one succeeds?


> And the testcase is fixed.

Thanks!

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 579263 [details] [diff] [review]
patch

Just to be clear about next-steps here, I'm flagging r- on the first patch, since (per comment 18) it sounds like it fails in some cases, and the alternative patch makes more intuitive sense to me.

Comment 21

[Bernd]

>Could you add a testcase where the previous patch fails but the alternative one >succeeds?

This will need some time, and I am not confident that it is possible.

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 580835 [details] [diff] [review]
alternative proposal

OK -- I'm also open to just landing your alternative patch with the included testcase.  Probably better to have this fixed than to stall on potential bonus testcases.

One nit:
>+  if (!haveRowSpan) {
>+    nsRect rowRect = aLastRow.GetRect();
>+    aDesiredHeight = rowRect.YMost();
>+  }

I'm not sure the variable |rowRect| really helps here with clarity or anything.  It seems like you could just as easily make this assignment a one-liner like this:
>    aDesiredHeight = aLastRow.GetRect().YMost();

but r=me either way.

Comment 23

[Bernd]

I'll certainly do the one-liner, its only a c&p code snippet.

Comment 24

[Bernd]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f2caed431a0

Comment 25

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/1f2caed431a0