[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
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
Pay special attention to the approach I used to implement the
CSSComputedStyleDeclaration logic, which is different to the one used for the
align-items property.
My understanding of the specification is that the Computed Value should be the
Specified Value, so it will be the specific RenderObject instance which should
decide whether "Auto" value computes to "Strech" or "Start"; the same applies
for the "Legacy" keyword.
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
According to the feedback got from www-style mailing list, the current
especification draft is wrong regarding how to resolve the computed value of
justify-self/items and align-self/items when using "auto" as specified value.
http://lists.w3.org/Archives/Public/www-style/2014Jun/0333.html
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
5 years, 5 months ago
(2014-06-26 12:58:44 UTC)
#7
Applied suggested changes.
https://codereview.chromium.org/333423005/diff/60001/LayoutTests/fast/alignme...
File LayoutTests/fast/alignment/parse-justify-items.html (right):
https://codereview.chromium.org/333423005/diff/60001/LayoutTests/fast/alignme...
LayoutTests/fast/alignment/parse-justify-items.html:6: justify-items: baseline;
On 2014/06/24 19:04:56, Julien Chaffraix - PST wrote:
> I think we should test last-baseline as it's a valid keyword for
> <baseline-position>.
Done.
https://codereview.chromium.org/333423005/diff/60001/LayoutTests/fast/alignme...
LayoutTests/fast/alignment/parse-justify-items.html:40:
On 2014/06/24 19:04:56, Julien Chaffraix - PST wrote:
> I don't see any testing for flex-start / flex-end
Done.
https://codereview.chromium.org/333423005/diff/60001/LayoutTests/fast/alignme...
LayoutTests/fast/alignment/parse-justify-items.html:246:
shouldBe("getComputedStyle(element, '').getPropertyValue('justify-items')",
"'start'");
On 2014/06/24 19:04:56, Julien Chaffraix - PST wrote:
> Maybe we should test legacy + auto / stretch too.
Done.
https://codereview.chromium.org/333423005/diff/60001/LayoutTests/fast/alignme...
LayoutTests/fast/alignment/parse-justify-items.html:253:
shouldBe("getComputedStyle(element, '').getPropertyValue('justify-items')",
"'start'");
On 2014/06/24 19:04:56, Julien Chaffraix - PST wrote:
> Would it be worth checking flex-container too as it would compute to 'stretch'
> in this case?
Done.
https://codereview.chromium.org/333423005/diff/60001/LayoutTests/fast/alignme...
LayoutTests/fast/alignment/parse-justify-items.html:265:
shouldBe("getComputedStyle(element, '').getPropertyValue('justify-items')",
"'end'");
On 2014/06/24 19:04:56, Julien Chaffraix - PST wrote:
> That's awesome that you cover getComputedStyle with these. I would either add
> testing for element.style as part of this change or file a bug about it as we
> usually discover that it's broken down the road if we don't add testing.
Done.
https://codereview.chromium.org/333423005/diff/60001/Source/build/scripts/tem...
File Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (right):
https://codereview.chromium.org/333423005/diff/60001/Source/build/scripts/tem...
Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:542: // FIXME: We
should clear the overflow-alignment mode here and probably
On 2014/06/24 19:04:56, Julien Chaffraix - PST wrote:
> Is this FIXME still required as you have testing for that?
Done.
https://codereview.chromium.org/333423005/diff/60001/Source/core/css/CSSCompu...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):
https://codereview.chromium.org/333423005/diff/60001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:1558: static
PassRefPtrWillBeRawPtr<CSSValueList>
valueForItemPositionWithOverflowAlignment(ItemPosition itemPosition,
OverflowAlignment overflowAlignment, bool isLegacy)
On 2014/06/24 19:04:56, Julien Chaffraix - PST wrote:
> PLEASE NO BOOLEAN PARAMETERS!
>
> You can either add an enum and use it, or just pass in a pointer to an
> identifier (that can be NULL). I would lean towards the enum as it is way more
> readable at the call site.
I finally implemented an enumeration.
https://codereview.chromium.org/333423005/diff/60001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:1565:
result->append(CSSPrimitiveValue::create(overflowAlignment));
On 2014/06/24 19:04:56, Julien Chaffraix - PST wrote:
> ASSERT(result.size() <= 2); as you make the assumption above that we can map
it
> to a Pair?
"result->length() <= 2", instead, but I've got the idea. thanks.
https://codereview.chromium.org/333423005/diff/60001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:1571: return display == FLEX ||
display == INLINE_FLEX || display == GRID || display == INLINE_GRID;
On 2014/06/24 19:04:56, Julien Chaffraix - PST wrote:
> RenderStyle has some similar checks, maybe we could share the code somehow.
You
> don't have to do it here as I am not sure how this would work but a FIXME
would
> be nice.
I've finally implemented a new method "isDisplayFlexibleOrGridBox" in the
RenderStyle class, removed the static one defined in the StyleAdjuster and
adapted the already present calls to the new method.
https://codereview.chromium.org/333423005/diff/60001/Source/core/css/parser/C...
File Source/core/css/parser/CSSPropertyParser.cpp (right):
https://codereview.chromium.org/333423005/diff/60001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:1207: if
(!parseLegacyPosition(propId, important)) {
On 2014/06/24 19:04:56, Julien Chaffraix - PST wrote:
> I would avoid the negative branch as it confused me here (maybe it's just me
> though). It also avoids nesting:
>
> if (parseLegacyPosition(propId, important))
> return true;
>
> // Rewind and try the <item-position> && <overflow-position> | auto | stretch.
> m_valueList->setCurrentIndex(0);
> return parseItemPositionOverflowPosition(propId, important);
Done.
https://codereview.chromium.org/333423005/diff/60001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:4153: if (!value || value->id !=
CSSValueLegacy || !(value = m_valueList->next()))
On 2014/06/24 19:04:56, Julien Chaffraix - PST wrote:
> Per the grammar: legacy && [ left | right | center ]
>
> That means legacy can appear before *and after* the other keywords. Btw your
> test coverage doesn't handle this case.
Done.
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
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
5 years, 5 months ago
(2014-07-01 11:42:56 UTC)
#28
Message was sent while issue was closed.
Change committed as 177297
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
Message was sent while issue was closed.
Just a couple of quick comments on the StyleBuilder changes:
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
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.
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')}}
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.
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
Message was sent while issue was closed.
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.
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
Issue 333423005: [CSS Grid Layout] Implement 'justify-items' parsing
(Closed)
Created 5 years, 6 months ago by jfernandez
Modified 5 years, 5 months ago
Reviewers: Julien - ping for review, cbiesinger, ojan, Timothy Loh
Base URL: https://chromium.googlesource.com/chromium/blink.git@master
Comments: 49