Closed Bug 404123 Opened 17 years ago Closed 16 years ago

[FIX]Percentage padding on <legend> triggers "ASSERTION: Bogus availSize.width. Should be bigger"

Categories

(Core :: Layout: Form Controls, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9beta2

People

(Reporter: jruderman, Assigned: bzbarsky)

References

Details

(Keywords: assertion, testcase)

Attachments

(3 files)

Attached file testcase
Loading the testcase gives me:

###!!! ASSERTION: Bogus availSize.width.  Should be bigger: 'availSize.width >= mLegendRect.width', file /Users/jruderman/trunk/mozilla/layout/forms/nsFieldSetFrame.cpp, line 533
Yeah, the fieldset code I wrote on reflow branch really doesn't handle percentage stuff all that well.  And this particular assertion is probably just wrong....
Attached patch Possible fixSplinter Review
This removes the unenforceable assertion, as well as a lot of the complexity, and the bogus possible manipulation of NS_UNCONSTRAINEDSIZE.

I'm also open to removing some of those !important rules this patch is adding, if we want to.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #290819 - Flags: superreview?(dbaron)
Attachment #290819 - Flags: review?(dbaron)
Summary: Percentage padding on <legend> triggers "ASSERTION: Bogus availSize.width. Should be bigger" → [FIX]Percentage padding on <legend> triggers "ASSERTION: Bogus availSize.width. Should be bigger"
OS: Mac OS X → All
Hardware: PC → All
Target Milestone: --- → mozilla1.9 M10
Blocks: 354502
No longer blocks: 364745
See also bug 402850.
Comment on attachment 290819 [details] [diff] [review]
Possible fix

In the forms.css changes, I think you should also add rules for
min-height and max-height.

Also, to be compatible with what you were doing before, I think you need 
to specify width:-moz-max-content, not width:auto.  Otherwise the
min-width computation for the fieldset will assume line breaking inside  
the legend.  Or was that change intentional?
(If you do this, fix the assertion in nsFieldSetFrame::Reflow to match.)

(Could you at least add a testcase for min-width computation of a
fieldset with a legend that has multiple words in it?)

r+sr=dbaron with that (although it's also ok if your change to allow the contents of the legend to wrap was intentional -- assuming you've tested that it works reasonably)
Attachment #290819 - Flags: superreview?(dbaron)
Attachment #290819 - Flags: superreview+
Attachment #290819 - Flags: review?(dbaron)
Attachment #290819 - Flags: review+
> I think you should also add rules for min-height and max-height.

Will do.

> Also, to be compatible with what you were doing before, I think you need 
> to specify width:-moz-max-content, not width:auto.  Otherwise the
> min-width computation for the fieldset will assume line breaking inside  
> the legend.  Or was that change intentional?

This was intentional.  The white-space:nowrap should keep the current behavior for normal cases, but if someone wants their legends to wrap, they can.  That fixes bug 354502.

> (Could you at least add a testcase for min-width computation of a
> fieldset with a legend that has multiple words in it?)

Yeah, I need to write some tests.  I did test this stuff at the time, but didn't seem to get those data: URIs into reftest format.  :(
David, see comment 5.
Drivers, this is a patch that makes sure that we don't do unconstrained math with fieldsets...  Risk should be low-ish.
Attachment #308982 - Flags: approval1.9?
Comment on attachment 308982 [details] [diff] [review]
Updated to comments

a1.9=beltzner
Attachment #308982 - Flags: approval1.9? → approval1.9+
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Depends on: 428423
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: