[css-ui] Make caret-color animatable

[css-ui] Make caret-color animatable

This patch makes caret-color an animatable property.
The only special part is becuase "auto", the initial value of
this property, is not interpolable, so we need to do some changes in
CSSAnimatableValueFactory::create() and ColorPropertyFunctions.
Also this patch modifies StyleAutoColor, so toStyleColor()
can only be called for non "auto" colors.

Adds 2 new tests (caret-color-composition.html and
caret-color-interpolation.html) to verify that
the interpolation works as expected.
Also the animations tests (caret-color-018.html and
caret-color-020.html) from the W3C suite are passing now.
Marks caret-color-021.html as failing because of the TODOs
on the patch (see http://crbug.com/676295).

BUG=669490
TEST=animations/composition/caret-color-composition.html
TEST=animations/interpolation/caret-color-interpolation.html
TEST=imported/csswg-test/css-ui-3/caret-color-018.html
TEST=imported/csswg-test/css-ui-3/caret-color-019.html
TEST=imported/csswg-test/css-ui-3/caret-color-020.html

Committed: https://crrev.com/3ddca0e44883c2ae38bcc45b48853d0db31e9691
Cr-Commit-Position: refs/heads/master@{#440377}

[Manuel Rego, 2016-12-01 00:22:29]

rego@igalia.com changed reviewers:
+ dstockwell@chromium.org, timloh@chromium.org

[Manuel Rego, 2016-12-01 00:22:30]

This patch is pretty simple, but it has a difference
with a regular color due to the "auto" value in caret-color.

Patch caret-color-019.html (the one checking that "auto"
is not interpolable) will be upstreamed to the CSS WG suite.
I'm not doing it right now, just in case we decide to have
a different behavior for caret-color, something similar
to what happens on the rest of color properties.

For example, with the current patch the following CSS
will cause that the color is gradually changed to "red",
but the caret-color is not animated:
  input { transition: color 2s, caret-color 2s; }
  input:focus { color: red; caret-color: red; }

I guess that it's not a big issue anyway, and we can deal
with this special behavior for caret-color in animations.

@florian was open to update the spec if we can think on
something to solve this issue.

[Timothy Loh, 2016-12-01 02:00:45]

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

[Timothy Loh, 2016-12-01 02:00:46]

+alancutter from animations team

[alancutter (OOO until 2018), 2016-12-01 02:05:31]

https://codereview.chromium.org/2537373005/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/editing/caret/caret-color-transition.html
(right):

https://codereview.chromium.org/2537373005/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/editing/caret/caret-color-transition.html:31:
runTransitionTest(expectedValues, setupTest);
Please add a test under animations/interpolation using the same style as other
tests in that directory rather than using runTransitionTest().

[Manuel Rego, 2016-12-01 10:16:54]

Hi @alancutter, thanks for the review.

Ok, I'll create one of those tests and remove the other one.
But I'm not sure what's going to be the final behavior from the spec.

Check this comment from @florian on the bug (crbug.com/669490):
> Hi. Spec editor there. The fact that you can both have `auto`
> compute to itself as a keyword and still have it animate
> is an interesting possibility that I had not considered.
>
> I've opened https://github.com/w3c/csswg-drafts/issues/781
> to discuss with the CSS Working Group.

I'm not an expert on transitions and animations, so I'm not sure
if that could make any sense or not, and if it's possible
to implement it.
Would be really nice if you could provide your feedback
here or in the GitHub issue directly. Thanks!

[alancutter (OOO until 2018), 2016-12-04 22:33:44]

On 2016/12/01 at 10:16:54, rego wrote:
> Hi @alancutter, thanks for the review.
> 
> Ok, I'll create one of those tests and remove the other one.
> But I'm not sure what's going to be the final behavior from the spec.
> 
> Check this comment from @florian on the bug (crbug.com/669490):
> > Hi. Spec editor there. The fact that you can both have `auto`
> > compute to itself as a keyword and still have it animate
> > is an interesting possibility that I had not considered.
> >
> > I've opened https://github.com/w3c/csswg-drafts/issues/781
> > to discuss with the CSS Working Group.
> 
> I'm not an expert on transitions and animations, so I'm not sure
> if that could make any sense or not, and if it's possible
> to implement it.
> Would be really nice if you could provide your feedback
> here or in the GitHub issue directly. Thanks!

This patch will not interpolate auto but will interpolate currentColor. If you
want to avoid interpolating currentColor you'll need to modify
CSSColorInterpolationType.

I'm not aware of any spec text that suggests currentColor should interpolate
with other colours but AFAIK all browsers support it anyway as legacy behaviour.
I would seek out Tab's opinions on CSS colours: http://www.xanthir.com/contact/

[Manuel Rego, 2016-12-14 15:50:24]

So it seems the things have been clarified on the CSS WG issue:
https://github.com/w3c/csswg-drafts/issues/781

Regarding the resolved value of caret-color,
that will be fixed in a different CL:
https://codereview.chromium.org/2577633002/

Regarding animations stuff, "auto" won't be interpolable;
but "currentcolor" will be interpolable
(as it's for any other color property).

I've added the suggested test:
animations/interpolation/caret-color-interpolation.html

However it's failing for the transitions case:
FAIL CSS Transitions: property <caret-color> from [initial] to [green] at (-0.3)
is [rgb(0, 128, 0)] assert_equals: expected "rgb ( 0 , 0 , 0 ) " but got "rgb (
0 , 128 , 0 ) "
FAIL CSS Transitions: property <caret-color> from [initial] to [green] at (0) is
[rgb(0, 128, 0)] assert_equals: expected "rgb ( 0 , 0 , 0 ) " but got "rgb ( 0 ,
128 , 0 ) "
FAIL CSS Transitions: property <caret-color> from [initial] to [green] at (0.3)
is [rgb(0, 128, 0)] assert_equals: expected "rgb ( 0 , 0 , 0 ) " but got "rgb (
0 , 128 , 0 ) "
PASS CSS Transitions: property <caret-color> from [initial] to [green] at (0.6)
is [rgb(0, 128, 0)] 
PASS CSS Transitions: property <caret-color> from [initial] to [green] at (1) is
[rgb(0, 128, 0)] 
PASS CSS Transitions: property <caret-color> from [initial] to [green] at (1.5)
is [rgb(0, 128, 0)] 
FAIL CSS Transitions: property <caret-color> from [auto] to [green] at (-0.3) is
[rgb(0, 128, 0)] assert_equals: expected "rgb ( 0 , 0 , 0 ) " but got "rgb ( 0 ,
128 , 0 ) "
FAIL CSS Transitions: property <caret-color> from [auto] to [green] at (0) is
[rgb(0, 128, 0)] assert_equals: expected "rgb ( 0 , 0 , 0 ) " but got "rgb ( 0 ,
128 , 0 ) "
FAIL CSS Transitions: property <caret-color> from [auto] to [green] at (0.3) is
[rgb(0, 128, 0)] assert_equals: expected "rgb ( 0 , 0 , 0 ) " but got "rgb ( 0 ,
128 , 0 ) "
PASS CSS Transitions: property <caret-color> from [auto] to [green] at (0.6) is
[rgb(0, 128, 0)] 
PASS CSS Transitions: property <caret-color> from [auto] to [green] at (1) is
[rgb(0, 128, 0)] 
PASS CSS Transitions: property <caret-color> from [auto] to [green] at (1.5) is
[rgb(0, 128, 0)] 

