Add support for unprefixed CSS Transforms

Add support for unprefixed CSS Transforms

Adds support for the following CSS properties as defined by css-transforms[1]:

(Note, these properties are only enabled under the experimental feature set
as guarded by the CSSTransformsUnprefixed runtime feature)

transform: Unlike -webkit-transform, does not allow the perspective()
function to contain numbers without units. Like -webkit-transform and other
user agents, the resolved value of transform is not the computed value as
currently defined by css-transitions[2].

transform-origin: Unlike -webkit-transform-origin, transform-origin is not a
shorthand. New parsing and serialization (mostly matching Gecko) has been
introduced.

transform-style: Behaves as -webkit-transform-style.

perspective: Unlike -webkit-perspective, does not allow numbers without units.

perspective-origin: Unlike -webkit-perspective-origin, perspective-origin is
not a shorthand. New parsing and serialization (mostly matching Gecko) has
been introduced.

backface-visibility: Behaves as -webkit-backface-visibility.


Special attention is paid in the animation system to avoid running animations
and transitions on both the prefixed and unprefixed variants.

Transitions now track the specified transition-property such that the end
event may reference it. (eg. transition: -webkit-transform; will fire an end
event with property == '-webkit-transform'). As transitions operate on changes
to computed values, 'transition: all;' only ever starts transitions on the
unprefixed variants.

Under this feature, support for independent animation/transition of
-webkit-transform-origin-* is removed. The X, Y and Z coordinates now always
transition together.

[1] http://www.w3.org/TR/css3-transforms/
[2] http://lists.w3.org/Archives/Public/www-style/2013Dec/0317.html

BUG=154772

[dstockwell, 2014-03-19 05:53:04]

Hi Eric, please add other reviewers as appropriate.

This change looks big, but the bulk is new tests and parsing for
transform-origin, perspective origin.

[dstockwell, 2014-03-19 05:53:16]

Hi Eric, please add other reviewers as appropriate.

This change looks big, but the bulk is new tests and parsing for
transform-origin, perspective origin.

[eseidel, 2014-03-20 16:40:56]

https://codereview.chromium.org/98663004/diff/340048/Source/core/animation/Ac...
File Source/core/animation/ActiveAnimations.cpp (right):

https://codereview.chromium.org/98663004/diff/340048/Source/core/animation/Ac...
Source/core/animation/ActiveAnimations.cpp:46: ||
activeAnimations->hasActiveAnimations(CSSPropertyTransform)
I see Transform, but not WebKitTransform here, is that intentional?

[Julien - ping for review, 2014-03-21 03:57:24]

This is a big change, maybe we could proceed a few properties at a time to ease
review as it's hard to keep the patch in one's head for review.

https://codereview.chromium.org/98663004/diff/340048/Source/core/animation/cs...
File Source/core/animation/css/CSSAnimations.cpp (right):

https://codereview.chromium.org/98663004/diff/340048/Source/core/animation/cs...
Source/core/animation/css/CSSAnimations.cpp:98: }
This function creates a double-standard where some part of the code (anything
dealing with animations) has to only check the unprefixed one. This makes the
patch harder to review and also increases the likelihood that someone will get
it wrong down the road.

Wouldn't it make sense to just use whatever the author provides and thus remove
this split?

https://codereview.chromium.org/98663004/diff/340048/Source/core/animation/cs...
Source/core/animation/css/CSSAnimations.cpp:596: update->startTransition(id,
eventId, from.get(), to.get(), InertAnimation::create(effect, timing,
isPaused));
Correct me if I am wrong but won't this only dispatch unprefixed event as we
massage the property IDs above? Also what happens if the page has a listener for
both prefixed and unprefixed? (Alexis added some code to handle this case as
part of unprefixing animation / transition and we may want to reuse this code or
be consistent)

https://codereview.chromium.org/98663004/diff/340048/Source/core/css/StylePro...
File Source/core/css/StylePropertySerializer.cpp (right):

https://codereview.chromium.org/98663004/diff/340048/Source/core/css/StylePro...
Source/core/css/StylePropertySerializer.cpp:316: case
CSSPropertyTransformOrigin:
Wouldn't that make us return the prefixed property shorthand in this case if
both prefixed / unprefixed are defined?

