[css-ui] Make caret-color animatable (issue 2537373005 by rego@igalia.com)

[re...@igalia.com]

Reviewers: Timothy Loh, dstockwell
CL: https://codereview.chromium.org/2537373005/

Message:

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.

Description:
[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

Affected files (+107, -2 lines):
M third_party/WebKit/LayoutTests/TestExpectations
A third_party/WebKit/LayoutTests/editing/caret/caret-color-transition.html
A third_party/WebKit/LayoutTests/editing/caret/caret-color-transition-expected.txt
A third_party/WebKit/LayoutTests/imported/csswg-test/css-ui-3/caret-color-019.html
M third_party/WebKit/Source/core/animation/ColorPropertyFunctions.cpp
M third_party/WebKit/Source/core/animation/PropertyInterpolationTypesMapping.cpp
M third_party/WebKit/Source/core/animation/css/CSSAnimatableValueFactory.cpp
M third_party/WebKit/Source/core/css/CSSProperties.in
M third_party/WebKit/Source/core/css/resolver/AnimatedStyleBuilder.cpp

[tim...@chromium.org]

+alancutter from animations team

https://codereview.chromium.org/2537373005/

[alanc...@chromium.org]

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

https://codereview.chromium.org/2537373005/diff/1/third_party/WebKit/LayoutTests/editing/caret/caret-color-transition.html#newcode31
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().

https://codereview.chromium.org/2537373005/

[re...@igalia.com]

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!


https://codereview.chromium.org/2537373005/

[alanc...@chromium.org]

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/


https://codereview.chromium.org/2537373005/

[re...@igalia.com]

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.


https://codereview.chromium.org/2537373005/

[alanc...@chromium.org]

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

https://codereview.chromium.org/2537373005/

[re...@igalia.com]

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.

https://codereview.chromium.org/2537373005/

[re...@igalia.com]

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!

https://codereview.chromium.org/2537373005/

[alanc...@chromium.org]

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


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

https://codereview.chromium.org/2537373005/diff/80001/third_party/WebKit/Source/core/animation/CSSColorInterpolationType.cpp#newcode188
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.

https://codereview.chromium.org/2537373005/

[alanc...@chromium.org]

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

https://codereview.chromium.org/2537373005/diff/80001/third_party/WebKit/LayoutTests/animations/interpolation/caret-color-interpolation.html#newcode22
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.

https://codereview.chromium.org/2537373005/

[re...@igalia.com]

Thanks for the review!

Applied suggested changes and uploaded new version.

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

https://codereview.chromium.org/2537373005/diff/80001/third_party/WebKit/LayoutTests/animations/interpolation/caret-color-interpolation.html#newcode22
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/Source/core/animation/CSSColorInterpolationType.cpp
File
third_party/WebKit/Source/core/animation/CSSColorInterpolationType.cpp
(right):

https://codereview.chromium.org/2537373005/diff/80001/third_party/WebKit/Source/core/animation/CSSColorInterpolationType.cpp#newcode188
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.

https://codereview.chromium.org/2537373005/

[alanc...@chromium.org]

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

https://codereview.chromium.org/2537373005/diff/120001/third_party/WebKit/Source/core/animation/ColorPropertyFunctions.cpp#newcode34
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;
}

https://codereview.chromium.org/2537373005/

[re...@igalia.com]

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/Source/core/animation/ColorPropertyFunctions.cpp
File third_party/WebKit/Source/core/animation/ColorPropertyFunctions.cpp
(right):

https://codereview.chromium.org/2537373005/diff/120001/third_party/WebKit/Source/core/animation/ColorPropertyFunctions.cpp#newcode34
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).

https://codereview.chromium.org/2537373005/

[alanc...@chromium.org]

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

https://codereview.chromium.org/2537373005/diff/140001/third_party/WebKit/LayoutTests/animations/interpolation/caret-color-interpolation.html#newcode20
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/Source/core/animation/CSSColorInterpolationType.cpp
File
third_party/WebKit/Source/core/animation/CSSColorInterpolationType.cpp
(right):

https://codereview.chromium.org/2537373005/diff/140001/third_party/WebKit/Source/core/animation/CSSColorInterpolationType.cpp#newcode257
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).

https://codereview.chromium.org/2537373005/

[re...@igalia.com]

@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/Source/core/animation/CSSColorInterpolationType.cpp
File
third_party/WebKit/Source/core/animation/CSSColorInterpolationType.cpp
(right):

https://codereview.chromium.org/2537373005/diff/140001/third_party/WebKit/Source/core/animation/CSSColorInterpolationType.cpp#newcode257
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.

https://codereview.chromium.org/2537373005/

[alanc...@chromium.org]

Awesome, thanks for all your work!

lgtm


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

https://codereview.chromium.org/2537373005/diff/160001/third_party/WebKit/Source/core/animation/ColorPropertyFunctions.cpp#newcode14
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.

https://codereview.chromium.org/2537373005/

[re...@igalia.com]

Thanks for the detailed reviews!

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

https://codereview.chromium.org/2537373005/diff/160001/third_party/WebKit/Source/core/animation/ColorPropertyFunctions.cpp#newcode14
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.

https://codereview.chromium.org/2537373005/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2537373005/180001


https://codereview.chromium.org/2537373005/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #10 (id:180001)

https://codereview.chromium.org/2537373005/

[commit-bot@chromium.org via codereview.chromium.org]

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

https://codereview.chromium.org/2537373005/