Closed Bug 476063 Opened 13 years ago Closed 13 years ago

[FIX]should construct <legend>s by display when they're not in a fieldset

Categories

(Core :: Layout: Form Controls, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dbaron, Assigned: bzbarsky)

References

Details

Attachments

(1 file, 1 obsolete file)

When <legend> elements are not in a fieldset, we should handle their frame construction based on the value of the 'display' CSS property rather than constructing an nsLegendFrame based on the tag name.  (I think HTML5 is using legend for other things, so this is reasonably important.)
Flags: wanted1.9.2?
Attached patch Like so? (obsolete) — Splinter Review
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #359693 - Flags: superreview?(dbaron)
Attachment #359693 - Flags: review?(dbaron)
Comment on attachment 359693 [details] [diff] [review]
Like so?

Actually, this might not handle dynamic insertion of legends in a fieldset correctly.  I should double-check that (and write some tests to make sure).
Attachment #359693 - Flags: superreview?(dbaron)
Attachment #359693 - Flags: superreview-
Attachment #359693 - Flags: review?(dbaron)
Attachment #359693 - Flags: review-
Attachment #359693 - Attachment is obsolete: true
Attachment #359695 - Flags: superreview?(dbaron)
Attachment #359695 - Flags: review?(dbaron)
The other option is to check our parent frame's content, I guess.
Summary: should construct <legend>s by display when they're not in a fieldset → [FIX]should construct <legend>s by display when they're not in a fieldset
Comment on attachment 359695 [details] [diff] [review]
Much better (dynamic insertion worked with the old patch, but _parsing_ works with this one!)

Might be good to have a comment explaining this check:
>+       (aParentFrame->GetType() != nsGkAtoms::fieldSetFrame &&
>+        aParentFrame->GetStyleContext()->GetPseudoType() !=
>+          nsCSSAnonBoxes::fieldsetContent))) {


I think checking the frame type is better; we'd want this check if we, in the future, supported changing what fieldsets look like using 'display'.  Then again, that doesn't match html.css...

r+sr=dbaron
Attachment #359695 - Flags: superreview?(dbaron)
Attachment #359695 - Flags: superreview+
Attachment #359695 - Flags: review?(dbaron)
Attachment #359695 - Flags: review+
Pushed http://hg.mozilla.org/mozilla-central/rev/e99cc373f724
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
It occurs to me... we should probably also have, in forms.css:
  legend { display: block }
so that we're still mostly compatible with what we used to do (and, according to Hixie, what HTML5 intends for legend).
Note that bug 430416 still hinders the HTML5 case.
Sure.  That's a pretty separate issue, though.  At least this fix allows constructing a fieldset-less DOM in which <legend> behaves sanely layout-wise via explicit DOM manipulation.

Agreed on the display thing; checked in http://hg.mozilla.org/mozilla-central/rev/65e8380bf4f9 with that change.
And I added a test for the display issue:
http://hg.mozilla.org/mozilla-central/rev/3381c874e76c
Depends on: 497519
You need to log in before you can comment on or make changes to this bug.