https://codereview.chromium.org/98663004/diff/340048/Source/core/css/parser/C...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/98663004/diff/340048/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:7526: xValue =
cssValuePool().createValue(50, CSSPrimitiveValue::CSS_PERCENTAGE);
The code is not symmetrical for this case: I would have expected something
similar for |yValue| so that you default to the initial value if absent.

https://codereview.chromium.org/98663004/diff/340048/Source/core/css/resolver...
File Source/core/css/resolver/AnimatedStyleBuilder.cpp (left):

https://codereview.chromium.org/98663004/diff/340048/Source/core/css/resolver...
Source/core/css/resolver/AnimatedStyleBuilder.cpp:556: case
CSSPropertyWebkitPerspective:
Don't we still need to handle this value?

https://codereview.chromium.org/98663004/diff/340048/Source/core/css/resolver...
Source/core/css/resolver/AnimatedStyleBuilder.cpp:581: case
CSSPropertyWebkitTransform: {
Ditto?

[Rik, 2014-03-21 04:48:41]

There are still issues that need to be resolved before you should land this.
For instance, Simon Fraser recently made a proposal to simplify how preserve-3d
works. If this goes in, it will alter existing behavior so he wants to make that
change when removing the prefix.
Blink is also still using the old matrix decomposition logic.

[abarth-chromium, 2014-03-21 12:58:13]

On 2014/03/21 04:48:41, Rik wrote:
> There are still issues that need to be resolved before you should land this.

It's not clear to me that those are blocking issues.  Transform is already a de
facto standard because many web site spam the same information both with and
without a -webkit prefix.  If you'd like to discuss further, we should take the
discussion to blink-dev.

[abarth-chromium, 2014-03-21 12:59:55]

On 2014/03/21 03:57:24, Julien Chaffraix - PST wrote:
> This function creates a double-standard where some part of the code (anything
> dealing with animations) has to only check the unprefixed one. This makes the
> patch harder to review and also increases the likelihood that someone will get
> it wrong down the road.
> 
> Wouldn't it make sense to just use whatever the author provides and thus
remove
> this split?

The other side of this argument is that new API shouldn't accept prefixed
properties to avoid growing the amount of content in the web that doesn't work
in Firefox.  When Firefox implements Web Animations, it would be better if all
the Web Animations content that exists at that time works in Firefox.

[Mike Lawther (Google), 2014-03-21 22:08:41]

On 2014/03/21 12:59:55, abarth wrote:
> On 2014/03/21 03:57:24, Julien Chaffraix - PST wrote:
> > This function creates a double-standard where some part of the code
(anything
> > dealing with animations) has to only check the unprefixed one. This makes
the
> > patch harder to review and also increases the likelihood that someone will
get
> > it wrong down the road.
> > 
> > Wouldn't it make sense to just use whatever the author provides and thus
> remove
> > this split?
> 
> The other side of this argument is that new API shouldn't accept prefixed
> properties to avoid growing the amount of content in the web that doesn't work
> in Firefox.  When Firefox implements Web Animations, it would be better if all
> the Web Animations content that exists at that time works in Firefox

That's right. Web Animations is a new API, and we don't want to accept prefixed
properties. There is no existing content to break, and like Adam says, we don't
want to add to the content that is incompatible with other browsers.

[eseidel, 2014-03-24 17:36:59]

Can we discuss this over IRC or VC?  I'm excited for this change, but daunted by
the size of this patch.

[dstockwell, 2014-03-24 21:40:55]

On 2014/03/21 04:48:41, Rik wrote:
> There are still issues that need to be resolved before you should land this.
> For instance, Simon Fraser recently made a proposal to simplify how
preserve-3d
> works. If this goes in, it will alter existing behavior so he wants to make
that
> change when removing the prefix.
> Blink is also still using the old matrix decomposition logic.

This patch is guarded by an experimental runtime feature flag.

https://codereview.chromium.org/98663004/diff/340048/Source/core/animation/Ac...
File Source/core/animation/ActiveAnimations.cpp (right):

https://codereview.chromium.org/98663004/diff/340048/Source/core/animation/Ac...
Source/core/animation/ActiveAnimations.cpp:46: ||
activeAnimations->hasActiveAnimations(CSSPropertyTransform)
On 2014/03/20 16:40:57, eseidel wrote:
> I see Transform, but not WebKitTransform here, is that intentional?

Yes, in the animation system we only want to animate one of these at a time. So
both are mapped to CSSPropertyTransform internally.

https://codereview.chromium.org/98663004/diff/340048/Source/core/animation/cs...
File Source/core/animation/css/CSSAnimations.cpp (right):

https://codereview.chromium.org/98663004/diff/340048/Source/core/animation/cs...
Source/core/animation/css/CSSAnimations.cpp:98: }
On 2014/03/21 03:57:25, Julien Chaffraix - PST wrote:
> This function creates a double-standard where some part of the code (anything
> dealing with animations) has to only check the unprefixed one. This makes the
> patch harder to review and also increases the likelihood that someone will get
> it wrong down the road.
> 
> Wouldn't it make sense to just use whatever the author provides and thus
remove
> this split?

