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

RESOLVED FIXED in mozilla12

Status

()

Core
Layout: Tables
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: dholbert, Assigned: Bernd)

Tracking

(Blocks: 1 bug, {dataloss})

Trunk
mozilla12
x86
Linux
dataloss
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Updated

8 years ago
Summary: Print-preview this testcase --> most of the table is missing → Table split across page is missing most of its contents in print-preview (with rowspan, large spacer div)
(Reporter)

Comment 1

8 years ago
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")
(Reporter)

Updated

8 years ago
Attachment #414603 - Attachment is patch: false
Attachment #414603 - Attachment mime type: text/plain → text/html
(Reporter)

Comment 2

8 years ago
Created attachment 414605 [details]
printout of testcase 1
(Reporter)

Updated

8 years ago
Attachment #414605 - Attachment is private: false
(Reporter)

Comment 3

8 years ago
Created attachment 414606 [details]
printout of testcase 2

Comment 4

8 years ago
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 ?
(Assignee)

Comment 5

6 years ago
Daniel is this still an issue after bug 642088 has landed?
(Reporter)

Comment 6

6 years ago
(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
(Reporter)

Comment 7

6 years ago
(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")
(Reporter)

Comment 8

6 years ago
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)
(Assignee)

Comment 9

6 years ago
with a custom scale I could reproduce the problem on testcase 1 with a letter format
(Assignee)

Comment 10

6 years ago
and my hope that 467444 would fix this does not hold
(Assignee)

Comment 11

6 years ago
Created attachment 579263 [details] [diff] [review]
patch
(Assignee)

Comment 12

6 years ago
https://tbpl.mozilla.org/?tree=Try&rev=fda92d9c3338
(Assignee)

Updated

6 years ago
Attachment #579263 - Flags: review?(dholbert+bmo)
(Assignee)

Comment 13

6 years ago
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
(Assignee)

Updated

6 years ago
Assignee: nobody → bernd_mozilla
(Reporter)

Comment 14

6 years ago
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. :) )
(Assignee)

Comment 15

6 years ago
>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.
(Reporter)

Comment 16

6 years ago
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?
(Reporter)

Comment 17

6 years ago
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.
(Assignee)

Comment 18

6 years ago
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.
(Reporter)

Comment 19

6 years ago
(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!
(Reporter)

Comment 20

6 years ago
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.
Attachment #579263 - Flags: review?(dholbert) → review-
(Assignee)

Comment 21

6 years ago
>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.
(Reporter)

Comment 22

6 years ago
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.
Attachment #580835 - Flags: review+
(Assignee)

Comment 23

6 years ago
I'll certainly do the one-liner, its only a c&p code snippet.
(Assignee)

Comment 24

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f2caed431a0
https://hg.mozilla.org/mozilla-central/rev/1f2caed431a0
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
(Assignee)

Updated

6 years ago
Flags: in-testsuite+
(Reporter)

Updated

6 years ago
Blocks: 692711
Duplicate of this bug: 692711
You need to log in before you can comment on or make changes to this bug.