Closed Bug 1339287 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Layout, defect)

51 Branch
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: eric, Assigned: mats)

References

(Depends on 2 open bugs)

Details

(Keywords: testcase)

Attachments

(4 files, 1 obsolete file)

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.
Attached file Reporter's testcase
Attached file Minimal testcase
Dumping the frame tree shows that we created a "Block(span)" frame.
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).
Severity: normal → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
OS: Unspecified → All
Hardware: Unspecified → All
Summary: [CSS3-flex] legend wraps children when display is set to flex even if flex-direction is row → [css-flexbox][css-grid] <legend> wraps children when display is set to flex or grid
Attached patch wip (obsolete) — Splinter Review
Adding a AutoParentDisplayBasedStyleFixupSkipper in ProcessChildren
seems to fix it.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7eea2c78b18322d15feb7f300117542f54a09ea7
Assignee: nobody → mats
Attached patch fixSplinter Review
Attachment #8837050 - Attachment is obsolete: true
Attachment #8837413 - Flags: review?(dholbert)
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.
Blocks: 1169627
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.)
Attachment #8837413 - Flags: review?(dholbert) → review+
Pushed by mpalmgren@mozilla.com:
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.
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/10d70527f1a0
https://hg.mozilla.org/mozilla-central/rev/5ef786064180
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1341310
Depends on: 1370718
You need to log in before you can comment on or make changes to this bug.