I guess we need to do something special regarding
the initial value (which is "auto") for transitions.
Any pointer on what should I change is really welcomed.

[Manuel Rego, 2016-12-14 15:51:07]

Description was changed from

==========
[css-ui] Make caret-color animatable

This patch makes caret-color an animatable property.
The only special part is becuase "auto" and "currentColor"
computed styles are "auto" and "currentColor". That makes them
not interpolable. So we need to do some extra checks in
CSSAnimatableValueFactory::create().

Add a new test (caret-color-transition.html) to verify that transitions
work as expected.
Also the animations test (caret-color-018.html) from the W3C suite
is passing now.
Created a new test (caret-color-019.html) to verify that
the caret-color doesn't interpolate when "auto" value is used.

BUG=669490
TEST=editing/caret/caret-color-transition.html
TEST=imported/csswg-test/css-ui-3/caret-color-018.html
TEST=imported/csswg-test/css-ui-3/caret-color-019.html
==========

to

==========
[css-ui] Make caret-color animatable

This patch makes caret-color an animatable property.
The only special part is becuase "auto", the initial value of
this property, is not interpolable, so we need to do some changes in
CSSAnimatableValueFactory::create() and
CSSColorInterpolationType::maybeConvertInitial().

Adds a new test (caret-color-interpolation.html) to verify that
the interpolation works as expected.
Also the animations test (caret-color-018.html) from the W3C suite
is passing now.
Creates two new tests (contributed upstream):
* caret-color-019.html: Verifies that caret-color doesn't interpolate
  when "auto" value is used.
