1230207 - [css-grid][css-flexbox] Implement grid/flex layout for <fieldset>

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Mats Palmgren (inactive)]

I don't see any reason not to support grid and flexbox layout for <fieldset>.

data:text/html,<fieldset style="display:flex">
data:text/html,<fieldset style="display:grid">

Dumping the frame tree for either of those results in:
FieldSet(fieldset) ... <
  Block(fieldset) ... <

If we make the inner frame there a nsGrid/FlexContainerFrame instead
it should just work I think.

Comment 1

[Mats Palmgren (inactive)]

Attachment: wip

This seems to work fine when I test it briefly with display:grid.
Also with display:grid;overflow:scroll.

Boris, do you see any problems from a frame construction POV?

Comment 2

[Mats Palmgren (inactive)]

Attachment: fix

I decided to keep 'display:block' on the pseudo frame rather than inherit it.
It seems safer to have nsGrid/FlexContainerFrame with mDisplay ==
NS_STYLE_DISPLAY_BLOCK, rather than having a nsBlockFrame with an arbitrary mDisplay.
I guess we could hack ApplyStyleFixups for this too, if necessary.

Daniel, do you see any problem with FlexContainerFrame having the "wrong" mDisplay?

Comment 3

[Mats Palmgren (inactive)]

Attachment: reftests

Comment 4

[Mats Palmgren (inactive)]

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

Comment 5

[Mats Palmgren (inactive)]

(I filed bug 1230672 for column layout, which is slightly more complicated.)

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

It really worries me to have these frames not matching their display types.

I would much rather we had a magic -moz-prefixed "display" value (restricted to UA sheets, if possible) that acted like "inherit" in some cases and "block" in others.

Or you could take the big hunk that already does different display fixeups based on aContext->GetPseudo() in nsRuleNode::ComputeDisplayData and add some more stuff to that, since you have a nice separate pseudo type here.

Apart from that, I assume the scrollframe handling of flex/grid is ok here?

In any case, someone who actually knows something about flex and grid should review the forms.css change.

Comment 7

[Daniel Holbert [:dholbert]]

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

The forms.css changes looks fine to me, assuming they're copypasted from the -moz-scrolled-content section of ua.css for flexbox, grid, & align. (looks like they are.)

Might be worth adding a comment in both instances of this copypasted list saying "Note: This should match the corresponding section in $FILE.css for $PSEUDO-ELEMENT", so that any changes to one of these instances will be more likely to be correctly copied over to the other instance.

r=me, anyway, for the forms.css stuff.  I assume there's still an implicit r?bz here on the actual C++ changes, pending answers to his questions in comment 6.

Comment 8

[Mats Palmgren (inactive)]

Attachment: fix

(In reply to Boris Zbarsky [:bz] from comment #6)
> Apart from that, I assume the scrollframe handling of flex/grid is ok here?

Yep, scrolling works fine.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

r=me.  This makes me much happier, thanks!  And sorry for the lag...

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/76a5cbbc0600
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c155345384e

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/76a5cbbc0600
https://hg.mozilla.org/mozilla-central/rev/3c155345384e

Comment 13

[Ilya]

Shouldn't https://bugzilla.mozilla.org/show_bug.cgi?id=1047590 also be marked as duplicate of this?

Comment 15

[mh_nichts]

Is there a way for the legend of a fieldset to be taken in account by flexbox as well ?

Here is a test case : https://codepen.io/mh-nichts/pen/VWLjjm

The fieldset has display:flex, and the divs inside behave accordingly (they are distributed evenly on the whole width), but the legend doesn't (it stays at the top of the fieldset, as a block element). 
It looks like flexbox is applied only to the elements after the legend (as if they were in a separate invisible container that has display:flex itself).

Comment 16

[Mats Palmgren (inactive)]

You mean the alignment of the <legend> should be derived from
the <fieldset>'s justify-content value?  That's an interesting idea!
As far as I know, there is nothing in the CSS specs about that.
(as a workaround you can do this: <legend align="center">)

Comment 17

[Boris Zbarsky [:bzbarsky]]

> as if they were in a separate invisible container that has display:flex itself

That is in fact exactly how it works in terms of the implementation.

In terms of what the spec says, it's defined at https://html.spec.whatwg.org/multipage/rendering.html#the-fieldset-and-legend-elements which certainly sounds like the placement of the rendered <legend> should not be affected by the display of the fieldset.

Note that if you make a <legend> not be the rendered legend by floating it, that might do what you want, because the flex container will then proceed to ignore the float style and just make it a flex item.  At least in Firefox; browser compat around this stuff is pretty poor.  For example, that technique will fail in Chrome due to <https://bugs.chromium.org/p/chromium/issues/detail?id=350505> and in Safari due to <https://bugs.webkit.org/show_bug.cgi?id=172999>.  And Edge seems to not really support fieldset being a flex container so far...

Comment 18

[mh_nichts]

Thank you, the float did it for Firefox !

Yes I observed the compatibility issues, surprisingly only IE10-11 is applying flexbox the way I expected it, but Edge and Chrome don't do anything at all for the fieldset elements.

Thinking more about it and after reading the spec, I agree with what you say : not sure there is something that should be implemented to force the legend to "flex" like the others, because of its special positioning by default.
I guess the float-workaround is also enough to adapt to the various needs.

Comment 19

[Mats Palmgren (inactive)]

FYI, Chrome doesn't support display:flex on <fieldset> yet.
https://bugs.chromium.org/p/chromium/issues/detail?id=375693