<legend> doesn't get a legend frame when inside a display:contents parent

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

({testcase})

unspecified
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

NOTE: Firefox may crash if you try to do this before bug 1140160 is fixed

<fieldset><div style="display:contents"><legend>legend</legend></div></fieldset>

The legend above should display the same as if the <div> did not exist.
This is what prevents <legend> from getting a legend frame in this case.
We don't actually need to check that the parent *content* is a <fieldset>
here, since IsFrameForFieldSet already does the relevant test which
is that the parent *frame* we're inserting to is a fieldset frame
(or some pseudo frame thereof):
http://hg.mozilla.org/mozilla-central/annotate/7d85ac833cff/layout/base/nsCSSFrameConstructor.cpp#l3442

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0fc78fa1915
Attachment #8574217 - Flags: review?(roc)
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e4e3a618c9c
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
This is just backing out the fix for bug 497519, no?  Why is that OK, exactly?
Flags: needinfo?(mats)
And note that the comment in the block in question discussed this, but wasn't adjusted in this patch, so now it no longer reflects the code either....
(In reply to Not doing reviews right now from comment #3)
> This is just backing out the fix for bug 497519, no?  Why is that OK,
> exactly?

It reverted the 2nd part of bug 497519, yes.
I'm guessing that part addressed this assertion:

###!!! ASSERTION: Legends should not be positioned and should not float:
'!disp->IsAbsolutelyPositioned() && !disp->IsFloating()',
file layout/forms/nsLegendFrame.cpp, line 53

which still exists in the code:
http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsLegendFrame.cpp#16

I can't reproduce that (with the check removed) in any of the existing
tests, or the new ones in this patch, so I'm a bit puzzled how we
reached NS_NewLegendFrame given that we already had these checks at
that time (as you can see in the attached patch in bug 497519):
aStyleContext->StyleDisplay()->IsFloatingStyle()
aStyleContext->StyleDisplay()->IsAbsolutelyPositionedStyle()

So, it turns out those checks comes from bug 269908, fixed by @tn
around the same time.  But bug 497519 was filed *before* bug 269908
landed so I think the first few comments there are about the state
of the code pre-269908, i.e. when we didn't have those checks and
instead depended on a forms.css rule:

fieldset > legend {
  position: static ! important;
  float: none ! important;
  ...
}

(which I guess didn't match for that XBL test)

So, I believe that the 2nd part of bug 497519 was redundant by the
time it landed.  The float/abs.pos. checks (above) that bug 269908
added had already made it impossible to reach that assertion.

I've also reviewed the nsLegendFrame/nsFieldSetFrame classes and
everything matching "::legend" or "::fieldset" in the frame ctor
and it all looks fine to me, fwiw.

Does the updated code comment look OK to you?
Attachment #8574430 - Flags: feedback?(bzbarsky)
Comment on attachment 8574430 [details] [diff] [review]
Additional tests + code comment update

This looks great, thanks.  Also, thank you for digging into the history here!
Attachment #8574430 - Flags: feedback?(bzbarsky) → feedback+
No problem.

Additional tests and comment update landed:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=364c12f1edde
You need to log in before you can comment on or make changes to this bug.