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

RESOLVED FIXED in Firefox 46

Status

()

Core
Layout
P3
normal
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: mats, Assigned: mats)

Tracking

(Blocks: 1 bug, {dev-doc-needed, testcase})

unspecified
mozilla46
dev-doc-needed, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8695965 [details] [diff] [review]
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?
Assignee: nobody → mats
Attachment #8695965 - Flags: feedback?(bzbarsky)
(Assignee)

Comment 2

3 years ago
Created attachment 8696051 [details] [diff] [review]
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?
Attachment #8695965 - Attachment is obsolete: true
Attachment #8695965 - Flags: feedback?(bzbarsky)
Attachment #8696051 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

3 years ago
Created attachment 8696053 [details] [diff] [review]
reftests
(Assignee)

Updated

3 years ago
Attachment #8696053 - Attachment description: refe → reftests
Attachment #8696053 - Attachment is patch: true
(Assignee)

Comment 5

3 years ago
(I filed bug 1230672 for column layout, which is slightly more complicated.)
Blocks: 1230672
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.
Attachment #8696051 - Flags: review?(bzbarsky) → review?(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.
Attachment #8696051 - Flags: review?(dholbert) → review+
(Assignee)

Comment 8

3 years ago
Created attachment 8696249 [details] [diff] [review]
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.
Attachment #8696051 - Attachment is obsolete: true
Attachment #8696249 - Flags: review?(bzbarsky)
Comment on attachment 8696249 [details] [diff] [review]
fix

r=me.  This makes me much happier, thanks!  And sorry for the lag...
Attachment #8696249 - Flags: review?(bzbarsky) → review+

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/76a5cbbc0600
https://hg.mozilla.org/mozilla-central/rev/3c155345384e
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
(Assignee)

Updated

3 years ago
Flags: in-testsuite+

Updated

3 years ago
Depends on: 1233135
(Assignee)

Updated

3 years ago
Depends on: 1233191

Comment 13

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

Updated

2 years ago
Duplicate of this bug: 1047590
Keywords: dev-doc-needed

Updated

2 years ago
Blocks: 1312610

Comment 15

a year ago
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).
(Assignee)

Comment 16

a year ago
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 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

a year ago
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.
(Assignee)

Comment 19

a year ago
FYI, Chrome doesn't support display:flex on <fieldset> yet.
https://bugs.chromium.org/p/chromium/issues/detail?id=375693
You need to log in before you can comment on or make changes to this bug.