[WIP] CSS Motion Path: offset shorthand ready for position and anchor

CSS Motion Path: offset shorthand ready for position and anchor

(Postponing this CL as it may not be safe to add offset-position and offset-anchor to shorthand while they are guarded by a feature flag.)

The spec for offset is being discussed in
https://drafts.fxtf.org/motion-1/#offset-shorthand
https://logs.csswg.org/irc.w3.org/css/2016-09-20/#e724237
https://github.com/w3c/fxtf-drafts/issues/42
https://logs.csswg.org/irc.w3.org/css/2016-09-20/#e724237
RESOLVED: Position before path before distance and anchor

Spec update pull request:
https://github.com/w3c/fxtf-drafts/pull/52

offset-position and offset-anchor are guarded by the experimental
web platform feature flag CSSOffsetPositionAnchor. When this flag is
not enabled, we reject any offset value that would set offset-position
or offset-anchor.

(We about to ship offset support for offset-path, offset-distance,
offset-rotation. Any 'auto' or distance appearing in offset before
offset-path must not be interpreted as offset-rotation or
offset-distance, as in future they will be interpreted as
offset-position.)

BUG=638055

[Eric Willigers, 2016-09-25 04:48:40]

The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-25 04:48:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-25 06:18:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-25 06:18:17]

Dry run: This issue passed the CQ dry run.

[Eric Willigers, 2016-09-25 10:31:26]

The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-25 10:31:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-25 11:34:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-25 11:34:39]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Eric Willigers, 2016-09-26 00:17:17]

Description was changed from

==========
[WIP] CSS Motion Path: offset shorthand ready for position and anchor

The spec for offset is discussed in
https://drafts.fxtf.org/motion-1/#offset-shorthand
https://logs.csswg.org/irc.w3.org/css/2016-09-20/#e724237
https://github.com/w3c/fxtf-drafts/issues/42
"RESOVED: Position before path before distane and anchor"

We about to ship offset support for
<offset-path> <offset-distance> <offset-rotation>

Any 'auto' or distance before <offset-path>
should not be interpreted as <offset-rotation>
or <offset-distance>, as in future they will
be interpreted as <offset-position>

BUG=638055
==========

to

==========
CSS Motion Path: offset shorthand ready for position and anchor

The spec for offset is being discussed in
https://drafts.fxtf.org/motion-1/#offset-shorthand
https://logs.csswg.org/irc.w3.org/css/2016-09-20/#e724237
https://github.com/w3c/fxtf-drafts/issues/42
RESOLVED: Position before path before distance and anchor

offset-position and offset-anchor are guarded by the experimental web platform
feature flag CSSOffsetPositionAnchor.

When this flag is not enabled, we reject any offset value that would set
offset-position or offset-anchor.

(We about to ship offset support for
offset-path, offset-distance, offset-rotation.
Any 'auto' or distance appearing in offset
before offset-path must not be interpreted
as offset-rotation or offset-distance, as
in future they will be interpreted as
offset-position.)

BUG=638055
==========

[Eric Willigers, 2016-09-26 00:17:17]

ericwilligers@chromium.org changed reviewers:
+ timloh@chromium.org

[Eric Willigers, 2016-09-26 00:17:27]

The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 00:17:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 01:09:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 01:09:16]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Eric Willigers, 2016-09-26 03:38:50]

Description was changed from

==========
CSS Motion Path: offset shorthand ready for position and anchor

The spec for offset is being discussed in
https://drafts.fxtf.org/motion-1/#offset-shorthand
https://logs.csswg.org/irc.w3.org/css/2016-09-20/#e724237
https://github.com/w3c/fxtf-drafts/issues/42
RESOLVED: Position before path before distance and anchor

offset-position and offset-anchor are guarded by the experimental web platform
feature flag CSSOffsetPositionAnchor.

When this flag is not enabled, we reject any offset value that would set
offset-position or offset-anchor.

(We about to ship offset support for
offset-path, offset-distance, offset-rotation.
Any 'auto' or distance appearing in offset
before offset-path must not be interpreted
as offset-rotation or offset-distance, as
in future they will be interpreted as
offset-position.)

BUG=638055
==========

to

==========
CSS Motion Path: offset shorthand ready for position and anchor

The spec for offset is being discussed in
https://drafts.fxtf.org/motion-1/#offset-shorthand
https://logs.csswg.org/irc.w3.org/css/2016-09-20/#e724237
https://github.com/w3c/fxtf-drafts/issues/42
https://logs.csswg.org/irc.w3.org/css/2016-09-20/#e724237
RESOLVED: Position before path before distance and anchor

