Closed
Bug 1230207
Opened 9 years ago
Closed 9 years ago
[css-grid][css-flexbox] Implement grid/flex layout for <fieldset>
Categories
(Core :: Layout, defect, P3)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-needed, testcase)
Attachments
(2 files, 2 obsolete files)
4.09 KB,
patch
|
Details | Diff | Splinter Review | |
8.73 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8696053 -
Attachment description: refe → reftests
Attachment #8696053 -
Attachment is patch: true
Assignee | ||
Comment 4•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f8e3c256b08
Assignee | ||
Comment 5•9 years ago
|
||
(I filed bug 1230672 for column layout, which is slightly more complicated.)
Blocks: 1230672
![]() |
||
Comment 6•9 years ago
|
||
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 7•9 years ago
|
||
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•9 years ago
|
||
(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 9•9 years ago
|
||
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 10•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/76a5cbbc0600 https://hg.mozilla.org/integration/mozilla-inbound/rev/3c155345384e
Comment 11•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/76a5cbbc0600 https://hg.mozilla.org/mozilla-central/rev/3c155345384e
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Assignee | ||
Updated•9 years ago
|
Flags: in-testsuite+
Comment 13•9 years ago
|
||
Shouldn't https://bugzilla.mozilla.org/show_bug.cgi?id=1047590 also be marked as duplicate of this?
Updated•9 years ago
|
Keywords: dev-doc-needed
Updated•8 years ago
|
Blocks: css-grid-1
Comment 15•7 years 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•7 years 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">)
![]() |
||
Comment 17•7 years ago
|
||
> 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•7 years 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•7 years 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.
Description
•