Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(10)

Issue 333423005: [CSS Grid Layout] Implement 'justify-items' parsing (Closed)

Created:
5 years, 6 months ago by jfernandez
Modified:
5 years, 5 months ago
CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-rendering, dglazkov+blink, eae+blinkwatch, ed+blinkwatch_opera.com, jchaffraix+rendering, leviw+renderwatch, pdr., rwlbuis, rune+blink, zoltan1
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

[CSS Grid Layout] Implement 'justify-items' parsing This change adds the property from CSS 3 Box Alignment and implements the parsing. As the justify-self property, for now is protected by the CSS Grid Layout runtime flag. BUG=249451 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177297

Patch Set 1 #

Total comments: 9

Patch Set 2 : Suggested changes and additional test cases. #

Total comments: 2

Patch Set 3 : Applied suggested changes. #

Patch Set 4 : Modifying computed value resolution. #

Total comments: 22

Patch Set 5 : Applied suggested changes. #

Total comments: 12

Patch Set 6 : Applied suggested changes. #

Patch Set 7 : Added a FIXME for additional unit testing. #

Patch Set 8 : Patch rebased. #

Patch Set 9 : Updated css-properties-as-js-properties expectations. #

Patch Set 10 : Patch rebased. #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+674 lines, -24 lines) Patch
A LayoutTests/fast/alignment/parse-justify-items.html View 1 2 3 4 5 1 chunk +333 lines, -0 lines 0 comments Download
A LayoutTests/fast/alignment/parse-justify-items-expected.txt View 1 2 3 4 5 1 chunk +143 lines, -0 lines 0 comments Download
A LayoutTests/fast/alignment/resources/alignment-parsing-utils.js View 1 2 3 4 5 1 chunk +27 lines, -0 lines 0 comments Download
M LayoutTests/webexposed/css-properties-as-js-properties-expected.txt View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl View 1 2 3 4 5 6 7 2 chunks +19 lines, -2 lines 4 comments Download
M Source/core/css/CSSComputedStyleDeclaration.cpp View 1 2 3 4 5 6 7 8 9 5 chunks +48 lines, -5 lines 0 comments Download
M Source/core/css/CSSPrimitiveValueMappings.h View 1 2 3 4 5 6 7 8 9 2 chunks +5 lines, -0 lines 0 comments Download
M Source/core/css/CSSProperties.in View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/CSSProperty.cpp View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/CSSPropertyNames.in View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/CSSValueKeywords.in View 1 2 3 4 5 6 7 8 9 2 chunks +3 lines, -1 line 0 comments Download
M Source/core/css/RuntimeCSSEnabled.cpp View 1 2 3 4 5 6 7 1 chunk +2 lines, -1 line 0 comments Download
M Source/core/css/parser/CSSPropertyParser.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/parser/CSSPropertyParser.cpp View 1 2 3 4 5 6 7 8 9 3 chunks +44 lines, -2 lines 0 comments Download
M Source/core/css/resolver/StyleAdjuster.cpp View 1 2 3 4 5 6 7 2 chunks +2 lines, -12 lines 0 comments Download
M Source/core/frame/UseCounter.cpp View 1 2 3 4 5 6 7 2 chunks +2 lines, -1 line 0 comments Download
M Source/core/rendering/RenderFlexibleBox.cpp View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/rendering/RenderGrid.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/rendering/style/RenderStyle.h View 1 2 3 4 5 6 7 8 9 5 chunks +20 lines, -0 lines 0 comments Download
M Source/core/rendering/style/RenderStyleConstants.h View 1 2 3 4 5 6 7 8 9 2 chunks +6 lines, -0 lines 0 comments Download
M Source/core/rendering/style/StyleRareNonInheritedData.h View 1 2 3 4 5 6 7 1 chunk +4 lines, -0 lines 0 comments Download
M Source/core/rendering/style/StyleRareNonInheritedData.cpp View 1 2 3 4 5 3 chunks +9 lines, -0 lines 0 comments Download

Messages

Total messages: 31 (0 generated)
jfernandez
Pay special attention to the approach I used to implement the CSSComputedStyleDeclaration logic, which is ...
5 years, 6 months ago (2014-06-17 13:02:00 UTC) #1
cbiesinger
https://codereview.chromium.org/333423005/diff/1/Source/core/css/CSSPropertyNames.in File Source/core/css/CSSPropertyNames.in (right): https://codereview.chromium.org/333423005/diff/1/Source/core/css/CSSPropertyNames.in#newcode140 Source/core/css/CSSPropertyNames.in:140: -webkit-justify-items alias_for=justify-items Why add this? https://codereview.chromium.org/333423005/diff/1/Source/core/css/parser/CSSPropertyParser.cpp File Source/core/css/parser/CSSPropertyParser.cpp (right): ...
5 years, 6 months ago (2014-06-17 22:53:34 UTC) #2
jfernandez
Applied suggested changed and added additional test cases for the "legacy" keyword. I had to ...
5 years, 5 months ago (2014-06-19 15:19:14 UTC) #3
cbiesinger
lgtm with these changes Once you upload a new version, an owner will have to ...
5 years, 5 months ago (2014-06-21 00:45:24 UTC) #4
jfernandez
According to the feedback got from www-style mailing list, the current especification draft is wrong ...
5 years, 5 months ago (2014-06-24 16:08:40 UTC) #5
Julien - ping for review
It's going in the right direction but I think it needs an extra round. https://codereview.chromium.org/333423005/diff/60001/LayoutTests/fast/alignment/parse-justify-items.html ...
5 years, 5 months ago (2014-06-24 19:04:56 UTC) #6
jfernandez
Applied suggested changes. https://codereview.chromium.org/333423005/diff/60001/LayoutTests/fast/alignment/parse-justify-items.html File LayoutTests/fast/alignment/parse-justify-items.html (right): https://codereview.chromium.org/333423005/diff/60001/LayoutTests/fast/alignment/parse-justify-items.html#newcode6 LayoutTests/fast/alignment/parse-justify-items.html:6: justify-items: baseline; On 2014/06/24 19:04:56, Julien ...
5 years, 5 months ago (2014-06-26 12:58:44 UTC) #7
Julien - ping for review
lgtm with additional testing. https://codereview.chromium.org/333423005/diff/80001/LayoutTests/fast/alignment/parse-justify-items.html File LayoutTests/fast/alignment/parse-justify-items.html (right): https://codereview.chromium.org/333423005/diff/80001/LayoutTests/fast/alignment/parse-justify-items.html#newcode95 LayoutTests/fast/alignment/parse-justify-items.html:95: } Your test coverage still ...
5 years, 5 months ago (2014-06-26 17:37:20 UTC) #8
jfernandez
Applied suggested changes. https://codereview.chromium.org/333423005/diff/80001/LayoutTests/fast/alignment/parse-justify-items.html File LayoutTests/fast/alignment/parse-justify-items.html (right): https://codereview.chromium.org/333423005/diff/80001/LayoutTests/fast/alignment/parse-justify-items.html#newcode95 LayoutTests/fast/alignment/parse-justify-items.html:95: } On 2014/06/26 17:37:20, Julien Chaffraix ...
5 years, 5 months ago (2014-06-26 22:36:05 UTC) #9
jfernandez
I've added a FIXME to remember the unit testing issue, which I think s not ...
5 years, 5 months ago (2014-06-27 17:08:37 UTC) #10
jfernandez
The CQ bit was checked by jfernandez@igalia.com
5 years, 5 months ago (2014-06-27 17:08:49 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jfernandez@igalia.com/333423005/120001
5 years, 5 months ago (2014-06-27 17:09:33 UTC) #12
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
5 years, 5 months ago (2014-06-27 17:10:02 UTC) #13
commit-bot: I haz the power
Failed to apply patch for Source/core/frame/UseCounter.cpp: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...
5 years, 5 months ago (2014-06-27 17:10:03 UTC) #14
jfernandez
The CQ bit was checked by jfernandez@igalia.com
5 years, 5 months ago (2014-06-30 20:55:37 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jfernandez@igalia.com/333423005/140001
5 years, 5 months ago (2014-06-30 20:57:10 UTC) #16
jfernandez
The CQ bit was checked by jfernandez@igalia.com
5 years, 5 months ago (2014-06-30 21:38:35 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jfernandez@igalia.com/333423005/160001
5 years, 5 months ago (2014-06-30 21:39:37 UTC) #18
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_rel on tryserver.blink ...
5 years, 5 months ago (2014-06-30 22:48:01 UTC) #19
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
5 years, 5 months ago (2014-06-30 23:56:21 UTC) #20
commit-bot: I haz the power
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/14692)
5 years, 5 months ago (2014-06-30 23:56:22 UTC) #21
jfernandez
The CQ bit was checked by jfernandez@igalia.com
5 years, 5 months ago (2014-07-01 09:49:29 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jfernandez@igalia.com/333423005/160001
5 years, 5 months ago (2014-07-01 09:50:32 UTC) #23
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
5 years, 5 months ago (2014-07-01 09:50:46 UTC) #24
commit-bot: I haz the power
Failed to apply patch for Source/core/css/parser/CSSPropertyParser.cpp: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...
5 years, 5 months ago (2014-07-01 09:50:47 UTC) #25
jfernandez
The CQ bit was checked by jfernandez@igalia.com
5 years, 5 months ago (2014-07-01 10:32:19 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jfernandez@igalia.com/333423005/180001
5 years, 5 months ago (2014-07-01 10:33:10 UTC) #27
commit-bot: I haz the power
Change committed as 177297
5 years, 5 months ago (2014-07-01 11:42:56 UTC) #28
Timothy Loh
Just a couple of quick comments on the StyleBuilder changes: https://codereview.chromium.org/333423005/diff/180001/Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl File Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (left): https://codereview.chromium.org/333423005/diff/180001/Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl#oldcode565 ...
5 years, 5 months ago (2014-07-01 12:44:51 UTC) #29
jfernandez
https://codereview.chromium.org/333423005/diff/180001/Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl File Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (left): https://codereview.chromium.org/333423005/diff/180001/Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl#oldcode565 Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:565: // FIXME: We should clear the overflow-alignment mode here ...
5 years, 5 months ago (2014-07-01 20:54:27 UTC) #30
jfernandez
5 years, 5 months ago (2014-07-01 23:35:19 UTC) #31
Message was sent while issue was closed.
On 2014/07/01 20:54:27, jfernandez wrote:
>
https://codereview.chromium.org/333423005/diff/180001/Source/build/scripts/te...
> File Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (left):
> 
>
https://codereview.chromium.org/333423005/diff/180001/Source/build/scripts/te...
> Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:565: // FIXME:
We
> should clear the overflow-alignment mode here and probably
> On 2014/07/01 12:44:51, Timothy Loh wrote:
> > I'm not sure why you've removed this FIXME, isn't the behaviour still
> incorrect?
> > If you specify inherit for one of these properties, and your parent has used
a
> > pair and set the overflow-alignment mode, then won't we fail to inherit the
> > overflow-alignment mode, since we use default initial/inherit handlers? You
> can
> > see the current behaviour for initial/inherit in the generated file
> > StyleBuilderFunctions.cpp.
> 
> I added cases in the layout tests to verify it was working as defined in the
> specs, but you are right, it's not working as expected in the case of
"inherit"
> keyword. It works fine for the "initial", at least, regarding the Computed
> Value. I think the FIXME still applies in the case of the Used Value. 
> 
> I'm preparing a new patch resolving these issues, according to the deleted
FIXME
> comment.
> 
>
https://codereview.chromium.org/333423005/diff/180001/Source/build/scripts/te...
> File Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (right):
> 
>
https://codereview.chromium.org/333423005/diff/180001/Source/build/scripts/te...
> Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:589:
> {{apply_alignment_with_legacy_keyword('CSSPropertyJustifyItems',
> 'JustifyItems')}}
> On 2014/07/01 12:44:51, Timothy Loh wrote:
> > If you're only going to have a single property with this application rule,
you
> > should probably put the logic in StyleBuilderCustom instead. I understand
that
> > there's a few places where the logic can end up, and writing some
> documentation
> > on the right way to do this is on my todo list.
> 
> I thought that "align-items" would eventually be allowed to use the "legacy",
I
> still have to clarify that with the specs editor. But you are right that for
the
> time being, this function is better defined in the StyleBuilderCustom. Ill
cook
> a new patch to do so.

The patch with the suggested changes can be reviewed here:

https://codereview.chromium.org/361113002

Powered by Google App Engine
This is Rietveld 408576698