Last Comment Bug 531200 - Table split across page is missing most of its contents in print-preview (with rowspan, large spacer div)
: Table split across page is missing most of its contents in print-preview (wit...
Status: RESOLVED FIXED
: dataloss
Product: Core
Classification: Components
Component: Layout: Tables (show other bugs)
: Trunk
: x86 Linux
: -- normal with 1 vote (vote)
: mozilla12
Assigned To: Bernd
:
:
Mentors:
: 692711 (view as bug list)
Depends on:
Blocks: 521204 513799 692711
  Show dependency treegraph
 
Reported: 2009-11-25 16:44 PST by Daniel Holbert [:dholbert]
Modified: 2011-12-30 19:29 PST (History)
6 users (show)
bernd_mozilla: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase for Linux (406 bytes, text/html)
2009-11-25 16:44 PST, Daniel Holbert [:dholbert]
no flags Details
testcase 2 for Linux (427 bytes, text/html)
2009-11-25 16:57 PST, Daniel Holbert [:dholbert]
no flags Details
printout of testcase 1 (12.39 KB, application/x-octet-stream)
2009-11-25 16:58 PST, Daniel Holbert [:dholbert]
no flags Details
printout of testcase 2 (12.54 KB, application/x-octet-stream)
2009-11-25 16:59 PST, Daniel Holbert [:dholbert]
no flags Details
patch (4.48 KB, patch)
2011-12-06 01:26 PST, Bernd
dholbert: review-
Details | Diff | Splinter Review
alternative proposal (5.19 KB, patch)
2011-12-11 23:27 PST, Bernd
dholbert: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2009-11-25 16:44:06 PST
Created attachment 414598 [details]
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] 2009-11-25 16:57:02 PST
Created attachment 414603 [details]
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] 2009-11-25 16:58:41 PST
Created attachment 414605 [details]
printout of testcase 1
Comment 3 Daniel Holbert [:dholbert] 2009-11-25 16:59:05 PST
Created attachment 414606 [details]
printout of testcase 2
Comment 4 Jean-Marc Desperrier 2010-02-11 16:38:08 PST
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 2011-07-30 02:57:30 PDT
Daniel is this still an issue after bug 642088 has landed?
Comment 6 Daniel Holbert [:dholbert] 2011-07-30 12:52:30 PDT
(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] 2011-07-30 12:54:10 PDT
(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] 2011-07-30 13:30:13 PDT
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 2011-08-23 11:59:41 PDT
with a custom scale I could reproduce the problem on testcase 1 with a letter format
Comment 10 Bernd 2011-12-02 08:24:34 PST
and my hope that 467444 would fix this does not hold
Comment 11 Bernd 2011-12-06 01:26:39 PST
Created attachment 579263 [details] [diff] [review]
patch
Comment 13 Bernd 2011-12-07 10:20:21 PST
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] 2011-12-09 18:17:50 PST
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 2011-12-10 01:43:42 PST
>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] 2011-12-11 16:53:02 PST
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] 2011-12-11 17:07:21 PST
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 2011-12-11 23:27:35 PST
Created attachment 580835 [details] [diff] [review]
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] 2011-12-12 02:19:27 PST
(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] 2011-12-13 12:02:04 PST
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 2011-12-17 03:41:49 PST
>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] 2011-12-18 16:41:56 PST
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 2011-12-18 22:31:42 PST
I'll certainly do the one-liner, its only a c&p code snippet.
Comment 25 Matt Brubeck (:mbrubeck) 2011-12-27 11:22:27 PST
https://hg.mozilla.org/mozilla-central/rev/1f2caed431a0
Comment 26 Daniel Holbert [:dholbert] 2011-12-30 19:29:16 PST
*** Bug 692711 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.