1339287 - [css-flexbox][css-grid] <legend> wraps children when display is set to flex or grid

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[eric]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 Safari/537.36

Steps to reproduce:

When using the <legend> element with it's display property set to "flex" it's children are wrapped when then would otherwise not be if using a <div> with the same style.

See: https://jsfiddle.net/ogg6jmdf/5/


Actual results:

The children of the <legend> element are wrapped where as the children of <div class="legend"> are displayed inline. 


Expected results:

The children of both elements should be displayed inline.

Comment 1

[Mats Palmgren (inactive)]

Attachment: Reporter's testcase

Comment 2

[Mats Palmgren (inactive)]

Attachment: Minimal testcase

Dumping the frame tree shows that we created a "Block(span)" frame.

Comment 3

[Mats Palmgren (inactive)]

The same problem also occurs for display:grid.

It seems we do blockification of the <legend> child boxes although we don't
actually support flex/grid layout of <legend> AFAICT.

We should probably add support for that, given that we support it for
<button> and <fieldset>.

In the near term though, we should try to remove the blockification
in cases where we ignore display:flex/grid (there are probably more
such cases).

Comment 4

[Mats Palmgren (inactive)]

Attachment: wip

Adding a AutoParentDisplayBasedStyleFixupSkipper in ProcessChildren
seems to fix it.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7eea2c78b18322d15feb7f300117542f54a09ea7

Comment 5

[Mats Palmgren (inactive)]

Attachment: fix

Comment 6

[Mats Palmgren (inactive)]

Attachment: reftest

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

Comment 7

[Mats Palmgren (inactive)]

BTW, I found bug 1169627 which is similar to this.  I think the fix here
should fix most of that bug too, but I'll leave that open for auditing
and writing tests for more elements later.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8837413 [details] [diff] [review]
fix

Review of attachment 8837413 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, with code comment added to clear up some potential confusion (see below). Thanks!

::: layout/base/nsCSSFrameConstructor.cpp
@@ +10786,4 @@
>    // The logic here needs to match the logic in GetFloatContainingBlock()
>    nsFrameConstructorSaveState floatSaveState;
> +  if (isFlexOrGridContainer ||
> +      ShouldSuppressFloatingOfDescendants(aFrame)) {

There's a comment here that says "The logic here needs to match the logic in GetFloatContainingBlock()".  Up until this patch, the logic matched. But now it does not (quite).  Initially this threw me & I thought maybe we'd need to consider updating GetFloatContainingBlock() to appease the comment.

But after a bit more digging, I get it -- ShouldSuppressFloatingOfDescendants (which we're already calling) internally calls IsFlexOrGridContainer(), and you're just trying to avoid having us redundantly call that function internally, since we'll now have already called it & can short-circuit before ShouldSuppressFloatingOfDescendants makes a redundant call. OK, cool.

Please add a code-comment to hint at that (because otherwise this superficially looks like a place where we forgot to update GetFloatContainingBlock, and/or it superficially looks like a place where we're redundantly checking the flex/grid status twice in the same "if" check.)


e.g. maybe something like this, just before the "if" check:
  // (Since we already have isFlexOrGridContainer, we check that eagerly instead
  // of letting ShouldSuppressFloatingOfDescendants look it up redundantly.)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/10d70527f1a0
Don't blockify 'display' in ApplyStyleFixups unless aFrame really is a flex/grid container frame, not just has display:flex/grid.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ef786064180
Reftest.

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/10d70527f1a0
https://hg.mozilla.org/mozilla-central/rev/5ef786064180