* caret-color-020.html: Verifies that caret-color interpolates
  when "currentcolor" value is used.

BUG=669490
TEST=animations/interpolation/caret-color-interpolation.html
TEST=imported/csswg-test/css-ui-3/caret-color-018.html
TEST=imported/csswg-test/css-ui-3/caret-color-019.html
TEST=imported/csswg-test/css-ui-3/caret-color-020.html
==========

[alancutter (OOO until 2018), 2016-12-14 23:44:31]

On 2016/12/14 at 15:50:24, rego wrote:
> So it seems the things have been clarified on the CSS WG issue:
> https://github.com/w3c/csswg-drafts/issues/781
> 
> Regarding the resolved value of caret-color,
> that will be fixed in a different CL:
> https://codereview.chromium.org/2577633002/
> 
> Regarding animations stuff, "auto" won't be interpolable;
> but "currentcolor" will be interpolable
> (as it's for any other color property).
> 
> I've added the suggested test:
> animations/interpolation/caret-color-interpolation.html
> 
> However it's failing for the transitions case:
> FAIL CSS Transitions: property <caret-color> from [initial] to [green] at
(-0.3) is [rgb(0, 128, 0)] assert_equals: expected "rgb ( 0 , 0 , 0 ) " but got
"rgb ( 0 , 128 , 0 ) "
> FAIL CSS Transitions: property <caret-color> from [initial] to [green] at (0)
is [rgb(0, 128, 0)] assert_equals: expected "rgb ( 0 , 0 , 0 ) " but got "rgb (
0 , 128 , 0 ) "
> FAIL CSS Transitions: property <caret-color> from [initial] to [green] at
(0.3) is [rgb(0, 128, 0)] assert_equals: expected "rgb ( 0 , 0 , 0 ) " but got
"rgb ( 0 , 128 , 0 ) "
> PASS CSS Transitions: property <caret-color> from [initial] to [green] at
(0.6) is [rgb(0, 128, 0)] 
> PASS CSS Transitions: property <caret-color> from [initial] to [green] at (1)
is [rgb(0, 128, 0)] 
> PASS CSS Transitions: property <caret-color> from [initial] to [green] at
(1.5) is [rgb(0, 128, 0)] 
> FAIL CSS Transitions: property <caret-color> from [auto] to [green] at (-0.3)
is [rgb(0, 128, 0)] assert_equals: expected "rgb ( 0 , 0 , 0 ) " but got "rgb (
0 , 128 , 0 ) "
> FAIL CSS Transitions: property <caret-color> from [auto] to [green] at (0) is
[rgb(0, 128, 0)] assert_equals: expected "rgb ( 0 , 0 , 0 ) " but got "rgb ( 0 ,
128 , 0 ) "
> FAIL CSS Transitions: property <caret-color> from [auto] to [green] at (0.3)
is [rgb(0, 128, 0)] assert_equals: expected "rgb ( 0 , 0 , 0 ) " but got "rgb (
0 , 128 , 0 ) "
> PASS CSS Transitions: property <caret-color> from [auto] to [green] at (0.6)
is [rgb(0, 128, 0)] 
> PASS CSS Transitions: property <caret-color> from [auto] to [green] at (1) is
[rgb(0, 128, 0)] 
> PASS CSS Transitions: property <caret-color> from [auto] to [green] at (1.5)
is [rgb(0, 128, 0)] 
> 
> I guess we need to do something special regarding
> the initial value (which is "auto") for transitions.
> Any pointer on what should I change is really welcomed.

Use assertNoInterpolation(), that will handle transitions' differing behaviour.

[Manuel Rego, 2016-12-15 11:12:22]

On 2016/12/14 23:44:31, alancutter wrote:
> Use assertNoInterpolation(), that will handle transitions' differing
behaviour.

Thanks @alancutter, this indeed fixed the test.

Uploaded new version for review.

[Manuel Rego, 2016-12-15 17:34:33]

The tests caret-color-019.html and caret-color-020.html
have been accepted and merged at csswg-tests repo.

I've just updated them in this patch, so I believe
this is ready for review.

PTAL, thanks!

[alancutter (OOO until 2018), 2016-12-15 22:27:11]

You'll need to update CSSInterpolationTypesMap to include this property.

https://codereview.chromium.org/2537373005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/CSSColorInterpolationType.cpp
(right):

https://codereview.chromium.org/2537373005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/CSSColorInterpolationType.cpp:188:
return nullptr;
Make getInitialColor() return a bool instead of this if. We try to avoid having
property specific logic in the InterpolationType classes.

[alancutter (OOO until 2018), 2016-12-15 22:31:15]

https://codereview.chromium.org/2537373005/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/animations/interpolation/caret-color-interpolation.html
(right):

https://codereview.chromium.org/2537373005/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/animations/interpolation/caret-color-interpolation.html:22:
from: neutralKeyframe,
We should have a test that uses an underlying value of auto. A composition test
(LayoutTests/animations/composition) would be ideal here.

[Manuel Rego, 2016-12-19 12:28:31]

Thanks for the review!

Applied suggested changes and uploaded new version.

https://codereview.chromium.org/2537373005/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/animations/interpolation/caret-color-interpolation.html
(right):

https://codereview.chromium.org/2537373005/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/animations/interpolation/caret-color-interpolation.html:22:
from: neutralKeyframe,
On 2016/12/15 22:31:15, alancutter wrote:
> We should have a test that uses an underlying value of auto. A composition
test
> (LayoutTests/animations/composition) would be ideal here.

Created a composition test too, but I'm not 100% sure is testing
what you were asking for.

https://codereview.chromium.org/2537373005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/CSSColorInterpolationType.cpp
(right):

https://codereview.chromium.org/2537373005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/CSSColorInterpolationType.cpp:188:
return nullptr;
On 2016/12/15 22:27:11, alancutter wrote:
> Make getInitialColor() return a bool instead of this if. We try to avoid
having
> property specific logic in the InterpolationType classes.

Done.

[Manuel Rego, 2016-12-19 12:29:43]

Description was changed from

==========
[css-ui] Make caret-color animatable

This patch makes caret-color an animatable property.
The only special part is becuase "auto", the initial value of
this property, is not interpolable, so we need to do some changes in
CSSAnimatableValueFactory::create() and
CSSColorInterpolationType::maybeConvertInitial().

Adds a new test (caret-color-interpolation.html) to verify that
the interpolation works as expected.
Also the animations test (caret-color-018.html) from the W3C suite
is passing now.
Creates two new tests (contributed upstream):
* caret-color-019.html: Verifies that caret-color doesn't interpolate
  when "auto" value is used.
* caret-color-020.html: Verifies that caret-color interpolates
  when "currentcolor" value is used.

BUG=669490
TEST=animations/interpolation/caret-color-interpolation.html
TEST=imported/csswg-test/css-ui-3/caret-color-018.html
TEST=imported/csswg-test/css-ui-3/caret-color-019.html
TEST=imported/csswg-test/css-ui-3/caret-color-020.html
==========

to

==========
[css-ui] Make caret-color animatable

This patch makes caret-color an animatable property.
The only special part is becuase "auto", the initial value of
this property, is not interpolable, so we need to do some changes in
CSSAnimatableValueFactory::create() and
CSSColorInterpolationType::maybeConvertInitial().

Adds 2 new tests (caret-color-composition.html and
caret-color-interpolation.html) to verify that
the interpolation works as expected.
Also the animations tests (caret-color-018.html and
caret-color-020.html) from the W3C suite are passing now.

BUG=669490
TEST=animations/composition/caret-color-composition.html
TEST=animations/interpolation/caret-color-interpolation.html
TEST=imported/csswg-test/css-ui-3/caret-color-018.html
TEST=imported/csswg-test/css-ui-3/caret-color-019.html
TEST=imported/csswg-test/css-ui-3/caret-color-020.html
==========

[alancutter (OOO until 2018), 2016-12-20 05:45:53]

https://codereview.chromium.org/2537373005/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/animation/ColorPropertyFunctions.cpp
(right):

https://codereview.chromium.org/2537373005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/ColorPropertyFunctions.cpp:34: return
style.caretColor().toStyleColor();
This seems wrong. You shouldn't be able to read a StyleColor when caret-color is
set to auto. toStyleColor() should DCHECK(!isAutoColor()) and this function
should return false.

Here's a CSS animation that hits this scenario:
@keyframes test {
  to { caret-color: blue; }
}
#target {
  animation: test 1s;
  caret-color: auto;
}

[Manuel Rego, 2016-12-20 17:18:17]

Thanks for the review, I've applied the suggested changes.

However I've issues now in caret-color-interpolation.html,
but only in the transitions case (not in animations).
I'm sure I'm missing something else, but I don't know what yet.
Current output is:
FAIL CSS Transitions: property <caret-color> from neutral to [green] at (-0.3)
is [rgb(255, 255, 0)] assert_equals: expected "rgb ( 0 , 128 , 0 ) " but got
"rgb ( 255 , 255 , 0 ) "
FAIL CSS Transitions: property <caret-color> from neutral to [green] at (0) is
[rgb(255, 255, 0)] assert_equals: expected "rgb ( 0 , 128 , 0 ) " but got "rgb (
255 , 255 , 0 ) "
FAIL CSS Transitions: property <caret-color> from neutral to [green] at (0.3) is
[rgb(179, 217, 0)] assert_equals: expected "rgb ( 0 , 128 , 0 ) " but got "rgb (
179 , 217 , 0 ) "
FAIL CSS Transitions: property <caret-color> from neutral to [green] at (0.5) is
[rgb(128, 192, 0)] assert_equals: expected "rgb ( 0 , 128 , 0 ) " but got "rgb (
128 , 192 , 0 ) "
FAIL CSS Transitions: property <caret-color> from neutral to [green] at (0.6) is
[rgb(102, 179, 0)] assert_equals: expected "rgb ( 0 , 128 , 0 ) " but got "rgb (
102 , 179 , 0 ) "
PASS CSS Transitions: property <caret-color> from neutral to [green] at (1) is
[rgb(0, 128, 0)] 
FAIL CSS Transitions: property <caret-color> from neutral to [green] at (1.5) is
[rgb(0, 65, 0)] assert_equals: expected "rgb ( 0 , 128 , 0 ) " but got "rgb ( 0
, 65 , 0 ) "

https://codereview.chromium.org/2537373005/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/animation/ColorPropertyFunctions.cpp
(right):

https://codereview.chromium.org/2537373005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/ColorPropertyFunctions.cpp:34: return
style.caretColor().toStyleColor();
On 2016/12/20 05:45:53, alancutter wrote:
> This seems wrong. You shouldn't be able to read a StyleColor when caret-color
is
> set to auto. toStyleColor() should DCHECK(!isAutoColor()) and this function
> should return false.

Good point, I've changed all this.

> Here's a CSS animation that hits this scenario:
> @keyframes test {
>   to { caret-color: blue; }
> }
> #target {
>   animation: test 1s;
>   caret-color: auto;
> }

Now this animation doesn't interpolate (as expected).

[alancutter (OOO until 2018), 2016-12-21 00:49:05]

https://codereview.chromium.org/2537373005/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/animations/interpolation/caret-color-interpolation.html
(right):

https://codereview.chromium.org/2537373005/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/animations/interpolation/caret-color-interpolation.html:20:
assertNoInterpolation({
The underlying value is yellow so it should be interpolating from yellow to
green.

https://codereview.chromium.org/2537373005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/animation/CSSColorInterpolationType.cpp
(right):

https://codereview.chromium.org/2537373005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/CSSColorInterpolationType.cpp:257:
return nullptr;
I see a problem here. If either the visited or unvisited caret-color are auto
then interpolation does not occur.
Unfortunately the weird behaviour of color properties actually being two
properties doesn't work nicely with the InterpolationType design. I think this
behavioural issue is corner case enough that we can leave this issue to another
patch, I'm happy to take it on as it's requires non-trivial changes to
ColorInterpolationType to resolve.

For now make getVisitedColor() return StyleColor instead of bool and return
currentcolor if caret-color is auto. That should resolve the issue in
caret-color-interpolation.html.

Create a failing test case with a TODO that points to a bug assigned to me.
<style>
@keyframes test {
  to { caret-color: green; }
}
#target {
  color: red;
  caret-color: blue;
  animation: test 1s paused;
}
#target:visited {
  caret-color: auto;
}
</style>
<a href=""><textarea id="target">The caret-color should not be
red.</textarea></a>

(I'm not sure how you do ref tests for caret-color, please correct my attempt
here).

[Manuel Rego, 2016-12-21 13:14:38]

@alancutter thanks for all your help on this,
really appreciated. :-)

Uploaded a new version, PTAL.

https://codereview.chromium.org/2537373005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/animation/CSSColorInterpolationType.cpp
(right):

https://codereview.chromium.org/2537373005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/CSSColorInterpolationType.cpp:257:
return nullptr;
On 2016/12/21 00:49:05, alancutter wrote:
> I see a problem here. If either the visited or unvisited caret-color are auto
> then interpolation does not occur.
> Unfortunately the weird behaviour of color properties actually being two
> properties doesn't work nicely with the InterpolationType design. I think this
> behavioural issue is corner case enough that we can leave this issue to
another
> patch, I'm happy to take it on as it's requires non-trivial changes to
> ColorInterpolationType to resolve.

Wow, it's clear this is getting more complex. :-)

> For now make getVisitedColor() return StyleColor instead of bool and return
> currentcolor if caret-color is auto. That should resolve the issue in
> caret-color-interpolation.html.

Ok, I've done it and yeah "caret-color-interpolation.html" passes now.

> Create a failing test case with a TODO that points to a bug assigned to me.
> <style>
> @keyframes test {
>   to { caret-color: green; }
> }
> #target {
>   color: red;
>   caret-color: blue;
>   animation: test 1s paused;
> }
> #target:visited {
>   caret-color: auto;
> }
> </style>
> <a href=""><textarea id="target">The caret-color should not be
> red.</textarea></a>
> 
> (I'm not sure how you do ref tests for caret-color, please correct my attempt
> here).

I think it'd be better to have testharness tests if possible
for these cases.
I've create a simple one caret-color-021.html that is not passing
with the last version of this patch.

[alancutter (OOO until 2018), 2016-12-21 23:24:03]

Awesome, thanks for all your work!

lgtm

https://codereview.chromium.org/2537373005/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/animation/ColorPropertyFunctions.cpp
(right):

https://codereview.chromium.org/2537373005/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/ColorPropertyFunctions.cpp:14: return
false;
Please add a TODO to make getUnvisitedColor() return bool so we don't need to
special case caret-color here.

