Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1798863005:
Move some grid-column/grid-row related longhands into CSSPropertyParser (Closed)
|
Created:
4 years, 9 months ago by rwlbuis Modified:
4 years, 9 months ago CC:
chromium-reviews, blink-reviews-css, dglazkov+blink, apavlov+blink_chromium.org, darktears, blink-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove some grid-column/grid-row related longhands into CSSPropertyParser
Move the following grid-column/grid-row related longhands from
LegacyCSSPropertyParser into CSSPropertyParser:
grid-column-start, grid-column-end,
grid-row-start, grid-row-end
BUG=499780
Committed: https://crrev.com/f4fd3e2d624ded5f0c038ed0da633b68061090cf
Cr-Commit-Position: refs/heads/master@{#381645}
Patch Set 1 #Patch Set 2 : Also take into account auto+csswide keywords #Patch Set 3 : Fix test result #Patch Set 4 : V4 #
Total comments: 4
Patch Set 5 : Address review comments #Patch Set 6 : Standalone change #
Messages
Total messages: 24 (10 generated)
Description was changed from ========== grid-column WIP BUG= ========== to ========== Move some grid-column/grid-row related longhands into CSSPropertyParser Move the following grid-column/grid-row related longhands from LegacyCSSPropertyParser into CSSPropertyParser: grid-column-start, grid-column-end, grid-row-start, grid-row-end BUG=499780 ==========
rob.buis@samsung.com changed reviewers: + jfernandez@igalia.com, timloh@chromium.org
PTAL.
jfernandez - do you know what the intended behavior for match 'auto' or 'span' as <custom-ident> in the <grid-line> type is? FF seems to allow 'auto' to match the production, so you can type '1 auto' or 'span auto 1' (but interestingly not 'auto 1') https://codereview.chromium.org/1798863005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1798863005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3274: if (isCSSWideKeyword(range.peek().id()) || range.peek().id() == CSSValueAuto || range.peek().id() == CSSValueSpan) isCSSWideKeyword should be in consumeCustomIdent, can we fix this first? Declarations like "animation: 2 inherit" should be invalid. Also I'm not sure if span/auto are actually invalid here... Maybe if you just write "span" it should match as a <custom-ident>? In any case, the function name needs to make it clear it's specific to grid lines https://codereview.chromium.org/1798863005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3304: // Negative numbers are not allowed for span (but are for <integer>). Comment is misleading since <integer> is a type defined in css-values which can be negative. Also IMO it's clearer if you break this into two separate if statements, it sort of looks like the brackets are in the wrong place.
On 2016/03/16 04:24:26, Timothy Loh wrote: > jfernandez - do you know what the intended behavior for match 'auto' or 'span' > as <custom-ident> in the <grid-line> type is? FF seems to allow 'auto' to match > the production, so you can type '1 auto' or 'span auto 1' (but interestingly not > 'auto 1') Last draft of the specs defines the <grid-position> parsing as follows: auto | <custom-ident> | [ <integer> && <custom-ident>? ] | [ span && [ <integer> || <custom-ident> ] ] So, "auto 1" is clearly invalid, IMO. I think "span auto 1" should be invalid as well, but I'll add @rego to the discussion as he has been working recently on the grid positioning logic. > > https://codereview.chromium.org/1798863005/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): > > https://codereview.chromium.org/1798863005/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3274: if > (isCSSWideKeyword(range.peek().id()) || range.peek().id() == CSSValueAuto || > range.peek().id() == CSSValueSpan) > isCSSWideKeyword should be in consumeCustomIdent, can we fix this first? > Declarations like "animation: 2 inherit" should be invalid. > > Also I'm not sure if span/auto are actually invalid here... Maybe if you just > write "span" it should match as a <custom-ident>? In any case, the function name > needs to make it clear it's specific to grid lines > > https://codereview.chromium.org/1798863005/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3304: // > Negative numbers are not allowed for span (but are for <integer>). > Comment is misleading since <integer> is a type defined in css-values which can > be negative. Also IMO it's clearer if you break this into two separate if > statements, it sort of looks like the brackets are in the wrong place.
jfernandez@igalia.com changed reviewers: + rego@igalia.com
On 2016/03/16 09:36:04, jfernandez wrote: > On 2016/03/16 04:24:26, Timothy Loh wrote: > > jfernandez - do you know what the intended behavior for match 'auto' or 'span' > > as <custom-ident> in the <grid-line> type is? FF seems to allow 'auto' to > match > > the production, so you can type '1 auto' or 'span auto 1' (but interestingly > not > > 'auto 1') > > Last draft of the specs defines the <grid-position> parsing as follows: > > auto | > <custom-ident> | > [ <integer> && <custom-ident>? ] | > [ span && [ <integer> || <custom-ident> ] ] > > So, "auto 1" is clearly invalid, IMO. > > I think "span auto 1" should be invalid as well, but I'll add @rego > to the discussion as he has been working recently on the grid > positioning logic. In theory "auto" and "span" shouldn't be valid custom-idents for these properties. So I think that "auto 1", "1 auto", "span auto", "auto span" "span 1 auto", "span auto 1", etc. all should be invalid. From the specs (https://drafts.csswg.org/css-values-3/#identifier-value): "Some properties accept arbitrary author-defined identifiers as a component value. This generic data type is denoted by <custom-ident>, and represents any valid CSS identifier that would not be misinterpreted as a pre-defined keyword in that property’s value definition." The problem is that this is not fixed in Blink, there were some attempts in the past (like https://codereview.chromium.org/179383003/) but the issue is still present. This is a generic issue that affects not only Grid Layout properties.
rego@igalia.com changed reviewers: + svillar@igalia.com
https://codereview.chromium.org/1798863005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1798863005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3274: if (isCSSWideKeyword(range.peek().id()) || range.peek().id() == CSSValueAuto || range.peek().id() == CSSValueSpan) On 2016/03/16 04:24:25, Timothy Loh wrote: > isCSSWideKeyword should be in consumeCustomIdent, can we fix this first? > Declarations like "animation: 2 inherit" should be invalid. Sure, I moved that here: https://codereview.chromium.org/1808753002/
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by rob.buis@samsung.com
The CQ bit was unchecked by rob.buis@samsung.com
On 2016/03/16 10:51:30, Manuel Rego wrote: > On 2016/03/16 09:36:04, jfernandez wrote: > > On 2016/03/16 04:24:26, Timothy Loh wrote: > > > jfernandez - do you know what the intended behavior for match 'auto' or > 'span' > > > as <custom-ident> in the <grid-line> type is? FF seems to allow 'auto' to > > match > > > the production, so you can type '1 auto' or 'span auto 1' (but interestingly > > not > > > 'auto 1') > > > > Last draft of the specs defines the <grid-position> parsing as follows: > > > > auto | > > <custom-ident> | > > [ <integer> && <custom-ident>? ] | > > [ span && [ <integer> || <custom-ident> ] ] > > > > So, "auto 1" is clearly invalid, IMO. > > > > I think "span auto 1" should be invalid as well, but I'll add @rego > > to the discussion as he has been working recently on the grid > > positioning logic. > > In theory "auto" and "span" shouldn't be valid custom-idents > for these properties. > > So I think that "auto 1", "1 auto", "span auto", "auto span" > "span 1 auto", "span auto 1", etc. all should be invalid. Good point, I added those tests to grid-item-end-after-get-set.html.
https://codereview.chromium.org/1798863005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1798863005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3304: // Negative numbers are not allowed for span (but are for <integer>). On 2016/03/16 04:24:26, Timothy Loh wrote: > Comment is misleading since <integer> is a type defined in css-values which can > be negative. Also IMO it's clearer if you break this into two separate if > statements, it sort of looks like the brackets are in the wrong place. Done.
PTAL. Note that this change depends on the following patch to work: https://codereview.chromium.org/1808753002/
lgtm On 2016/03/16 10:51:30, Manuel Rego wrote: > On 2016/03/16 09:36:04, jfernandez wrote: > > On 2016/03/16 04:24:26, Timothy Loh wrote: > > > jfernandez - do you know what the intended behavior for match 'auto' or > 'span' > > > as <custom-ident> in the <grid-line> type is? FF seems to allow 'auto' to > > match > > > the production, so you can type '1 auto' or 'span auto 1' (but interestingly > > not > > > 'auto 1') > > > > Last draft of the specs defines the <grid-position> parsing as follows: > > > > auto | > > <custom-ident> | > > [ <integer> && <custom-ident>? ] | > > [ span && [ <integer> || <custom-ident> ] ] > > > > So, "auto 1" is clearly invalid, IMO. > > > > I think "span auto 1" should be invalid as well, but I'll add @rego > > to the discussion as he has been working recently on the grid > > positioning logic. > > In theory "auto" and "span" shouldn't be valid custom-idents > for these properties. > > So I think that "auto 1", "1 auto", "span auto", "auto span" > "span 1 auto", "span auto 1", etc. all should be invalid. > > From the specs (https://drafts.csswg.org/css-values-3/#identifier-value): > "Some properties accept arbitrary author-defined identifiers > as a component value. This generic data type is denoted > by <custom-ident>, and represents any valid CSS identifier > that would not be misinterpreted as a pre-defined keyword > in that property’s value definition." > > The problem is that this is not fixed in Blink, > there were some attempts in the past > (like https://codereview.chromium.org/179383003/) > but the issue is still present. > This is a generic issue that affects not only > Grid Layout properties. Seems reasonable for this patch to match current behaviour in rejecting auto/span as <custom-ident>. I'm not entirely convinced by the specs though, as css-values says "Specifications using <custom-ident> must specify clearly what other keywords are excluded from <custom-ident>, if any—for example by saying that any pre-defined keywords in that property’s value definition are excluded." The grid spec doesn't specify any keywords to be excluded. Example 4 in css-values has "animation: ease-in ease-out" as valid with ease-out as the animation name, so it seems like "span auto" should be similarly valid. Tab has said (https://lists.w3.org/Archives/Public/www-style/2014Feb/0734.html) that these wouldn't be valid though, so maybe the css-grid spec just needs to be updated to be clearer?
The CQ bit was checked by rob.buis@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1798863005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1798863005/120001
Message was sent while issue was closed.
Description was changed from ========== Move some grid-column/grid-row related longhands into CSSPropertyParser Move the following grid-column/grid-row related longhands from LegacyCSSPropertyParser into CSSPropertyParser: grid-column-start, grid-column-end, grid-row-start, grid-row-end BUG=499780 ========== to ========== Move some grid-column/grid-row related longhands into CSSPropertyParser Move the following grid-column/grid-row related longhands from LegacyCSSPropertyParser into CSSPropertyParser: grid-column-start, grid-column-end, grid-row-start, grid-row-end BUG=499780 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Move some grid-column/grid-row related longhands into CSSPropertyParser Move the following grid-column/grid-row related longhands from LegacyCSSPropertyParser into CSSPropertyParser: grid-column-start, grid-column-end, grid-row-start, grid-row-end BUG=499780 ========== to ========== Move some grid-column/grid-row related longhands into CSSPropertyParser Move the following grid-column/grid-row related longhands from LegacyCSSPropertyParser into CSSPropertyParser: grid-column-start, grid-column-end, grid-row-start, grid-row-end BUG=499780 Committed: https://crrev.com/f4fd3e2d624ded5f0c038ed0da633b68061090cf Cr-Commit-Position: refs/heads/master@{#381645} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/f4fd3e2d624ded5f0c038ed0da633b68061090cf Cr-Commit-Position: refs/heads/master@{#381645}
Message was sent while issue was closed.
On 2016/03/17 00:12:31, Timothy Loh wrote: > Seems reasonable for this patch to match current behaviour in rejecting > auto/span as <custom-ident>. I'm not entirely convinced by the specs though, as > css-values says > > "Specifications using <custom-ident> must specify clearly what other keywords > are excluded from <custom-ident>, if any—for example by saying that any > pre-defined keywords in that property’s value definition are excluded." > > The grid spec doesn't specify any keywords to be excluded. Example 4 in > css-values has "animation: ease-in ease-out" as valid with ease-out as the > animation name, so it seems like "span auto" should be similarly valid. Tab has > said (https://lists.w3.org/Archives/Public/www-style/2014Feb/0734.html) that > these wouldn't be valid though, so maybe the css-grid spec just needs to be > updated to be clearer? Yep, you're right there's a kind of contradiction in the spec, I've mailed CSS WG to clarify it: https://lists.w3.org/Archives/Public/www-style/2016Mar/0247.html
Message was sent while issue was closed.
On 2016/03/17 09:05:32, Manuel Rego wrote: > On 2016/03/17 00:12:31, Timothy Loh wrote: > > Seems reasonable for this patch to match current behaviour in rejecting > > auto/span as <custom-ident>. I'm not entirely convinced by the specs though, > as > > css-values says > > > > "Specifications using <custom-ident> must specify clearly what other keywords > > are excluded from <custom-ident>, if any—for example by saying that any > > pre-defined keywords in that property’s value definition are excluded." > > > > The grid spec doesn't specify any keywords to be excluded. Example 4 in > > css-values has "animation: ease-in ease-out" as valid with ease-out as the > > animation name, so it seems like "span auto" should be similarly valid. Tab > has > > said (https://lists.w3.org/Archives/Public/www-style/2014Feb/0734.html) that > > these wouldn't be valid though, so maybe the css-grid spec just needs to be > > updated to be clearer? > > Yep, you're right there's a kind of contradiction in the spec, > I've mailed CSS WG to clarify it: > https://lists.w3.org/Archives/Public/www-style/2016Mar/0247.html As you thought Tab confirmed that the grid spec needs to be updated: https://lists.w3.org/Archives/Public/www-style/2016Mar/0256.html |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
