338770 - text overwrites table in moz-column when height is set

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Jason Harrop]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3

In Firefox 1.5.0.3 handling of -moz-column-count, the paragraphs following a table may write over the top of the table if "height" is set on the surrounding div.

See the attached test case (derived from Wikipedia's page on Poland).

If the height property is removed, or table rows deleted, the page displays as expected.

Reproducible: Always

Steps to Reproduce:
Open the attached test case. 

It reproduces for me irrespective of the size of the browser window.
Actual Results:  
Paragraphs write over the top of the table.

Expected Results:  
Layed out the table, then the P tags after it, flowing into next column.

Comment 1

[Jason Harrop]

Attachment: test case

Comment 2

[Jason Harrop]

Attachment: test case 2

in this example, the second paragraph in column 2 overwrites the first paragraph.  i'm presuming this has the same underlying cause, even though the problem shows up at the top of column 2 this time (compare column 1 in the first test case).

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Don't attempt to split a rowgroup unless we're in paginated mode

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: better patch

Get rid of the conditional exit in SplitRowGroup, it won't be taken now

Comment 5

[Bernd]

   NS_ASSERTION(aPresContext->IsPaginated(), "SplitRowGroup currently supports only paged media"); 
-  if (!aPresContext->IsPaginated())
-    return  NS_ERROR_NOT_IMPLEMENTED;
+

Robert didn't you ask me just to keep this in code? Why should we assert and not exit?

I put this line to get rid off all invalid callers caused by mangleme and its siblings. So before this happens, we should ask  bclary to run his tests on this patch. (The predicted outcome will be a couple of new crashes due to the OrderRowGroup blues).

Btw, the elimination of OrderRowGroup is currently P1 for me.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

There is only one caller to SplitRowGroup and the other part of my patch ensures that that caller never calls it with !IsPaginated(). Right?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 224013 [details] [diff] [review]
better patch

sr=bzbarsky, but add:

  NS_PRECONDITION(aPresContext->IsPaginated(), "Don't know how to split across columns");

at the beginning of SplitRowGroup?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Alright, I moved the assertion up and made it a precondition :-)

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 224013 [details] [diff] [review]
better patch

I'd like to have this in FF2 since it's a very simple fix to a nasty columns bug.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Apparently this fixed bug 331446 too, which is a potential security issue, so that's another reason to take this on branch.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 224013 [details] [diff] [review]
better patch

This should go into 1.8.0 because it fixed an important crash in bug 331446.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked into 1.8.1 branch.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 224013 [details] [diff] [review]
better patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked into 1.8.0 branch

Comment 16

[alice nodelman [:alice] [:anode]]

https://bugzilla.mozilla.org/attachment.cgi?id=222961&action=view should appear without overlapping columns

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7pre) Gecko/20060821 Firefox/1.5.0.7pre

verified 1.8.0.7

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1b2) Gecko/2006082203 BonEcho/2.0b2

verified 1.8.1b2

Comment 17

[Bernd]

we learned from another bug (bug 362275) that the patch will not protect against splitting in print mode :-(