Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2375343002:
CSS Motion Path: offset-shorthand requires path before distance rotation (Closed)
|
Created:
4 years, 2 months ago by Eric Willigers Modified:
4 years, 1 month ago CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, chromium-reviews, dglazkov+blink, rwlbuis Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCSS Motion Path: offset-shorthand requires path before distance rotation
We require offset-path to appear first, then offset-distance and
offset-rotation.
Standards discussion is continuing, this is intentionally conservative.
Spec (not yet updated):
https://drafts.fxtf.org/motion-1/#offset-shorthand
BUG=638055
Committed: https://crrev.com/96a59f8297eaff8ff8f70f35ec489d7f47e0e6c8
Cr-Commit-Position: refs/heads/master@{#423059}
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 10
Patch Set 3 : comment #
Total comments: 2
Patch Set 4 : options #Patch Set 5 : path distance rotation all required #
Messages
Total messages: 43 (25 generated)
The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ericwilligers@chromium.org changed reviewers: + timloh@chromium.org
This CL prepares us for shipping of offset-position, offset-distance and offset-rotation. The larger CL https://codereview.chromium.org/2368013002/ can wait until we are ready to add offset-position and offset-anchor to the shorthand.
ericwilligers@chromium.org changed reviewers: + sashab@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
alancutter@chromium.org changed reviewers: + alancutter@chromium.org
Please link to spec in description. https://drafts.fxtf.org/motion-1/#offset-shorthand
Description was changed from ========== CSS Motion Path: offset-shorthand requires path before distance rotation We require offset-path to appear first. offset-distance and/or offset-rotation may optionally follow, in either order. Standards discussion: https://logs.csswg.org/irc.w3.org/css/2016-09-20/#e724237 RESOLVED: Position before path before distance and anchor Shorthand support for offset-position and offset-anchor is not yet implemented. BUG=638055 ========== to ========== CSS Motion Path: offset-shorthand requires path before distance rotation We require offset-path to appear first. offset-distance and/or offset-rotation may optionally follow, in either order. Standards discussion: https://logs.csswg.org/irc.w3.org/css/2016-09-20/#e724237 RESOLVED: Position before path before distance and anchor Shorthand support for offset-position and offset-anchor is not yet implemented. Spec (not yet updated): https://drafts.fxtf.org/motion-1/#offset-shorthand BUG=638055 ==========
Description was changed from ========== CSS Motion Path: offset-shorthand requires path before distance rotation We require offset-path to appear first. offset-distance and/or offset-rotation may optionally follow, in either order. Standards discussion: https://logs.csswg.org/irc.w3.org/css/2016-09-20/#e724237 RESOLVED: Position before path before distance and anchor Shorthand support for offset-position and offset-anchor is not yet implemented. Spec (not yet updated): https://drafts.fxtf.org/motion-1/#offset-shorthand BUG=638055 ========== to ========== CSS Motion Path: offset-shorthand requires path before distance rotation We require offset-path to appear first. offset-distance and/or offset-rotation may optionally follow, in either order. Standards discussion: https://logs.csswg.org/irc.w3.org/css/2016-09-20/#e724237 RESOLVED: Position before path before distance and anchor Shorthand support for offset-position and offset-anchor is not yet implemented. Spec (not yet updated): https://drafts.fxtf.org/motion-1/#offset-shorthand Grammar supported by this CL: <offset-path> [ <offset-distance>? && <offset-rotation>? ] BUG=638055 ==========
https://codereview.chromium.org/2375343002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/css-parser/offset-parsing.html (right): https://codereview.chromium.org/2375343002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/css-parser/offset-parsing.html:8: assert_valid_value("offset", "path('M 0 0 H 1') -200% auto"); Add more positive tests that cover the different branches in the code. https://codereview.chromium.org/2375343002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2375343002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1608: bool CSSPropertyParser::consumeOffsetShorthand(bool important) { Add a comment with the grammar this is implementing.
https://github.com/w3c/fxtf-drafts/pull/52 has the possible grammar when offset-position and offset-anchor are also accepted.
Patchset #3 (id:30001) has been deleted
ericwilligers@chromium.org changed reviewers: - sashab@chromium.org
https://codereview.chromium.org/2375343002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/css-parser/offset-parsing.html (right): https://codereview.chromium.org/2375343002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/css-parser/offset-parsing.html:8: assert_valid_value("offset", "path('M 0 0 H 1') -200% auto"); On 2016/10/04 04:42:15, alancutter wrote: > Add more positive tests that cover the different branches in the code. Done. https://codereview.chromium.org/2375343002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2375343002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1608: bool CSSPropertyParser::consumeOffsetShorthand(bool important) { On 2016/10/04 04:42:15, alancutter wrote: > Add a comment with the grammar this is implementing. Done.
alancutter@chromium.org changed reviewers: + sashab@chromium.org
https://codereview.chromium.org/2375343002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2375343002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1623: 0, CSSPrimitiveValue::UnitType::Pixels), Nit: Handle this the same way you handle !offsetRotation, as a separate if construct. https://codereview.chromium.org/2375343002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1630: } I don't see a reason to mix up parsing and calls to addProperty, let's leave the adds to the end. https://codereview.chromium.org/2375343002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1631: addProperty(CSSPropertyOffsetRotation, CSSPropertyOffset, *offsetRotation, I don't think we should be adding properties if we're going to return false at least that's the rule other functions seem to be following, timloh can probably clarify on this.
https://codereview.chromium.org/2375343002/diff/50001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/css-parser/offset-parsing.html (right): https://codereview.chromium.org/2375343002/diff/50001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/css-parser/offset-parsing.html:23: assert_invalid_value("offset", "none / 10px 20px 30deg"); Add negative test that has valid value with extra stuff at the end.
Patchset #4 (id:70001) has been deleted
https://codereview.chromium.org/2375343002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2375343002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1623: 0, CSSPrimitiveValue::UnitType::Pixels), On 2016/10/04 05:00:18, alancutter wrote: > Nit: Handle this the same way you handle !offsetRotation, as a separate if > construct. Done. https://codereview.chromium.org/2375343002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1630: } On 2016/10/04 05:00:18, alancutter wrote: > I don't see a reason to mix up parsing and calls to addProperty, let's leave the > adds to the end. Changed. This wasn't parsing, just creating default values. https://codereview.chromium.org/2375343002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1631: addProperty(CSSPropertyOffsetRotation, CSSPropertyOffset, *offsetRotation, On 2016/10/04 05:00:18, alancutter wrote: > I don't think we should be adding properties if we're going to return false at > least that's the rule other functions seem to be following, timloh can probably > clarify on this. Changed. Font shorthand, for example, does add properties even if we might return false. None of the elements' properties change values if we return false. https://codereview.chromium.org/2375343002/diff/50001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/css-parser/offset-parsing.html (right): https://codereview.chromium.org/2375343002/diff/50001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/css-parser/offset-parsing.html:23: assert_invalid_value("offset", "none / 10px 20px 30deg"); On 2016/10/04 05:02:05, alancutter wrote: > Add negative test that has valid value with extra stuff at the end. Done.
lgtm
The code looks fine, but from the IRC log it isn't clear to me that the is the grammar we want to be implementing. It seems like removing the not-yet-implemented position/anchor properties from the grammar in the IRC log (<offset-position>? [ <offset-path> [ <offset-rotation> || <offset-distance> ] [ / <offset-anchor> ]? ]) would result in something slightly different to what is implemented here as we would require at least one of offset-rotation/offset-distance.
Description was changed from ========== CSS Motion Path: offset-shorthand requires path before distance rotation We require offset-path to appear first. offset-distance and/or offset-rotation may optionally follow, in either order. Standards discussion: https://logs.csswg.org/irc.w3.org/css/2016-09-20/#e724237 RESOLVED: Position before path before distance and anchor Shorthand support for offset-position and offset-anchor is not yet implemented. Spec (not yet updated): https://drafts.fxtf.org/motion-1/#offset-shorthand Grammar supported by this CL: <offset-path> [ <offset-distance>? && <offset-rotation>? ] BUG=638055 ========== to ========== CSS Motion Path: offset-shorthand requires path before distance rotation We require offset-path to appear first. offset-distance and/or offset-rotation may optionally follow, in either order. Standards discussion: https://logs.csswg.org/irc.w3.org/css/2016-09-20/#e724237 RESOLVED: Position before path before distance and anchor Shorthand support for offset-position and offset-anchor is not yet implemented. Spec (not yet updated): https://drafts.fxtf.org/motion-1/#offset-shorthand Grammar supported by this CL: <offset-path> [ <offset-distance> || <offset-rotation> ]? BUG=638055 ==========
ericwilligers@chromium.org changed reviewers: + shans@chromium.org - sashab@chromium.org
Patchset #4 (id:50002) has been deleted
On 2016/10/04 at 05:45:46, timloh wrote: > The code looks fine, but from the IRC log it isn't clear to me that the is the grammar we want to be implementing. It seems like removing the not-yet-implemented position/anchor properties from the grammar in the IRC log (<offset-position>? [ <offset-path> [ <offset-rotation> || <offset-distance> ] [ / <offset-anchor> ]? ]) would result in something slightly different to what is implemented here as we would require at least one of offset-rotation/offset-distance. That was Tab's quick stab at a grammar and I think it's wrong. I *think* you want: <offset-position>? [ <offset-path> [ <offset-rotation> || <offset-distance> ]? ]? [ / <offset-anchor> ]? i.e. the following constructions (and only the following constructions) are OK: (null) <offset-position> <offset-position> / <offset-anchor> <offset-position> <offset-path> <offset-position> <offset-path> <offset-rotation> <offset-position> <offset-path> <offset-distance> <offset-position> <offset-path> <offset-rotation> <offset-distance> <offset-position> <offset-path> <offset-distance> <offset-rotation> <offset-position> <offset-path> / <offset-anchor> <offset-position> <offset-path> <offset-rotation> / <offset-anchor> <offset-position> <offset-path> <offset-distance> / <offset-anchor> <offset-position> <offset-path> <offset-rotation> <offset-distance> / <offset-anchor> <offset-position> <offset-path> <offset-distance> <offset-rotation> / <offset-anchor> / <offset-anchor> (*though this is **weird** and maybe shouldn't be OK) <offset-path> <offset-path> <offset-rotation> <offset-path> <offset-distance> <offset-path> <offset-rotation> <offset-distance> <offset-path> <offset-distance> <offset-rotation> <offset-path> / <offset-anchor> <offset-path> <offset-rotation> / <offset-anchor> <offset-path> <offset-distance> / <offset-anchor> <offset-path> <offset-rotation> <offset-distance> / <offset-anchor> <offset-path> <offset-distance> <offset-rotation> / <offset-anchor> We know there are problems with <offset-position> next to <offset-anchor>, <offset-position> or <offset-anchor> next to <offset-distance>, and <offset-position> or <offset-anchor> next to <offset-rotation>. This grammar seems to prevent all of those constructions. If there are no other issues that I'm forgetting, I'm OK with implementing the subset of this that's in this patch (<offset-path> [ <offset-rotation> || <offset-distance> ]?). If I'm forgetting stuff - perhaps if in the interests of forward compatibility we should *only* accept <offset-path> <offset-distance> <offset-rotation> *in that order* and none optional for now? Or maybe this is a good idea regardless?
On 2016/10/04 10:42:48, shans wrote: > On 2016/10/04 at 05:45:46, timloh wrote: > > The code looks fine, but from the IRC log it isn't clear to me that the is the > grammar we want to be implementing. It seems like removing the > not-yet-implemented position/anchor properties from the grammar in the IRC log > (<offset-position>? [ <offset-path> [ <offset-rotation> || <offset-distance> ] [ > / <offset-anchor> ]? ]) would result in something slightly different to what is > implemented here as we would require at least one of > offset-rotation/offset-distance. > > That was Tab's quick stab at a grammar and I think it's wrong. I *think* you > want: > > <offset-position>? [ <offset-path> [ <offset-rotation> || <offset-distance> ]? > ]? [ / <offset-anchor> ]? > > i.e. the following constructions (and only the following constructions) are OK: > (null) > <offset-position> > <offset-position> / <offset-anchor> > <offset-position> <offset-path> > <offset-position> <offset-path> <offset-rotation> > <offset-position> <offset-path> <offset-distance> > <offset-position> <offset-path> <offset-rotation> <offset-distance> > <offset-position> <offset-path> <offset-distance> <offset-rotation> > <offset-position> <offset-path> / <offset-anchor> > <offset-position> <offset-path> <offset-rotation> / <offset-anchor> > <offset-position> <offset-path> <offset-distance> / <offset-anchor> > <offset-position> <offset-path> <offset-rotation> <offset-distance> / > <offset-anchor> > <offset-position> <offset-path> <offset-distance> <offset-rotation> / > <offset-anchor> > / <offset-anchor> (*though this is **weird** and maybe shouldn't be OK) > <offset-path> > <offset-path> <offset-rotation> > <offset-path> <offset-distance> > <offset-path> <offset-rotation> <offset-distance> > <offset-path> <offset-distance> <offset-rotation> > <offset-path> / <offset-anchor> > <offset-path> <offset-rotation> / <offset-anchor> > <offset-path> <offset-distance> / <offset-anchor> > <offset-path> <offset-rotation> <offset-distance> / <offset-anchor> > <offset-path> <offset-distance> <offset-rotation> / <offset-anchor> > > We know there are problems with <offset-position> next to <offset-anchor>, > <offset-position> or <offset-anchor> next to <offset-distance>, and > <offset-position> or <offset-anchor> next to <offset-rotation>. This grammar > seems to prevent all of those constructions. If there are no other issues that > I'm forgetting, I'm OK with implementing the subset of this that's in this patch > (<offset-path> [ <offset-rotation> || <offset-distance> ]?). > > If I'm forgetting stuff - perhaps if in the interests of forward compatibility > we should *only* accept <offset-path> <offset-distance> <offset-rotation> *in > that order* and none optional for now? Or maybe this is a good idea regardless? An advantage only accepting <offset-path> <offset-distance> <offset-rotation> *in > that order* would be that we leave the door open for the following proposal:- [ <offset-position> | <offset-position>? <offset-path> <offset-distance>? ] [ <offset-rotation> <offset-anchor>? ]? If <offset-path> is not present, <offset-position> must not be 'auto'. 'auto' is not a valid value for <offset-anchor>. (The initial value of <offset-anchor> is 'center'. The value indicating we use <offset-position> is TBD.)
Description was changed from ========== CSS Motion Path: offset-shorthand requires path before distance rotation We require offset-path to appear first. offset-distance and/or offset-rotation may optionally follow, in either order. Standards discussion: https://logs.csswg.org/irc.w3.org/css/2016-09-20/#e724237 RESOLVED: Position before path before distance and anchor Shorthand support for offset-position and offset-anchor is not yet implemented. Spec (not yet updated): https://drafts.fxtf.org/motion-1/#offset-shorthand Grammar supported by this CL: <offset-path> [ <offset-distance> || <offset-rotation> ]? BUG=638055 ========== to ========== CSS Motion Path: offset-shorthand requires path before distance rotation We require offset-path to appear first, then offset-distance and offset-rotation. Standards discussion is continuing, this is intentionally conservative. Spec (not yet updated): https://drafts.fxtf.org/motion-1/#offset-shorthand BUG=638055 ==========
lgtm
The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ericwilligers@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alancutter@chromium.org Link to the patchset: https://codereview.chromium.org/2375343002/#ps120001 (title: "path distance rotation all required")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== CSS Motion Path: offset-shorthand requires path before distance rotation We require offset-path to appear first, then offset-distance and offset-rotation. Standards discussion is continuing, this is intentionally conservative. Spec (not yet updated): https://drafts.fxtf.org/motion-1/#offset-shorthand BUG=638055 ========== to ========== CSS Motion Path: offset-shorthand requires path before distance rotation We require offset-path to appear first, then offset-distance and offset-rotation. Standards discussion is continuing, this is intentionally conservative. Spec (not yet updated): https://drafts.fxtf.org/motion-1/#offset-shorthand BUG=638055 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== CSS Motion Path: offset-shorthand requires path before distance rotation We require offset-path to appear first, then offset-distance and offset-rotation. Standards discussion is continuing, this is intentionally conservative. Spec (not yet updated): https://drafts.fxtf.org/motion-1/#offset-shorthand BUG=638055 ========== to ========== CSS Motion Path: offset-shorthand requires path before distance rotation We require offset-path to appear first, then offset-distance and offset-rotation. Standards discussion is continuing, this is intentionally conservative. Spec (not yet updated): https://drafts.fxtf.org/motion-1/#offset-shorthand BUG=638055 Committed: https://crrev.com/96a59f8297eaff8ff8f70f35ec489d7f47e0e6c8 Cr-Commit-Position: refs/heads/master@{#423059} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/96a59f8297eaff8ff8f70f35ec489d7f47e0e6c8 Cr-Commit-Position: refs/heads/master@{#423059}
Message was sent while issue was closed.
On 2016/10/04 10:42:48, shans wrote: ... > That was Tab's quick stab at a grammar and I think it's wrong. I *think* you > want: > > <offset-position>? [ <offset-path> [ <offset-rotation> || <offset-distance> ]? > ]? [ / <offset-anchor> ]? > > i.e. the following constructions (and only the following constructions) are OK: > (null) > ... An empty string can't be set via CSSStyleDeclaration object: https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-setpropertyvalue explains how removeProperty() is invoked when an empty string is supplied. https://github.com/w3c/fxtf-drafts/pull/52 has my counter-proposal. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