We don't ever want to animate both -webkit-transform and transform at the same
time. (the author might have supplied both).

Also note that this isn't a direct mapping. As noted in the description
-webkit-transform-origin-* map to transform-origin.

https://codereview.chromium.org/98663004/diff/340048/Source/core/animation/cs...
Source/core/animation/css/CSSAnimations.cpp:596: update->startTransition(id,
eventId, from.get(), to.get(), InertAnimation::create(effect, timing,
isPaused));
On 2014/03/21 03:57:25, Julien Chaffraix - PST wrote:
> Correct me if I am wrong but won't this only dispatch unprefixed event as we
> massage the property IDs above? Also what happens if the page has a listener
for
> both prefixed and unprefixed? (Alexis added some code to handle this case as
> part of unprefixing animation / transition and we may want to reuse this code
or
> be consistent)

Which listeners are called is orthogonal to the property (and this change). As
you note Alexis has already implemented event dispatch for prefixed and prefixed
handlers.

https://codereview.chromium.org/98663004/diff/340048/Source/core/css/StylePro...
File Source/core/css/StylePropertySerializer.cpp (right):

https://codereview.chromium.org/98663004/diff/340048/Source/core/css/StylePro...
Source/core/css/StylePropertySerializer.cpp:316: case
CSSPropertyTransformOrigin:
On 2014/03/21 03:57:25, Julien Chaffraix - PST wrote:
> Wouldn't that make us return the prefixed property shorthand in this case if
> both prefixed / unprefixed are defined?

I see. I'll look into removing this.

https://codereview.chromium.org/98663004/diff/340048/Source/core/css/parser/C...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/98663004/diff/340048/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:7526: xValue =
cssValuePool().createValue(50, CSSPrimitiveValue::CSS_PERCENTAGE);
On 2014/03/21 03:57:25, Julien Chaffraix - PST wrote:
> The code is not symmetrical for this case: I would have expected something
> similar for |yValue| so that you default to the initial value if absent.

This method fills outputs the x,y,z parts in order, but only as many as are
required to match what was input.

x -> x
y -> x y
x y -> x y

https://codereview.chromium.org/98663004/diff/340048/Source/core/css/resolver...
File Source/core/css/resolver/AnimatedStyleBuilder.cpp (left):

https://codereview.chromium.org/98663004/diff/340048/Source/core/css/resolver...
Source/core/css/resolver/AnimatedStyleBuilder.cpp:556: case
CSSPropertyWebkitPerspective:
On 2014/03/21 03:57:25, Julien Chaffraix - PST wrote:
> Don't we still need to handle this value?

No. In the animation system to avoid animating such properties twice we map them
to the unprefixed variant.

https://codereview.chromium.org/98663004/diff/340048/Source/core/css/resolver...
Source/core/css/resolver/AnimatedStyleBuilder.cpp:581: case
CSSPropertyWebkitTransform: {
On 2014/03/21 03:57:25, Julien Chaffraix - PST wrote:
> Ditto?

As above.

[dstockwell, 2014-03-25 08:40:02]

Closing this issue in favor of splitting this patch up into smaller units.