[Manuel Rego, 2016-12-22 09:02:29]

Description was changed from

==========
[css-ui] Make caret-color animatable

This patch makes caret-color an animatable property.
The only special part is becuase "auto", the initial value of
this property, is not interpolable, so we need to do some changes in
CSSAnimatableValueFactory::create() and
CSSColorInterpolationType::maybeConvertInitial().

Adds 2 new tests (caret-color-composition.html and
caret-color-interpolation.html) to verify that
the interpolation works as expected.
Also the animations tests (caret-color-018.html and
caret-color-020.html) from the W3C suite are passing now.

BUG=669490
TEST=animations/composition/caret-color-composition.html
TEST=animations/interpolation/caret-color-interpolation.html
TEST=imported/csswg-test/css-ui-3/caret-color-018.html
TEST=imported/csswg-test/css-ui-3/caret-color-019.html
TEST=imported/csswg-test/css-ui-3/caret-color-020.html
==========

to

==========
[css-ui] Make caret-color animatable

This patch makes caret-color an animatable property.
The only special part is becuase "auto", the initial value of
this property, is not interpolable, so we need to do some changes in
CSSAnimatableValueFactory::create() and ColorPropertyFunctions.
Also this patch modifies StyleAutoColor, so toStyleColor()
can only be called for non "auto" colors.

Adds 2 new tests (caret-color-composition.html and
caret-color-interpolation.html) to verify that
the interpolation works as expected.
Also the animations tests (caret-color-018.html and
caret-color-020.html) from the W3C suite are passing now.
Marks caret-color-021.html as failing because of the TODOs
on the patch (see http://crbug.com/676295).

BUG=669490
TEST=animations/composition/caret-color-composition.html
TEST=animations/interpolation/caret-color-interpolation.html
TEST=imported/csswg-test/css-ui-3/caret-color-018.html
TEST=imported/csswg-test/css-ui-3/caret-color-019.html
TEST=imported/csswg-test/css-ui-3/caret-color-020.html
==========

[Manuel Rego, 2016-12-22 09:03:01]

Thanks for the detailed reviews!

https://codereview.chromium.org/2537373005/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/animation/ColorPropertyFunctions.cpp
(right):

https://codereview.chromium.org/2537373005/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/ColorPropertyFunctions.cpp:14: return
false;
On 2016/12/21 23:24:03, alancutter wrote:
> Please add a TODO to make getUnvisitedColor() return bool so we don't need to
> special case caret-color here.

Done.

[Manuel Rego, 2016-12-22 09:03:07]

The CQ bit was checked by rego@igalia.com

[Manuel Rego, 2016-12-22 09:03:07]

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/2537373005/#ps180001
(title: "Patch for landing adding TODO")

[commit-bot: I haz the power, 2016-12-22 09:03:18]

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-12-22 10:42:17]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1482397387307020,
"parent_rev": "df0c51e0c69376693647323e6f6c88459cf445c7", "commit_rev":
"1a825c935a83a2eee0aec6f43c4d896ed7ceaeeb"}

[commit-bot: I haz the power, 2016-12-22 10:42:43]

Description was changed from

==========
[css-ui] Make caret-color animatable

This patch makes caret-color an animatable property.
The only special part is becuase "auto", the initial value of
this property, is not interpolable, so we need to do some changes in
CSSAnimatableValueFactory::create() and ColorPropertyFunctions.
Also this patch modifies StyleAutoColor, so toStyleColor()
can only be called for non "auto" colors.

