Closed Bug 403249 Opened 17 years ago Closed 17 years ago

[FIX]"ASSERTION: How did that happen??" with <col>, changing "span" attribute

Categories

(Core :: Layout: Tables, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta2

People

(Reporter: jruderman, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(3 files)

Attached file testcase (deleted) —
Loading the testcase triggers: ###!!! ASSERTION: How did that happen??: 'col->GetStyleContext() == colFrame->GetStyleContext() && col->GetContent() == colFrame->GetContent()', file /Users/jruderman/trunk/mozilla/layout/tables/nsTableColGroupFrame.cpp, line 326 This assertion was added recently, in bug 399209.
Attached patch Proposed fix (deleted) — Splinter Review
Good thing I added that assert! The frame model depends on the span attribute, but changing that attribute only triggered a reflow. Not good. The other change in this patch is to make the span style prop never be 0 or negative. Not sure whether that's correct; do we support span="0" in some way different from span="1"?
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #288234 - Flags: superreview?(roc)
Attachment #288234 - Flags: review?(bernd_mozilla)
We should get this fixed. Having the frame tree not match what we expect is very bad.
Severity: normal → major
Flags: blocking1.9?
OS: Mac OS X → All
Hardware: PC → All
Summary: "ASSERTION: How did that happen??" with <col>, changing "span" attribute → [FIX]"ASSERTION: How did that happen??" with <col>, changing "span" attribute
Target Milestone: --- → mozilla1.9 M10
Attachment #288234 - Flags: superreview?(roc) → superreview+
Plusing based on comment 2
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
>do we support span="0" in some way different from span="1"? There is no reason for this the HTML 4.01 spec says clearly: " span = number [CN] This attribute, whose value must be an integer > 0,.... "
Attachment #288234 - Flags: review?(bernd_mozilla) → review+
Checked in. We should get the test in once we have a place for assertion tests...
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Comment on attachment 289279 [details] [diff] [review] Some correctness tests (checked in; still need assertion test) Checked in the correctness tests. Still need to do the assertion test.
Attachment #289279 - Attachment description: Some correctness tests (still need assertion test) → Some correctness tests (checked in; still need assertion test)
Added a visibility:collapse test too.
verified fixed using debug build Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007120520 Minefield/3.0b2pre - no assertion on testcase -> Verified
Status: RESOLVED → VERIFIED
I checked in my original testcase as a crashtest. (bz, I assume that's what you wanted.)
Flags: in-testsuite? → in-testsuite+
Yes, that sounds great.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: