Closed Bug 1230207 Opened 8 years ago Closed 8 years ago

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


(Core :: Layout, defect, P3)




Tracking Status
firefox46 --- fixed


(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)


(Blocks 1 open bug)


(Keywords: dev-doc-needed, testcase)


(2 files, 2 obsolete files)

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.
Attached patch wip (obsolete) — Splinter Review
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?
Assignee: nobody → mats
Attachment #8695965 - Flags: feedback?(bzbarsky)
Attached patch fix (obsolete) — Splinter Review
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?
Attachment #8695965 - Attachment is obsolete: true
Attachment #8695965 - Flags: feedback?(bzbarsky)
Attachment #8696051 - Flags: review?(bzbarsky)
Attached patch reftestsSplinter Review
Attachment #8696053 - Attachment description: refe → reftests
Attachment #8696053 - Attachment is patch: true
(I filed bug 1230672 for column layout, which is slightly more complicated.)
Blocks: 1230672
Comment on attachment 8696051 [details] [diff] [review]

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.
Attachment #8696051 - Flags: review?(bzbarsky) → review?(dholbert)
Comment on attachment 8696051 [details] [diff] [review]

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.
Attachment #8696051 - Flags: review?(dholbert) → review+
Attached patch fixSplinter Review
(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.
Attachment #8696051 - Attachment is obsolete: true
Attachment #8696249 - Flags: review?(bzbarsky)
Comment on attachment 8696249 [details] [diff] [review]

r=me.  This makes me much happier, thanks!  And sorry for the lag...
Attachment #8696249 - Flags: review?(bzbarsky) → review+
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Flags: in-testsuite+
Depends on: 1233135
Depends on: 1233191
Shouldn't also be marked as duplicate of this?
Blocks: css-grid-1
Is there a way for the legend of a fieldset to be taken in account by flexbox as well ?

Here is a test case :

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).
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">)
> 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 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 <> and in Safari due to <>.  And Edge seems to not really support fieldset being a flex container so far...
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.
FYI, Chrome doesn't support display:flex on <fieldset> yet.
You need to log in before you can comment on or make changes to this bug.