Adds 2 new tests (caret-color-composition.html and
caret-color-interpolation.html) to verify that
the interpolation works as expected.
Also the animations tests (caret-color-018.html and
caret-color-020.html) from the W3C suite are passing now.
Marks caret-color-021.html as failing because of the TODOs
on the patch (see http://crbug.com/676295).

BUG=669490
TEST=animations/composition/caret-color-composition.html
TEST=animations/interpolation/caret-color-interpolation.html
TEST=imported/csswg-test/css-ui-3/caret-color-018.html
TEST=imported/csswg-test/css-ui-3/caret-color-019.html
TEST=imported/csswg-test/css-ui-3/caret-color-020.html
==========

to

==========
[css-ui] Make caret-color animatable

This patch makes caret-color an animatable property.
The only special part is becuase "auto", the initial value of
this property, is not interpolable, so we need to do some changes in
CSSAnimatableValueFactory::create() and ColorPropertyFunctions.
Also this patch modifies StyleAutoColor, so toStyleColor()
can only be called for non "auto" colors.

Adds 2 new tests (caret-color-composition.html and
caret-color-interpolation.html) to verify that
the interpolation works as expected.
Also the animations tests (caret-color-018.html and
caret-color-020.html) from the W3C suite are passing now.
Marks caret-color-021.html as failing because of the TODOs
on the patch (see http://crbug.com/676295).

BUG=669490
TEST=animations/composition/caret-color-composition.html
TEST=animations/interpolation/caret-color-interpolation.html
TEST=imported/csswg-test/css-ui-3/caret-color-018.html
TEST=imported/csswg-test/css-ui-3/caret-color-019.html
TEST=imported/csswg-test/css-ui-3/caret-color-020.html

Review-Url: https://codereview.chromium.org/2537373005
==========

[commit-bot: I haz the power, 2016-12-22 10:42:44]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-12-22 10:45:03]

Description was changed from

==========
[css-ui] Make caret-color animatable

This patch makes caret-color an animatable property.
The only special part is becuase "auto", the initial value of
this property, is not interpolable, so we need to do some changes in
CSSAnimatableValueFactory::create() and ColorPropertyFunctions.
Also this patch modifies StyleAutoColor, so toStyleColor()
can only be called for non "auto" colors.

Adds 2 new tests (caret-color-composition.html and
caret-color-interpolation.html) to verify that
the interpolation works as expected.
Also the animations tests (caret-color-018.html and
caret-color-020.html) from the W3C suite are passing now.
Marks caret-color-021.html as failing because of the TODOs
on the patch (see http://crbug.com/676295).

BUG=669490
TEST=animations/composition/caret-color-composition.html
TEST=animations/interpolation/caret-color-interpolation.html
TEST=imported/csswg-test/css-ui-3/caret-color-018.html
TEST=imported/csswg-test/css-ui-3/caret-color-019.html
TEST=imported/csswg-test/css-ui-3/caret-color-020.html

Review-Url: https://codereview.chromium.org/2537373005
==========

to

==========
[css-ui] Make caret-color animatable

This patch makes caret-color an animatable property.
The only special part is becuase "auto", the initial value of
this property, is not interpolable, so we need to do some changes in
CSSAnimatableValueFactory::create() and ColorPropertyFunctions.
Also this patch modifies StyleAutoColor, so toStyleColor()
can only be called for non "auto" colors.

Adds 2 new tests (caret-color-composition.html and
caret-color-interpolation.html) to verify that
the interpolation works as expected.
Also the animations tests (caret-color-018.html and
caret-color-020.html) from the W3C suite are passing now.
Marks caret-color-021.html as failing because of the TODOs
on the patch (see http://crbug.com/676295).

BUG=669490
TEST=animations/composition/caret-color-composition.html
TEST=animations/interpolation/caret-color-interpolation.html
TEST=imported/csswg-test/css-ui-3/caret-color-018.html
TEST=imported/csswg-test/css-ui-3/caret-color-019.html
TEST=imported/csswg-test/css-ui-3/caret-color-020.html

Committed: https://crrev.com/3ddca0e44883c2ae38bcc45b48853d0db31e9691
Cr-Commit-Position: refs/heads/master@{#440377}
==========

[commit-bot: I haz the power, 2016-12-22 10:45:04]

Patchset 10 (id:??) landed as
https://crrev.com/3ddca0e44883c2ae38bcc45b48853d0db31e9691
Cr-Commit-Position: refs/heads/master@{#440377}