text overwrites table in moz-column when height is set

VERIFIED FIXED

Status

()

Core
Layout
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: Jason Harrop, Assigned: roc)

Tracking

({verified1.8.0.7, verified1.8.1})

Trunk
x86
Windows XP
verified1.8.0.7, verified1.8.1
Points:
---
Bug Flags:
blocking1.8.0.7 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

11 years ago
Created attachment 222820 [details]
test case
(Reporter)

Comment 2

11 years ago
Created attachment 222961 [details]
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).
Created attachment 224011 [details] [diff] [review]
Don't attempt to split a rowgroup unless we're in paginated mode
Assignee: nobody → roc
Status: UNCONFIRMED → ASSIGNED
Attachment #224011 - Flags: review?(bernd_mozilla)
Created attachment 224013 [details] [diff] [review]
better patch

Get rid of the conditional exit in SplitRowGroup, it won't be taken now
Attachment #224011 - Attachment is obsolete: true
Attachment #224013 - Flags: review?(bernd_mozilla)
Attachment #224011 - Flags: review?(bernd_mozilla)

Comment 5

11 years ago
   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.
There is only one caller to SplitRowGroup and the other part of my patch ensures that that caller never calls it with !IsPaginated(). Right?

Updated

11 years ago
Attachment #224013 - Flags: review?(bernd_mozilla) → review+
Attachment #224013 - Flags: superreview?(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?
Attachment #224013 - Flags: superreview?(bzbarsky) → superreview+
Alright, I moved the assertion up and made it a precondition :-)
checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
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.
Attachment #224013 - Flags: approval-branch-1.8.1?(bernd_mozilla)
Blocks: 331446
Apparently this fixed bug 331446 too, which is a potential security issue, so that's another reason to take this on branch.

Updated

11 years ago
Attachment #224013 - Flags: approval-branch-1.8.1?(bernd_mozilla) → approval1.8.1?

Updated

11 years ago
Attachment #224013 - Flags: approval1.8.1? → approval1.8.1+
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.
Attachment #224013 - Flags: approval1.8.0.6?
Checked into 1.8.1 branch.
Keywords: fixed1.8.1
Flags: blocking1.8.0.6?
Flags: blocking1.8.0.7? → blocking1.8.0.7+
Comment on attachment 224013 [details] [diff] [review]
better patch

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #224013 - Flags: approval1.8.0.7? → approval1.8.0.7+
checked into 1.8.0 branch
Keywords: fixed1.8.0.7
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
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.0.7, fixed1.8.1 → verified1.8.0.7, verified1.8.1

Comment 17

11 years ago
we learned from another bug (bug 362275) that the patch will not protect against splitting in print mode :-(
You need to log in before you can comment on or make changes to this bug.