offset-position and offset-anchor are guarded by the experimental
web platform feature flag CSSOffsetPositionAnchor. When this flag is
not enabled, we reject any offset value that would set offset-position
or offset-anchor.

(We about to ship offset support for offset-path, offset-distance,
offset-rotation. Any 'auto' or distance appearing in offset before
offset-path must not be interpreted as offset-rotation or
offset-distance, as in future they will be interpreted as
offset-position.)

BUG=638055
==========

[Timothy Loh, 2016-09-27 08:17:48]

What's the timeline/plan for shipping offset, and the new properties? I'm not
sure that having longhands dependent on runtime flags is going to work correctly
when the flag is off. I suspect there'll be a few cases where things go wrong
(devtools, setting the shorthand to css-wide keywords, serialization).

https://codereview.chromium.org/2368013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/2368013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSProperties.in:522: offset
longhands=offset-position;offset-path;offset-distance;offset-rotation;offset-anchor
I don't think this works with longhands that are conditionally enabled based on
a runtime flag, this isn't really something we've done before.

https://codereview.chromium.org/2368013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.h (right):

https://codereview.chromium.org/2368013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.h:39: static
CSSValue* valueForOffset(const ComputedStyle&);
Does this need to be on the class?

https://codereview.chromium.org/2368013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/StylePropertySerializer.cpp (right):

https://codereview.chromium.org/2368013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StylePropertySerializer.cpp:550: String
StylePropertySerializer::offsetValue() const
This is going to do the wrong thing when the anchor/position properties aren't
enabled.

https://codereview.chromium.org/2368013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StylePropertySerializer.cpp:569: const
CSSValue* offsetPositionValue = offsetPositionProperty.value();
= m_propertySet.getPropertyCSSValue(CSSPropertyOffsetPosition);

etc.

https://codereview.chromium.org/2368013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/2368013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1321: const
CSSValue* offsetPosition = parseSingleValue(CSSPropertyOffsetPosition,
CSSPropertyOffset);
For all of these we should prefer just calling the property consume functions
(e.g. consumeOffsetPosition) directly.

https://codereview.chromium.org/2368013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1324: if
(!offsetPath && (!offsetPosition || !offsetPosition->isValuePair()))
What's the offsetPosition->isValuePair() check for?

