Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2832503003:
[css-flex] Implement the space-evenly content-distribution value (Closed)
|
Created:
3 years, 8 months ago by jfernandez Modified:
3 years, 7 months ago CC:
chromium-reviews, cbiesinger, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, blink-reviews, eae Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[css-flex] Implement the space-evenly content-distribution value
The CSS Box Alignment specification defines the <content-distribution>
set as some of the allowed values for the Content Distribution
properties, align-content and justify-content. The 'space-evenly' value
is not among the ones allowed for these properties according to the CSS
Flexible Box specification.
The CSS Flexbible box specification states that it must follow the CSS
Box Alignment specification, so this new value must be considered as
part of an upgraded level of the spec, which should be implemented
eventually.
Since we have already shipped an implementation of the new CSS Box
Alignment values for CSS Grid Layout, we need to implement it for
Flexbox as well.
This is the intent-to-implement-and-ship request approved for this
change:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/_4wD-NXS6Ik/W51KNjTzAAAJ
BUG=708121
Review-Url: https://codereview.chromium.org/2832503003
Cr-Commit-Position: refs/heads/master@{#474237}
Committed: https://chromium.googlesource.com/chromium/src/+/69568ed51b77ef4a94d96f99134e09223acf6223
Patch Set 1 #
Total comments: 2
Patch Set 2 : Unskip an external WPT for the space-evenly value. #Patch Set 3 : Patch rebased #Patch Set 4 : Fixed build errors. #Patch Set 5 : Added space-evenly to the css-properties expectations. #Patch Set 6 : Fixed layout tests. #Messages
Total messages: 51 (23 generated)
Description was changed from ========== [css-flex] Implement the space-evenly content-distribution value The CSS Box Alignment specification defines the <content-distribution> set as some of the allowed values for the Content Distribution properties, align-content and justify-content. The 'space-evenly' value is not among the ones allowed for these properties according to the CSS Flexible Box specification. The CSS Flexbible box specification states that it must follow the CSS Box Alignment specification, so this new value must be considered as part of a upgraded level od the spec, which should be implemented eventually. Since we have already shipped an implementation of the new CSS Box Alignment values for CSS Grid Layout, we need to implement it for Flexbox as well. BUG=708121 ========== to ========== [css-flex] Implement the space-evenly content-distribution value The CSS Box Alignment specification defines the <content-distribution> set as some of the allowed values for the Content Distribution properties, align-content and justify-content. The 'space-evenly' value is not among the ones allowed for these properties according to the CSS Flexible Box specification. The CSS Flexbible box specification states that it must follow the CSS Box Alignment specification, so this new value must be considered as part of an upgraded level of the spec, which should be implemented eventually. Since we have already shipped an implementation of the new CSS Box Alignment values for CSS Grid Layout, we need to implement it for Flexbox as well. BUG=708121 ==========
jfernandez@igalia.com changed reviewers: + cbiesinger@chromium.org, rego@igalia.com, svillar@igalia.com
Patch ready for review.
Just a quick comment as I feel @cbiesinger should be the one reviewing this. Please remove the following line from TestExpectations: crbug.com/708121 external/wpt/css/css-align-3/distribution-values/space-evenly-001.html [ Failure ] https://codereview.chromium.org/2832503003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right): https://codereview.chromium.org/2832503003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1522: // Fallback to 'center' Supernit: End comment with dot. https://codereview.chromium.org/2832503003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1979: // Fallback to 'center' Ditto.
Thanks, lgtm. This is fine to submit but I'll note that this still only partially implements the css-align keywords. eae -- does this need an intent to ship?
On 2017/04/20 21:38:38, cbiesinger wrote: > Thanks, lgtm. This is fine to submit but I'll note that this still only > partially implements the css-align keywords. > Yes, the bug reported talks only about Content Distribution properties, so I decided to start with that. I'll file new bugs for the Self and Default alignment properties.
I think we should be added 'space-evenly' to the Devtools.
On 2017/04/20 21:47:58, jfernandez wrote:
> On 2017/04/20 21:38:38, cbiesinger wrote:
> > Thanks, lgtm. This is fine to submit but I'll note that this still only
> > partially implements the css-align keywords.
> >
>
> Yes, the bug reported talks only about Content Distribution properties,
> so I decided to start with that.
I guess @cbiesinger is talking about the other values from the spec:
Value: normal | <baseline-position> | <content-distribution> ||
[ <overflow-position>? && <content-position> ]
Is all that supported in both Grid and Flexbox?
On 2017/04/21 03:06:51, Yiorsi wrote:
> I think we should be added 'space-evenly' to the Devtools.
I guess we'd need 2 things here:
1) A PR to add the new values (and maybe the place-XXX shorthands)
for CodeMirror (https://github.com/codemirror/CodeMirror).
2) A CL to update Source/devtools/front_end/sdk/CSSMetadata.js
@Yiorsi, am I missing something else? Thanks.
On 2017/04/21 04:52:20, Manuel Rego wrote: > On 2017/04/20 21:47:58, jfernandez wrote: > > On 2017/04/20 21:38:38, cbiesinger wrote: > > > Thanks, lgtm. This is fine to submit but I'll note that this still only > > > partially implements the css-align keywords. > > > > > > > Yes, the bug reported talks only about Content Distribution properties, > > so I decided to start with that. > > I guess @cbiesinger is talking about the other values from the spec: > Value: normal | <baseline-position> | <content-distribution> || > [ <overflow-position>? && <content-position> ] > > Is all that supported in both Grid and Flexbox? > > > On 2017/04/21 03:06:51, Yiorsi wrote: > > I think we should be added 'space-evenly' to the Devtools. > > I guess we'd need 2 things here: > 1) A PR to add the new values (and maybe the place-XXX shorthands) > for CodeMirror (https://github.com/codemirror/CodeMirror). > 2) A CL to update Source/devtools/front_end/sdk/CSSMetadata.js > > @Yiorsi, am I missing something else? Thanks. It's gonna be great.There is still a lack of place-* values in CSSMetadata.js.
On Fri, Apr 21, 2017 at 6:38 AM, <cbiesinger@chromium.org> wrote: > Thanks, lgtm. This is fine to submit but I'll note that this still only > partially implements the css-align keywords. > > > eae -- does this need an intent to ship? Yes, I believe it does. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Fri, Apr 21, 2017 at 6:38 AM, <cbiesinger@chromium.org> wrote: > Thanks, lgtm. This is fine to submit but I'll note that this still only > partially implements the css-align keywords. > > > eae -- does this need an intent to ship? Yes, I believe it does. -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2017/04/21 04:52:20, Manuel Rego wrote: > On 2017/04/20 21:47:58, jfernandez wrote: > > On 2017/04/20 21:38:38, cbiesinger wrote: > > > Thanks, lgtm. This is fine to submit but I'll note that this still only > > > partially implements the css-align keywords. > > > > > > > Yes, the bug reported talks only about Content Distribution properties, > > so I decided to start with that. > > I guess @cbiesinger is talking about the other values from the spec: > Value: normal | <baseline-position> | <content-distribution> || > [ <overflow-position>? && <content-position> ] > > Is all that supported in both Grid and Flexbox? > > Yes, you're right. We will need to implement the new <content-position> values for align-content and justify-content. Grid doesn't implement baseline-position for the Content Distribution properties, so no need to do anything in Flex on this regard. We can have something to do with overflow-alignment, but it shouldn't be too hard. > On 2017/04/21 03:06:51, Yiorsi wrote: > > I think we should be added 'space-evenly' to the Devtools. > > I guess we'd need 2 things here: > 1) A PR to add the new values (and maybe the place-XXX shorthands) > for CodeMirror (https://github.com/codemirror/CodeMirror). > 2) A CL to update Source/devtools/front_end/sdk/CSSMetadata.js > > @Yiorsi, am I missing something else? Thanks.
On 2017/04/21 06:34:29, eae wrote: > On Fri, Apr 21, 2017 at 6:38 AM, <mailto:cbiesinger@chromium.org> wrote: > > Thanks, lgtm. This is fine to submit but I'll note that this still only > > partially implements the css-align keywords. > > > > > > eae -- does this need an intent to ship? > > Yes, I believe it does. > I'll send the request then.
On 2017/04/21 06:25:04, Yiorsi wrote: > On 2017/04/21 04:52:20, Manuel Rego wrote: > > I guess we'd need 2 things here: > > 1) A PR to add the new values (and maybe the place-XXX shorthands) > > for CodeMirror (https://github.com/codemirror/CodeMirror). > > 2) A CL to update Source/devtools/front_end/sdk/CSSMetadata.js > > > > @Yiorsi, am I missing something else? Thanks. > > It's gonna be great.There is still a lack of place-* values in CSSMetadata.js. Actually I've found today CSSMetadata.js and I see it's missing a lot of things about CSS Grid Layout. I used to send PRs to CodeMirror to update the grid properties, but it seems I was missing to update things in CSSMetadata.js too. We should fix this http://crbug.com/714069
I'm still thinking about the intent-to-ship request. Perhaps it should be better to send a request for all the missing values. We had this chromestatus entry for the whole Alignment feature: https://www.chromestatus.com/features/6173208034148352 Firefox has an entry per layout model https://platform-status.mozilla.org/#css-box-alignment-block Since we have already shipped the feature for grid, I guess we can create a new one for Flexbox.
Description was changed from ========== [css-flex] Implement the space-evenly content-distribution value The CSS Box Alignment specification defines the <content-distribution> set as some of the allowed values for the Content Distribution properties, align-content and justify-content. The 'space-evenly' value is not among the ones allowed for these properties according to the CSS Flexible Box specification. The CSS Flexbible box specification states that it must follow the CSS Box Alignment specification, so this new value must be considered as part of an upgraded level of the spec, which should be implemented eventually. Since we have already shipped an implementation of the new CSS Box Alignment values for CSS Grid Layout, we need to implement it for Flexbox as well. BUG=708121 ========== to ========== [css-flex] Implement the space-evenly content-distribution value The CSS Box Alignment specification defines the <content-distribution> set as some of the allowed values for the Content Distribution properties, align-content and justify-content. The 'space-evenly' value is not among the ones allowed for these properties according to the CSS Flexible Box specification. The CSS Flexbible box specification states that it must follow the CSS Box Alignment specification, so this new value must be considered as part of an upgraded level of the spec, which should be implemented eventually. Since we have already shipped an implementation of the new CSS Box Alignment values for CSS Grid Layout, we need to implement it for Flexbox as well. This is the intent-to-implement-and-ship request approved for this change: https://groups.google.com/a/chromium.org/d/msg/blink-dev/_4wD-NXS6Ik/W51KNjTz... BUG=708121 ==========
jfernandez@igalia.com changed reviewers: + chrishtr@chromium.org, rbyers@chromium.org, tkent@chromium.org
I'd need an API owner to land this change.
On 2017/05/23 11:54:41, jfernandez wrote: > I'd need an API owner to land this change. I'm not sure you need it, checking the files that you're updating here. And the intent already has 3 LGTMs. BTW, you should remove external/wpt/css/css-align-3/distribution-values/space-evenly-001.html from TestExpectations, as it should be passing now.
The CQ bit was checked by jfernandez@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org Link to the patchset: https://codereview.chromium.org/2832503003/#ps20001 (title: "Unskip an external WPT for the space-evenly value.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jfernandez@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org Link to the patchset: https://codereview.chromium.org/2832503003/#ps40001 (title: "Patch rebased")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/23 12:53:09, Manuel Rego wrote: > On 2017/05/23 11:54:41, jfernandez wrote: > > I'd need an API owner to land this change. > > I'm not sure you need it, checking the files that you're updating here. > And the intent already has 3 LGTMs. Correct. > > BTW, you should remove > external/wpt/css/css-align-3/distribution-values/space-evenly-001.html > from TestExpectations, as it should be passing now.
rbyers@chromium.org changed reviewers: - chrishtr@chromium.org, rbyers@chromium.org, tkent@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jfernandez@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org Link to the patchset: https://codereview.chromium.org/2832503003/#ps50010 (title: "Fixed build errors.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jfernandez@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org Link to the patchset: https://codereview.chromium.org/2832503003/#ps70001 (title: "Added space-evenly to the css-properties expectations.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jfernandez@igalia.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jfernandez@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org Link to the patchset: https://codereview.chromium.org/2832503003/#ps40002 (title: "Fixed layout tests.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40002, "attempt_start_ts": 1495613362740960,
"parent_rev": "11b18084163dab946f4647306f8d93b65d3efdab", "commit_rev":
"69568ed51b77ef4a94d96f99134e09223acf6223"}
Message was sent while issue was closed.
Description was changed from ========== [css-flex] Implement the space-evenly content-distribution value The CSS Box Alignment specification defines the <content-distribution> set as some of the allowed values for the Content Distribution properties, align-content and justify-content. The 'space-evenly' value is not among the ones allowed for these properties according to the CSS Flexible Box specification. The CSS Flexbible box specification states that it must follow the CSS Box Alignment specification, so this new value must be considered as part of an upgraded level of the spec, which should be implemented eventually. Since we have already shipped an implementation of the new CSS Box Alignment values for CSS Grid Layout, we need to implement it for Flexbox as well. This is the intent-to-implement-and-ship request approved for this change: https://groups.google.com/a/chromium.org/d/msg/blink-dev/_4wD-NXS6Ik/W51KNjTz... BUG=708121 ========== to ========== [css-flex] Implement the space-evenly content-distribution value The CSS Box Alignment specification defines the <content-distribution> set as some of the allowed values for the Content Distribution properties, align-content and justify-content. The 'space-evenly' value is not among the ones allowed for these properties according to the CSS Flexible Box specification. The CSS Flexbible box specification states that it must follow the CSS Box Alignment specification, so this new value must be considered as part of an upgraded level of the spec, which should be implemented eventually. Since we have already shipped an implementation of the new CSS Box Alignment values for CSS Grid Layout, we need to implement it for Flexbox as well. This is the intent-to-implement-and-ship request approved for this change: https://groups.google.com/a/chromium.org/d/msg/blink-dev/_4wD-NXS6Ik/W51KNjTz... BUG=708121 Review-Url: https://codereview.chromium.org/2832503003 Cr-Commit-Position: refs/heads/master@{#474237} Committed: https://chromium.googlesource.com/chromium/src/+/69568ed51b77ef4a94d96f99134e... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:40002) as https://chromium.googlesource.com/chromium/src/+/69568ed51b77ef4a94d96f99134e... |
