Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2662573002:
[css-align] Implement place-content alignment shorthand (Closed)
|
Created:
3 years, 10 months ago by jfernandez Modified:
3 years, 9 months ago CC:
chromium-reviews, blink-reviews-style_chromium.org, blink-reviews-css, dglazkov+blink, apavlov+blink_chromium.org, darktears, blink-reviews, rwlbuis, Timothy Loh, sashab Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[css-align] Implement place-content alignment shorthand
The CSS Box Alignment specification defines a new shorthand to set the
Content Alignment properties (align-content and justify-content) at the
same time.
This patch provides the implementation of the CSS parsing logic and the
required regression tests. For the time being, as it happens with the
rest of the new alignment properties, the new parsing logic is
implemented behind the CSS Grid Layout runtime flag.
The intent-to-implement-and-ship of CSS Box Alignment shorthands is:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4
BUG=668639
Review-Url: https://codereview.chromium.org/2662573002
Cr-Commit-Position: refs/heads/master@{#454864}
Committed: https://chromium.googlesource.com/chromium/src/+/6c74eacf0850ef3ed1c3036af6231eb89cfb782f
Patch Set 1 #
Total comments: 12
Patch Set 2 : Applied suggested changes. #Patch Set 3 : Fixed layout tests. #Patch Set 4 : Fixing more layout tests. #Patch Set 5 : Fixed error in the test description. #Patch Set 6 : Patch rebased. #Messages
Total messages: 54 (23 generated)
Description was changed from ========== [css-align] Implement place-content alignment shorthand The CSS Box Alignment specification defines a new shorthand to set the Content Alignment properties (align-content and justify-content) at the same time. This patch provides the implementation of the CSS parsing logic and the required regression tests. For the time being, as it happens with the rest of the new alignment properties, the new parsing logic is implemented behind the CSS Grid Layout runtime flag. BUG=668639 ========== to ========== [css-align] Implement place-content alignment shorthand The CSS Box Alignment specification defines a new shorthand to set the Content Alignment properties (align-content and justify-content) at the same time. This patch provides the implementation of the CSS parsing logic and the required regression tests. For the time being, as it happens with the rest of the new alignment properties, the new parsing logic is implemented behind the CSS Grid Layout runtime flag. BUG=668639 ==========
jfernandez@igalia.com changed reviewers: + cbiesinger@chromium.org, rego@igalia.com, svillar@igalia.com, timloh@chromium.org
Sent out for review.
I'll let the parser experts review it. Looks good overall. https://codereview.chromium.org/2662573002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/alignment/parse-place-content.html (right): https://codereview.chromium.org/2662573002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/alignment/parse-place-content.html:3: <head> No need for <html> and <head> tags https://codereview.chromium.org/2662573002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/alignment/resources/alignment-parsing-utils-th.js (right): https://codereview.chromium.org/2662573002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/alignment/resources/alignment-parsing-utils-th.js:7: property + ' specified value is not what it should.'); unrelated change? https://codereview.chromium.org/2662573002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp (right): https://codereview.chromium.org/2662573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:2291: // value. You mean "specified" value? Also in the tests you check the value of the longhands but not the computed value of the shorthand.
timloh@ could you please take a look ?
Thanks for the review. https://codereview.chromium.org/2662573002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/alignment/parse-place-content.html (right): https://codereview.chromium.org/2662573002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/alignment/parse-place-content.html:3: <head> On 2017/02/02 09:11:19, svillar wrote: > No need for <html> and <head> tags Done. https://codereview.chromium.org/2662573002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/alignment/resources/alignment-parsing-utils-th.js (right): https://codereview.chromium.org/2662573002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/alignment/resources/alignment-parsing-utils-th.js:7: property + ' specified value is not what it should.'); On 2017/02/02 09:11:19, svillar wrote: > unrelated change? Umm, I guess git cl format applied some changes here. https://codereview.chromium.org/2662573002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp (right): https://codereview.chromium.org/2662573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:2291: // value. On 2017/02/02 09:11:19, svillar wrote: > You mean "specified" value? > Yes. > Also in the tests you check the value of the longhands but not the computed > value of the shorthand. Actually I do; look at the calls to checkValues in the different Test functions.
timloh@chromium.org changed reviewers: + meade@chromium.org - timloh@chromium.org
I don't work on style any more, meade@ is the current team lead and should be able to help out with reviews or routing to other appropriate reviewers. Quick comments below but I didn't look too closely. I think you might need to update StylePropertySerializer too so that inline style serialization works? https://codereview.chromium.org/2662573002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/CSSProperties.in (right): https://codereview.chromium.org/2662573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/CSSProperties.in:576: place-content runtime_flag=CSSGridLayout, longhands=align-content;justify-content This is a JSON file now btw, needs some rebasing. https://codereview.chromium.org/2662573002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2662573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4582: return CSSContentDistributionValue::create( Is this supposed to be the same as the first if() block? https://codereview.chromium.org/2662573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4594: if (m_range.atEnd()) I think this never happens or otherwise the next consume will return nullptr anyway.
FWIW, after Tim, sashab is probably the person in Sydney with the most knowledge of the parser. We've also been moving away from keeping all the logic in CSSPropertyParser.cpp, and splitting things out by property. Here's the design doc for this project: https://docs.google.com/document/d/1R2qiaQJ-SKkTTvnoUkuT9f-dmk5GlhzcvzeQj3iQa... Could you please implement this the new way instead of the old way? Here's an example CL for how this is being done for the existing properties: https://codereview.chromium.org/2653733005
On 2017/02/09 07:00:49, Eddy wrote: > FWIW, after Tim, sashab is probably the person in Sydney with the most knowledge > of the parser. > > We've also been moving away from keeping all the logic in CSSPropertyParser.cpp, > and splitting things out by property. Here's the design doc for this project: > https://docs.google.com/document/d/1R2qiaQJ-SKkTTvnoUkuT9f-dmk5GlhzcvzeQj3iQa... > > Could you please implement this the new way instead of the old way? Here's an > example CL for how this is being done for the existing properties: > https://codereview.chromium.org/2653733005 Scratch that, it's a shorthand, which we haven't got to yet. I think once you've addressed all of Tim's comments it should be fine.
Hi, On 2017/02/09 07:00:49, Eddy wrote: > FWIW, after Tim, sashab is probably the person in Sydney with the most knowledge > of the parser. > > We've also been moving away from keeping all the logic in CSSPropertyParser.cpp, > and splitting things out by property. Here's the design doc for this project: > https://docs.google.com/document/d/1R2qiaQJ-SKkTTvnoUkuT9f-dmk5GlhzcvzeQj3iQa... > Thanks for the info, I'll follow it for future CSS parsing patches.
New patch addressing the issues identified in the last review. https://codereview.chromium.org/2662573002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/CSSProperties.in (right): https://codereview.chromium.org/2662573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/CSSProperties.in:576: place-content runtime_flag=CSSGridLayout, longhands=align-content;justify-content On 2017/02/06 23:15:14, Timothy Loh wrote: > This is a JSON file now btw, needs some rebasing. Acknowledged. https://codereview.chromium.org/2662573002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2662573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4582: return CSSContentDistributionValue::create( On 2017/02/06 23:15:14, Timothy Loh wrote: > Is this supposed to be the same as the first if() block? Yes, we treat normal, baseline and last-baseline (<baseline-position>) as <item-position> values. We could have used a single if clause, but I thought it'd be clearer, form the spec-impl matching point of view, to handle such cases separately. https://codereview.chromium.org/2662573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4594: if (m_range.atEnd()) On 2017/02/06 23:15:14, Timothy Loh wrote: > I think this never happens or otherwise the next consume will return nullptr > anyway. Acknowledged.
@Eddy is there anything else to do on this CL ?
lgtm sorry I think I lost this one!
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_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 meade@chromium.org Link to the patchset: https://codereview.chromium.org/2662573002/#ps40001 (title: "Fixed layout tests.")
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_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 meade@chromium.org Link to the patchset: https://codereview.chromium.org/2662573002/#ps60001 (title: "Fixing more layout tests.")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
jfernandez@igalia.com changed reviewers: + rbyers@chromium.org, tkent@chromium.org
It seems I need API_OWNERS review to land this patch.
On 2017/02/23 at 13:07:28, jfernandez wrote: > It seems I need API_OWNERS review to land this patch. Grid Layout was already shipped, and this CL enables new CSS property by default. This needs Intent-to-ship.
On 2017/02/24 04:45:38, tkent wrote: > On 2017/02/23 at 13:07:28, jfernandez wrote: > > It seems I need API_OWNERS review to land this patch. > > > Grid Layout was already shipped, and this CL enables new CSS property by > default. This needs Intent-to-ship. Yes that's true. So just to clarify the topic when we ship Grid Layout we were shipping the alignment properties too: align-content, justify-content, align-items, justify-items, align-self and justify-self. Now the new shorthands place-content, place-items and place-self have been added to the spec and we're implementing them. They're just shorthands of properties that have been already shipped, but it seems that we need a new intent-to-ship regarding this. Can you confirm it? Thanks!
> They're just shorthands of properties that have been already shipped, > but it seems that we need a new intent-to-ship regarding this. Yes, we need.
On 2017/02/24 08:12:01, tkent wrote: > > They're just shorthands of properties that have been already shipped, > > but it seems that we need a new intent-to-ship regarding this. > > Yes, we need. Yes - from https://www.chromium.org/blink#trivial-changes: "However, any new API (no matter how small) is considered non-trivial. " Basically the motivation here is that every new API brings some risk that sites will come to depend on that API and we'll be unable to rename/remove it. So we need to be intentional about weighing the risk. In this case I don't expect it to be contentious.
As this hasn't landed yet and we need to wait for the intent-to-ship, maybe it's a good idea to upstream the test in csswg-test repository, and then importing it into Blink. @jfernandez, what do you think?
On 2017/02/24 22:05:59, Rick Byers wrote: > On 2017/02/24 08:12:01, tkent wrote: > > > They're just shorthands of properties that have been already shipped, > > > but it seems that we need a new intent-to-ship regarding this. > > > > Yes, we need. > > Yes - from https://www.chromium.org/blink#trivial-changes: > "However, any new API (no matter how small) is considered non-trivial. " > > Basically the motivation here is that every new API brings some risk that sites > will come to depend on that API and we'll be unable to rename/remove it. So we > need to be intentional about weighing the risk. In this case I don't expect it > to be contentious. I think there are enough support for the intent-to-ship request: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/qCfsZJhGtOQ
On 2017/03/03 21:20:15, jfernandez wrote: > On 2017/02/24 22:05:59, Rick Byers wrote: > > On 2017/02/24 08:12:01, tkent wrote: > > > > They're just shorthands of properties that have been already shipped, > > > > but it seems that we need a new intent-to-ship regarding this. > > > > > > Yes, we need. > > > > Yes - from https://www.chromium.org/blink#trivial-changes: > > "However, any new API (no matter how small) is considered non-trivial. " > > > > Basically the motivation here is that every new API brings some risk that > sites > > will come to depend on that API and we'll be unable to rename/remove it. So > we > > need to be intentional about weighing the risk. In this case I don't expect > it > > to be contentious. > > I think there are enough support for the intent-to-ship request: > > https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/qCfsZJhGtOQ Yep, thanks for the ping. Please put the intent-to-ship link into the CL description. LayoutTests/virtual/stable LGTM
Description was changed from
==========
[css-align] Implement place-content alignment shorthand
The CSS Box Alignment specification defines a new shorthand to set the
Content Alignment properties (align-content and justify-content) at the
same time.
This patch provides the implementation of the CSS parsing logic and the
required regression tests. For the time being, as it happens with the
rest of the new alignment properties, the new parsing logic is
implemented behind the CSS Grid Layout runtime flag.
BUG=668639
==========
to
==========
[css-align] Implement place-content alignment shorthand
The CSS Box Alignment specification defines a new shorthand to set the
Content Alignment properties (align-content and justify-content) at the
same time.
This patch provides the implementation of the CSS parsing logic and the
required regression tests. For the time being, as it happens with the
rest of the new alignment properties, the new parsing logic is
implemented behind the CSS Grid Layout runtime flag.
The intent-to-implement-and-ship of CSS Box Alignment
shorthands is:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4
BUG=668639
==========
Description was changed from
==========
[css-align] Implement place-content alignment shorthand
The CSS Box Alignment specification defines a new shorthand to set the
Content Alignment properties (align-content and justify-content) at the
same time.
This patch provides the implementation of the CSS parsing logic and the
required regression tests. For the time being, as it happens with the
rest of the new alignment properties, the new parsing logic is
implemented behind the CSS Grid Layout runtime flag.
The intent-to-implement-and-ship of CSS Box Alignment
shorthands is:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4
BUG=668639
==========
to
==========
[css-align] Implement place-content alignment shorthand
The CSS Box Alignment specification defines a new shorthand to set the
Content Alignment properties (align-content and justify-content) at the
same time.
This patch provides the implementation of the CSS parsing logic and the
required regression tests. For the time being, as it happens with the
rest of the new alignment properties, the new parsing logic is
implemented behind the CSS Grid Layout runtime flag.
The intent-to-implement-and-ship of CSS Box Alignment shorthands is:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4
BUG=668639
==========
The CQ bit was checked by jfernandez@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from meade@chromium.org Link to the patchset: https://codereview.chromium.org/2662573002/#ps80001 (title: "Fixed error in the test description.")
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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by jfernandez@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from meade@chromium.org, rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/2662573002/#ps100001 (title: "Patch rebased.")
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": 100001, "attempt_start_ts": 1488804713115130,
"parent_rev": "c5572dcc109df1bd1568efb6dbc79716f698e438", "commit_rev":
"6c74eacf0850ef3ed1c3036af6231eb89cfb782f"}
Message was sent while issue was closed.
Description was changed from
==========
[css-align] Implement place-content alignment shorthand
The CSS Box Alignment specification defines a new shorthand to set the
Content Alignment properties (align-content and justify-content) at the
same time.
This patch provides the implementation of the CSS parsing logic and the
required regression tests. For the time being, as it happens with the
rest of the new alignment properties, the new parsing logic is
implemented behind the CSS Grid Layout runtime flag.
The intent-to-implement-and-ship of CSS Box Alignment shorthands is:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4
BUG=668639
==========
to
==========
[css-align] Implement place-content alignment shorthand
The CSS Box Alignment specification defines a new shorthand to set the
Content Alignment properties (align-content and justify-content) at the
same time.
This patch provides the implementation of the CSS parsing logic and the
required regression tests. For the time being, as it happens with the
rest of the new alignment properties, the new parsing logic is
implemented behind the CSS Grid Layout runtime flag.
The intent-to-implement-and-ship of CSS Box Alignment shorthands is:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4
BUG=668639
Review-Url: https://codereview.chromium.org/2662573002
Cr-Commit-Position: refs/heads/master@{#454864}
Committed:
https://chromium.googlesource.com/chromium/src/+/6c74eacf0850ef3ed1c3036af623...
==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/6c74eacf0850ef3ed1c3036af623... |