https://codereview.chromium.org/2368013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1335: if
(consumeSlashIncludingWhitespace(m_range)) {
This code incorrectly allows <offset-position> / <offset-anchor>

[Timothy Loh, 2016-09-27 08:20:47]

https://codereview.chromium.org/2368013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/2368013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1324: if
(!offsetPath && (!offsetPosition || !offsetPosition->isValuePair()))
On 2016/09/27 08:17:48, Timothy Loh wrote:
> What's the offsetPosition->isValuePair() check for?

Actually, shouldn't we always fail if offsetPath isn't set? It's non-optional in
the grammar you've linked to

<offset-position>? [ <offset-path> [ <offset-rotation> || <offset-distance> ] [
/ <offset-anchor> ]? ]

[Eric Willigers, 2016-09-29 03:52:46]

On 2016/09/27 08:17:48, Timothy Loh wrote:
> What's the timeline/plan for shipping offset, and the new properties? I'm not
> sure that having longhands dependent on runtime flags is going to work
correctly
> when the flag is off. I suspect there'll be a few cases where things go wrong
> (devtools, setting the shorthand to css-wide keywords, serialization).

offset-position and offset-anchor might ship 12 weeks after the others.

So I'll hold off with this CL and for now we'll just ensure path appears first.

[Eric Willigers, 2016-09-29 03:53:19]

https://codereview.chromium.org/2368013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.h (right):

https://codereview.chromium.org/2368013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.h:39: static
CSSValue* valueForOffset(const ComputedStyle&);
On 2016/09/27 08:17:48, Timothy Loh wrote:
> Does this need to be on the class?

No, I was just being consistent with the others.

https://codereview.chromium.org/2368013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/StylePropertySerializer.cpp (right):

https://codereview.chromium.org/2368013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StylePropertySerializer.cpp:550: String
StylePropertySerializer::offsetValue() const
On 2016/09/27 08:17:48, Timothy Loh wrote:
> This is going to do the wrong thing when the anchor/position properties aren't
> enabled.

Now changed to never read them if not enabled.

https://codereview.chromium.org/2368013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StylePropertySerializer.cpp:569: const
CSSValue* offsetPositionValue = offsetPositionProperty.value();
On 2016/09/27 08:17:48, Timothy Loh wrote:
> = m_propertySet.getPropertyCSSValue(CSSPropertyOffsetPosition);
> 
> etc.

Done.

https://codereview.chromium.org/2368013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/2368013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1321: const
CSSValue* offsetPosition = parseSingleValue(CSSPropertyOffsetPosition,
CSSPropertyOffset);
On 2016/09/27 08:17:48, Timothy Loh wrote:
> For all of these we should prefer just calling the property consume functions
> (e.g. consumeOffsetPosition) directly.

Done.

https://codereview.chromium.org/2368013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1321: const
CSSValue* offsetPosition = parseSingleValue(CSSPropertyOffsetPosition,
CSSPropertyOffset);
On 2016/09/27 08:17:48, Timothy Loh wrote:
> For all of these we should prefer just calling the property consume functions
> (e.g. consumeOffsetPosition) directly.

Done.

https://codereview.chromium.org/2368013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1324: if
(!offsetPath && (!offsetPosition || !offsetPosition->isValuePair()))
On 2016/09/27 08:17:48, Timothy Loh wrote:
> What's the offsetPosition->isValuePair() check for?

Checking that we don't have 'auto'.

The offset shorthand may simply contain a position.
https://logs.csswg.org/irc.w3.org/css/2016-09-20/#e724174


This CL uses the following grammar:
https://github.com/w3c/fxtf-drafts/pull/52/files
which also appears at the bottom of
https://github.com/w3c/fxtf-drafts/issues/42

https://codereview.chromium.org/2368013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1335: if
(consumeSlashIncludingWhitespace(m_range)) {
On 2016/09/27 08:17:48, Timothy Loh wrote:
> This code incorrectly allows <offset-position> / <offset-anchor>

Intended.

[Timothy Loh, 2016-09-29 04:03:27]

The description should probably explicitly state what grammar you're
implementing since it's not obvious from the irc log you linked (I thought it
was <offset-position>? [ <offset-path> [ <offset-rotation> || <offset-distance>
] [ / <offset-anchor> ]? ])

[Eric Willigers, 2016-09-29 04:57:15]

Description was changed from

==========
CSS Motion Path: offset shorthand ready for position and anchor

The spec for offset is being discussed in
https://drafts.fxtf.org/motion-1/#offset-shorthand
https://logs.csswg.org/irc.w3.org/css/2016-09-20/#e724237
https://github.com/w3c/fxtf-drafts/issues/42
https://logs.csswg.org/irc.w3.org/css/2016-09-20/#e724237
RESOLVED: Position before path before distance and anchor

offset-position and offset-anchor are guarded by the experimental
web platform feature flag CSSOffsetPositionAnchor. When this flag is
not enabled, we reject any offset value that would set offset-position
or offset-anchor.

(We about to ship offset support for offset-path, offset-distance,
offset-rotation. Any 'auto' or distance appearing in offset before
offset-path must not be interpreted as offset-rotation or
offset-distance, as in future they will be interpreted as
offset-position.)

BUG=638055
==========

to

==========
CSS Motion Path: offset shorthand ready for position and anchor

(Postponing this CL as it may not be safe to add offset-position and
offset-anchor to shorthand while they are guarded by a feature flag.)

The spec for offset is being discussed in
https://drafts.fxtf.org/motion-1/#offset-shorthand
https://logs.csswg.org/irc.w3.org/css/2016-09-20/#e724237
https://github.com/w3c/fxtf-drafts/issues/42
https://logs.csswg.org/irc.w3.org/css/2016-09-20/#e724237
RESOLVED: Position before path before distance and anchor

Spec update pull request:
https://github.com/w3c/fxtf-drafts/pull/52

offset-position and offset-anchor are guarded by the experimental
web platform feature flag CSSOffsetPositionAnchor. When this flag is
not enabled, we reject any offset value that would set offset-position
or offset-anchor.

(We about to ship offset support for offset-path, offset-distance,
offset-rotation. Any 'auto' or distance appearing in offset before
offset-path must not be interpreted as offset-rotation or
offset-distance, as in future they will be interpreted as
offset-position.)

BUG=638055
==========

[Eric Willigers, 2016-09-29 04:57:15]

ericwilligers@chromium.org changed reviewers:
- timloh@chromium.org