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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

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.

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix + adjustments to some tests added in bug 1140160

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

Comment 2

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e4e3a618c9c

Comment 3

[Boris Zbarsky [:bzbarsky]]

This is just backing out the fix for bug 497519, no?  Why is that OK, exactly?

Comment 4

[Boris Zbarsky [:bzbarsky]]

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....

Comment 5

[Mats Palmgren (inactive)]

Attachment: Additional tests + code comment update

(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?

Comment 6

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c70eb4faf0d

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a3053a836a09
https://hg.mozilla.org/mozilla-central/rev/8e4e3a618c9c

Comment 8

[Boris Zbarsky [: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!

Comment 9

[Mats Palmgren (inactive)]

No problem.

Additional tests and comment update landed:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=364c12f1edde

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/364c12f1edde