[css-ui] Add support for caret-color property

[css-ui] Add support for caret-color property

The test basically adds the new CSS property "caret-color":
https://drafts.csswg.org/css-ui/#propdef-caret-color

"caret-color" is a regular color property but it accepts also "auto".
To support this a new class StyleAutoColor has been created,
including a new flag to know if the color is auto, currentColor
or a specified one.

Finally the patches modifies CaretBase::paintCaret() to use
that property instead of "color" as it's been doing until now.

We're using the test suite from the W3C CSS WG,
which was imported on r434986.
This CL manually imports some pixel tests too.

This is based on a patch by Claudio Saavedra <csaavedra@igalia.com>;.

BUG=665422
TEST=editing/caret/caret-color-*.html
TEST=imported/csswg-test/css-ui-3/caret-color-*.html

Committed: https://crrev.com/c142daf285cf004dd28c7a88c57920279d123300
Cr-Commit-Position: refs/heads/master@{#435338}

[Manuel Rego, 2016-11-21 10:04:10]

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

[Manuel Rego, 2016-11-21 10:04:11]

Adding some people as reviewers, but please let me know
if I should ping someone else. Thanks!

[Manuel Rego, 2016-11-21 10:08:38]

Description was changed from

==========
Add support for caret-color property

The test basically adds the new CSS property "caret-color":
https://drafts.csswg.org/css-ui/#propdef-caret-color

"caret-color" is a regular color property but it accepts also "auto".
To support this StyleColor has been modified, including a new flag
to know if the color is auto, currentColor or a specified one.

Finally the patches modifies CaretBase::paintCaret() to use
that property instead of "color" as it's been doing until now.

We're using the test suite from the W3C CSS WG.
This patch passes all the test but caret-color-006.html,
which was not imported as it's something optional on the spec
(background and caret-color are both "gray").
One of the tests (caret-color-015.html) needs a small modification
to work properly on Blink.

BUG=665422
TEST=editing/caret/caret-color-015.html
TEST=imported/csswg-test/css-ui-3/caret-color-001.html
==========

to

==========
Add support for caret-color property

The test basically adds the new CSS property "caret-color":
https://drafts.csswg.org/css-ui/#propdef-caret-color

"caret-color" is a regular color property but it accepts also "auto".
To support this StyleColor has been modified, including a new flag
to know if the color is auto, currentColor or a specified one.

Finally the patches modifies CaretBase::paintCaret() to use
that property instead of "color" as it's been doing until now.

We're using the test suite from the W3C CSS WG.
This patch passes all the test but caret-color-006.html,
which was not imported as it's something optional on the spec
(background and caret-color are both "gray").
One of the tests (caret-color-015.html) needs a small modification
to work properly on Blink.

BUG=665422
TEST=editing/caret/caret-color-015.html
TEST=imported/csswg-test/css-ui-3/caret-color-001.html
==========

[Manuel Rego, 2016-11-21 10:20:19]

Description was changed from

==========
Add support for caret-color property

The test basically adds the new CSS property "caret-color":
https://drafts.csswg.org/css-ui/#propdef-caret-color

"caret-color" is a regular color property but it accepts also "auto".
To support this StyleColor has been modified, including a new flag
to know if the color is auto, currentColor or a specified one.

Finally the patches modifies CaretBase::paintCaret() to use
that property instead of "color" as it's been doing until now.

We're using the test suite from the W3C CSS WG.
This patch passes all the test but caret-color-006.html,
which was not imported as it's something optional on the spec
(background and caret-color are both "gray").
One of the tests (caret-color-015.html) needs a small modification
to work properly on Blink.

BUG=665422
TEST=editing/caret/caret-color-015.html
TEST=imported/csswg-test/css-ui-3/caret-color-001.html
==========

to

==========
Add support for caret-color property

The test basically adds the new CSS property "caret-color":
https://drafts.csswg.org/css-ui/#propdef-caret-color

"caret-color" is a regular color property but it accepts also "auto".
To support this StyleColor has been modified, including a new flag
to know if the color is auto, currentColor or a specified one.

Finally the patches modifies CaretBase::paintCaret() to use
that property instead of "color" as it's been doing until now.

We're using the test suite from the W3C CSS WG.
This patch passes all the test but caret-color-006.html,
which was not imported as it's something optional on the spec
(background and caret-color are both "gray").
One of the tests (caret-color-015.html) needs a small modification
to work properly on Blink.

This is based on a patch by Claudio Saavedra <csaavedra@igalia.com>.

BUG=665422
TEST=editing/caret/caret-color-015.html
TEST=imported/csswg-test/css-ui-3/caret-color-001.html
==========

[chrishtr, 2016-11-21 19:15:38]

Going to defer to timloh and yosin for the main review. I'll approve for
webexposed files
once that's done.

[Timothy Loh, 2016-11-22 02:17:35]

Just skimmed it, some quick comments.

- Are transitions supported or supposed to be supported? I didn't see any tests
for these.
- I'm not sure the tests are imported correctly, see
http://www.chromium.org/blink/importing-the-w3c-tests for the process.

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

https://codereview.chromium.org/2520873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StyleColor.h:66: enum ColorType { AutoColor,
CurrentColor, SpecifiedColor };
I'd prefer if we didn't add Auto to StyleColor. This class is intended to map
directly to a <color> type. Maybe make a separate class (or can we get away with
not adding a new class at all?)

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

https://codereview.chromium.org/2520873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:72:
CSSValue* consumeAutoOrColor(CSSParserTokenRange&, CSSParserMode);
This probably isn't obvious but this sort of function should just be
consumeCaretColor in CSSPropertyParser.cpp for now. This file is for stuff which
is going to be re-used for multiple properties, and in particular for types from
css-values and other main specs.

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

https://codereview.chromium.org/2520873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:95: case
CSSPropertyCaretColor:
Do we need to do this? What are the use cases for styling caret-color
differently for visited and unvisited links? I think it'd be better to not
consider caret-color specially for :visited. Otherwise we'd need to add tests
for this behaviour.

https://codereview.chromium.org/2520873002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/2520873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:2084: if
(result.isAutoColor())
Maybe this logic for resolving auto is better in paint code.

[yosin_UTC9, 2016-11-22 02:25:10]

core/editing lgtm since there are two line changes and these changes are very
explicit.

Since I'm not so familiar with style resolver, visited color handling for
caret-color might be wrong. At first glance, "visited" caret-color is so
redundant.

Could you ask security and privacy guy about "visited" caret-color, since I'm
not familiar with privacy thing? Style for "visited" can leak privacy
information to other site, e.g. the page just have link to "example.com" and the
page checks CSS properties changed with "visited" to know whether user visited
"example.com" or not. So, we need to be careful about CSS properties changed
with "visited".

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

https://codereview.chromium.org/2520873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPropertyEquality.cpp:121:
a.visitedLinkCaretColor() == b.visitedLinkCaretColor();
Why do we need to handle "visited link caret color" specially? Is it for
"currentColor"?
It seems this is copying logic of "color". But, it seems doing so in two places
is redundant.

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

https://codereview.chromium.org/2520873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StyleColor.h:66: enum ColorType { AutoColor,
CurrentColor, SpecifiedColor };
nit: Let's use |enum class|.

|ColorType| is ambiguous. How about |ValueType|?
e.g. enum ValueType { Auto, CurrentColor, Numeric };
According to the spce, <color> = currentColor | transparent | RGB | HSL | ...

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

https://codereview.chromium.org/2520873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/AnimatedStyleBuilder.cpp:414: case
CSSPropertyCaretColor:
Nice! (^_^)b

https://codereview.chromium.org/2520873002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/2520873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:2085: result =
StyleColor::currentColor();
Could you add TODO for improving "auto" color handling? We may want to handle at
least background-color == color case.

[Manuel Rego, 2016-11-22 09:36:01]

Thanks for the reviews!

I'll upload a new version but meanwhile let's reply
to some of your comments.


On 2016/11/22 02:17:35, Timothy Loh wrote:
> - Are transitions supported or supposed to be supported? I didn't see any
tests
> for these.

It should be animatable as color, so I believe transitions
should be supported too.
We've a test for animations but not transitions,
I'll add one for transitions.

It'd be a nice use case for example when you focus the input of a form,
the text can go from lightgrey to black and the caret do the same
in the first seconds.

> - I'm not sure the tests are imported correctly, see
> http://www.chromium.org/blink/importing-the-w3c-tests for the process.

Ups, I'll review the process thanks for the link.
Sorry but it's the first time I import W3C tests,
and I used Tools/Scripts/import-w3c-tests manually.


On 2016/11/22 02:25:10, Yosi_UTC9 wrote:
> Could you ask security and privacy guy about "visited" caret-color, since I'm
> not familiar with privacy thing? Style for "visited" can leak privacy
> information to other site, e.g. the page just have link to "example.com" and
the
> page checks CSS properties changed with "visited" to know whether user visited
> "example.com" or not. So, we need to be careful about CSS properties changed
> with "visited".

The computed style for the visited links is the same than
for the non-visited ones. So we're not leaking any information.
We might need a specific test regarding this too.

For example if you have the following CSS:
  a { color: green; caret-color: lime; }
  a:visited { color: blue; caret-color: cyan; }

The computed style for a visited link would be:
  * color: green
  * caret-color: lime

So we're not leaking any information. Do you still think I should ping
someone from security team. If that's the case, any suggestion of
who should I contact? Thanks.

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

https://codereview.chromium.org/2520873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPropertyEquality.cpp:121:
a.visitedLinkCaretColor() == b.visitedLinkCaretColor();
On 2016/11/22 02:25:10, Yosi_UTC9 wrote:
> Why do we need to handle "visited link caret color" specially? Is it for
> "currentColor"?
> It seems this is copying logic of "color". But, it seems doing so in two
places
> is redundant.

It's not only color, all the other color properties have a similar logic:
CSSPropertyBackgroundColor, CSSPropertyBorderXXXColor,
CSSPropertyColumnRuleColor, CSSPropertyWebkitTextStrokeColor.

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

https://codereview.chromium.org/2520873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StyleColor.h:66: enum ColorType { AutoColor,
CurrentColor, SpecifiedColor };
On 2016/11/22 02:17:35, Timothy Loh wrote:
> I'd prefer if we didn't add Auto to StyleColor. This class is intended to map
> directly to a <color> type. Maybe make a separate class (or can we get away
with
> not adding a new class at all?)

I can create a different class, but I don't see how we can manage "auto"
without having a specific class for this.

Specifically I'm wondering how we could return "auto" as computed value,
if we don't store it using something like this.
The spec is clear about caret-color returning "auto" as computed value.

https://codereview.chromium.org/2520873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StyleColor.h:66: enum ColorType { AutoColor,
CurrentColor, SpecifiedColor };
On 2016/11/22 02:25:10, Yosi_UTC9 wrote:
> nit: Let's use |enum class|.
> 
> |ColorType| is ambiguous. How about |ValueType|?
> e.g. enum ValueType { Auto, CurrentColor, Numeric };
> According to the spce, <color> = currentColor | transparent | RGB | HSL | ...

Good suggestion, anyway as @timloh suggested I can move this
to a different class.

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

https://codereview.chromium.org/2520873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:72:
CSSValue* consumeAutoOrColor(CSSParserTokenRange&, CSSParserMode);
On 2016/11/22 02:17:35, Timothy Loh wrote:
> This probably isn't obvious but this sort of function should just be
> consumeCaretColor in CSSPropertyParser.cpp for now. This file is for stuff
which
> is going to be re-used for multiple properties, and in particular for types
from
> css-values and other main specs.

I added it here as is supposed that any new/future color property
that is added on CSS will follow the same pattern than caret-color:
  auto | <color>

Anyway I can manage this specifically for caret-color at this moment
if you think it's better.

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

https://codereview.chromium.org/2520873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:95: case
CSSPropertyCaretColor:
On 2016/11/22 02:17:35, Timothy Loh wrote:
> Do we need to do this? What are the use cases for styling caret-color
> differently for visited and unvisited links? I think it'd be better to not
> consider caret-color specially for :visited. Otherwise we'd need to add tests
> for this behaviour.

We actually have a test for this specific behavior:
  editing/caret/caret-color-015.html

The tests were validated by Florian Rivoal (the spec editor)
so I believe we really want to support this.

Authors usually modify the color of the link if it's visited,
if the link is editable it'd be nice to have a different caret-color too.

https://codereview.chromium.org/2520873002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/2520873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:2084: if
(result.isAutoColor())
On 2016/11/22 02:17:35, Timothy Loh wrote:
> Maybe this logic for resolving auto is better in paint code.

If we don't do this here, we need to modify the if after this switch.
But as so far only caret-color accepts "auto" I thought it was better
doing it here.

https://codereview.chromium.org/2520873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:2085: result =
StyleColor::currentColor();
On 2016/11/22 02:25:10, Yosi_UTC9 wrote:
> Could you add TODO for improving "auto" color handling? We may want to handle
at
> least background-color == color case.

Sure that would allow us to pass: caret-color-006.html

https://codereview.chromium.org/2520873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:2127: return
result.getColor();
The call to getColor() will ASSERT if it's "auto".

[Manuel Rego, 2016-11-23 11:01:54]

I've uploaded a new version applying suggested changes.
Please take a look, thanks.

Regarding tests, I included a new test for transitions
and I'm upstreaming a few more tests to the W3C suite
related to visited links computed style and animations.

Once they're merged (hopefully tomorrow), I'll upload
a new CL, we'll be passing all the W3C test suite but:
* caret-color-006.html: Which is optional. It's the one
  where caret-color and background color are the same.

Then we'll be skipping:
* caret-color-017.html: It's a manual test to check
  a transition. We've a custom test to verify that at
  editing/caret/caret-color-transition.html.
* caret-color-008.html: It's an infinite animation
  changing caret-color. We'll use a new testharness test
  (caret-color-018.html) that checks the animation.

[Manuel Rego, 2016-11-25 11:32:29]

The tests have been merged upstream (W3C csswg-test repo)
and I've imported them in the last patch.
Please take a look to the new verision, thanks!


BTW, I know that the official process regarding W3C tests
is that we modify LayoutTests/W3CImportExpectations
and import the whole "css-ui-3/" folder.
However I'm only interested on the caret-color tests,
the rest (most of them) are failing.

So I'm not sure what's the best approach, mark the folder
to be imported in LayoutTests/W3CImportExpectations,
and then blacklist all the tests but caret-color ones.
Or just do what I'm doing now, maybe adding a comment
on W3CImportExpectations like:
## Owners: rego@igalia.com
# imported/csswg-test/css-ui-3/caret-color-* [ Pass ] 

Also for the tests that we're skipping
(explained reasons on previous comment):
* caret-color-006.html
* caret-color-008.html
* caret-color-017.html

I'm directly not importing them, but I don't know
if I should import them and mark as "Skip" or what.

Do you know who can help to solve these doubts?
Thanks!

[Timothy Loh, 2016-11-28 03:11:21]

https://codereview.chromium.org/2520873002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl
(right):

https://codereview.chromium.org/2520873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:354:
{{apply_auto_color('CSSPropertyCaretColor') }}
Since this is only for a single property, it's better to not have a macro and
just write this out in StyleBuilderCustom.cpp

https://codereview.chromium.org/2520873002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/animation/css/CSSAnimatableValueFactory.cpp
(right):

https://codereview.chromium.org/2520873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/css/CSSAnimatableValueFactory.cpp:411:
return createFromColor(property, style);
I'm not sure if this is going to end up doing the right thing for auto. Since
the computed value for auto is auto, we (I think) shouldn't transition between
auto and specified colors. From what I can tell, this code will end up doing
that? I would suggest implementing transition and animation logic for
caret-color in a separate patch.

https://codereview.chromium.org/2520873002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
(right):

https://codereview.chromium.org/2520873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:2214:
style.caretColor().resolve(style.color()).rgb());
This line is weird, resolve() is to handle currentColor but at this point we're
never currentColor.

https://codereview.chromium.org/2520873002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/StyleAutoColor.h (right):

https://codereview.chromium.org/2520873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/StyleAutoColor.h:12: class StyleAutoColor :
public StyleColor {
This is a weird use of inheritance because it breaks some invariants of the
StyleColor class. Also, shadowing functions as your resolve() does is generally
not a good idea. I would prefer if this class didn't inherit and just stored a
Color and two bools.

https://codereview.chromium.org/2520873002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp
(right):

https://codereview.chromium.org/2520873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:1107: if
(toCSSIdentifierValue(value).getValueID() == CSSValueCurrentcolor)
Not sure why this changed.

https://codereview.chromium.org/2520873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:1118: if
(value.isIdentifierValue()) {
if (value.isIdentifierValue() && toCSSIdentifierValue(value).getValueID() ==
CSSValueAuto)
  return StyleAutoColor::autoColor();
return convertStyleColor(...);

https://codereview.chromium.org/2520873002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/2520873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.h:2504: void
setCaretColor(const StyleAutoColor& c) {
Is there somewhere in this file with lots of other color setters that this can
be adjacent to?

[Timothy Loh, 2016-11-28 03:12:46]

On 2016/11/25 11:32:29, Manuel Rego wrote:
> The tests have been merged upstream (W3C csswg-test repo)
> and I've imported them in the last patch.
> Please take a look to the new verision, thanks!
> 
> 
> BTW, I know that the official process regarding W3C tests
> is that we modify LayoutTests/W3CImportExpectations
> and import the whole "css-ui-3/" folder.
> However I'm only interested on the caret-color tests,
> the rest (most of them) are failing.
> 
> So I'm not sure what's the best approach, mark the folder
> to be imported in LayoutTests/W3CImportExpectations,
> and then blacklist all the tests but caret-color ones.
> Or just do what I'm doing now, maybe adding a comment
> on W3CImportExpectations like:
> ## Owners: mailto:rego@igalia.com
> # imported/csswg-test/css-ui-3/caret-color-* [ Pass ] 
> 
> Also for the tests that we're skipping
> (explained reasons on previous comment):
> * caret-color-006.html
> * caret-color-008.html
> * caret-color-017.html
> 
> I'm directly not importing them, but I don't know
> if I should import them and mark as "Skip" or what.
> 
> Do you know who can help to solve these doubts?
> Thanks!

Maybe tkent or foolip?

[Manuel Rego, 2016-11-28 10:47:55]

Thanks for the review, I'll upload a new version ASAP.

I've sent a mail to @tkent and @foolip to see if we can clarify
the tests issue too.

https://codereview.chromium.org/2520873002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl
(right):

https://codereview.chromium.org/2520873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:354:
{{apply_auto_color('CSSPropertyCaretColor') }}
On 2016/11/28 03:11:21, Timothy Loh wrote:
> Since this is only for a single property, it's better to not have a macro and
> just write this out in StyleBuilderCustom.cpp

Ok I'll move it.
Anyway, as I said it's supposed that new color properties follow the same
pattern,
but we can create a macro later if needed.

https://codereview.chromium.org/2520873002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/animation/css/CSSAnimatableValueFactory.cpp
(right):

https://codereview.chromium.org/2520873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/css/CSSAnimatableValueFactory.cpp:411:
return createFromColor(property, style);
On 2016/11/28 03:11:21, Timothy Loh wrote:
> I'm not sure if this is going to end up doing the right thing for auto. Since
> the computed value for auto is auto, we (I think) shouldn't transition between
> auto and specified colors. From what I can tell, this code will end up doing
> that? I would suggest implementing transition and animation logic for
> caret-color in a separate patch.

Mmmm, interesting I didn't realize about that.
I'm fine with moving this to a follow-up patch.

The problem with caret-color is that both "auto" and "currentColor"
have as computed value "auto" and "currentColor".
So I'm not sure if we could do a transition/animation from
"currentColor" to "red" or from "auto" to "red".
However, the rest of color properties allow a transition from
"currentColor" to "red" (because the computed value is never
"currentColor").
I'm not sure what should we do here, maybe @florian can help
to resolve this doubt from the specs point of view.

https://codereview.chromium.org/2520873002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
(right):

https://codereview.chromium.org/2520873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:2214:
style.caretColor().resolve(style.color()).rgb());
On 2016/11/28 03:11:21, Timothy Loh wrote:
> This line is weird, resolve() is to handle currentColor but at this point
we're
> never currentColor.

Sure, I didn't realize before but it seems we don't need this.

https://codereview.chromium.org/2520873002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/StyleAutoColor.h (right):

https://codereview.chromium.org/2520873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/StyleAutoColor.h:12: class StyleAutoColor :
public StyleColor {
On 2016/11/28 03:11:21, Timothy Loh wrote:
> This is a weird use of inheritance because it breaks some invariants of the
> StyleColor class. Also, shadowing functions as your resolve() does is
generally
> not a good idea. I would prefer if this class didn't inherit and just stored a
> Color and two bools.

Ok, yeah I had doubts about what to do. I'll create a new class for this.

https://codereview.chromium.org/2520873002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp
(right):

https://codereview.chromium.org/2520873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:1107: if
(toCSSIdentifierValue(value).getValueID() == CSSValueCurrentcolor)
On 2016/11/28 03:11:21, Timothy Loh wrote:
> Not sure why this changed.

This is a leftover from the previous version of the patch, sorry.

https://codereview.chromium.org/2520873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:1118: if
(value.isIdentifierValue()) {
On 2016/11/28 03:11:21, Timothy Loh wrote:
> if (value.isIdentifierValue() && toCSSIdentifierValue(value).getValueID() ==
> CSSValueAuto)
>   return StyleAutoColor::autoColor();
> return convertStyleColor(...);

Ok.

https://codereview.chromium.org/2520873002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/2520873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.h:2504: void
setCaretColor(const StyleAutoColor& c) {
On 2016/11/28 03:11:21, Timothy Loh wrote:
> Is there somewhere in this file with lots of other color setters that this can
> be adjacent to?

I didn't find it, in this file they're quite mixed with other properties.

[Manuel Rego, 2016-11-29 15:49:58]

Description was changed from

==========
Add support for caret-color property

The test basically adds the new CSS property "caret-color":
https://drafts.csswg.org/css-ui/#propdef-caret-color

"caret-color" is a regular color property but it accepts also "auto".
To support this StyleColor has been modified, including a new flag
to know if the color is auto, currentColor or a specified one.

Finally the patches modifies CaretBase::paintCaret() to use
that property instead of "color" as it's been doing until now.

We're using the test suite from the W3C CSS WG.
This patch passes all the test but caret-color-006.html,
which was not imported as it's something optional on the spec
(background and caret-color are both "gray").
One of the tests (caret-color-015.html) needs a small modification
to work properly on Blink.

This is based on a patch by Claudio Saavedra <csaavedra@igalia.com>.

BUG=665422
TEST=editing/caret/caret-color-015.html
TEST=imported/csswg-test/css-ui-3/caret-color-001.html
==========

to

==========
[css-ui] Add support for caret-color property

The test basically adds the new CSS property "caret-color":
https://drafts.csswg.org/css-ui/#propdef-caret-color

"caret-color" is a regular color property but it accepts also "auto".
To support this a new class StyleAutoColor has been created,
including a new flag to know if the color is auto, currentColor
or a specified one.

Finally the patches modifies CaretBase::paintCaret() to use
that property instead of "color" as it's been doing until now.

We're using the test suite from the W3C CSS WG,
which was imported on r434986.
This CL manually imports some pixel tests too.

This is based on a patch by Claudio Saavedra <csaavedra@igalia.com>.

BUG=665422
TEST=editing/caret/caret-color-*.html
TEST=imported/csswg-test/css-ui-3/caret-color-*.html
==========

[Manuel Rego, 2016-11-29 15:54:16]

I've uploaded a new version of the patch applying suggested changes.
Please @timloh take a look to the new version.

Regarding the tests and thanks to the help from @foolip,
we imported the css-ui-3/ test suite in:
https://codereview.chromium.org/2539753002/

And as part of this CL I'm manually importing a few pixel tests,
as they're not automatically imported by the W3C script.

Also I removed all the code related to animations/transitions
from this patch, as I'll do that in a different CL.

PS: Sorry for doing a few rebases in this CL,
but they were needed to deal with the tests.

[Timothy Loh, 2016-11-30 14:51:10]

lgtm

https://codereview.chromium.org/2520873002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/StyleAutoColor.h (right):

https://codereview.chromium.org/2520873002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/StyleAutoColor.h:18: StyleAutoColor() :
m_type(ValueType::Auto) {}
Might be nicer to omit this constructor so users of StyleAutoColor are more
explicit.

https://codereview.chromium.org/2520873002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp
(right):

https://codereview.chromium.org/2520873002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:1107:
(toCSSIdentifierValue(value).getValueID() == CSSValueCurrentcolor))
No reason to add parenthesis here :)

https://codereview.chromium.org/2520873002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/StyleRareInheritedData.cpp (right):

https://codereview.chromium.org/2520873002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleRareInheritedData.cpp:80:
m_visitedLinkCaretColorIsAuto(false),
Visited should also default to auto, not currentColor

https://codereview.chromium.org/2520873002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleRareInheritedData.cpp:192:
m_caretColor == o.m_caretColor &&
Missing a check for m_visitedLinkCaretColor?

[chrishtr, 2016-11-30 15:10:14]

lgtm

[Manuel Rego, 2016-11-30 15:41:53]

Thank you all for the reviews and all the feedback on this!

https://codereview.chromium.org/2520873002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/StyleAutoColor.h (right):

https://codereview.chromium.org/2520873002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/StyleAutoColor.h:18: StyleAutoColor() :
m_type(ValueType::Auto) {}
On 2016/11/30 14:51:10, Timothy Loh wrote:
> Might be nicer to omit this constructor so users of StyleAutoColor are more
> explicit.

Ok.

https://codereview.chromium.org/2520873002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp
(right):

https://codereview.chromium.org/2520873002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:1107:
(toCSSIdentifierValue(value).getValueID() == CSSValueCurrentcolor))
On 2016/11/30 14:51:10, Timothy Loh wrote:
> No reason to add parenthesis here :)

Sure, sorry for the noise again. :-/

https://codereview.chromium.org/2520873002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/StyleRareInheritedData.cpp (right):

https://codereview.chromium.org/2520873002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleRareInheritedData.cpp:80:
m_visitedLinkCaretColorIsAuto(false),
On 2016/11/30 14:51:10, Timothy Loh wrote:
> Visited should also default to auto, not currentColor

Yes.

https://codereview.chromium.org/2520873002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleRareInheritedData.cpp:192:
m_caretColor == o.m_caretColor &&
On 2016/11/30 14:51:10, Timothy Loh wrote:
> Missing a check for m_visitedLinkCaretColor?

Indeed. Nice catch.

[Manuel Rego, 2016-11-30 15:42:05]

The CQ bit was checked by rego@igalia.com

[Manuel Rego, 2016-11-30 15:42:06]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org,
chrishtr@chromium.org, timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/2520873002/#ps200001
(title: "Patch for landing applying suggested changes")

[commit-bot: I haz the power, 2016-11-30 15:42:37]

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-11-30 18:19:38]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1480520525481990,
"parent_rev": "6c0d041dd82e5cfa3b0c45ecb2fd950dfe595f11", "commit_rev":
"f595fc24cdb89da0a0a2166195d2fd6ca4876ee9"}

[commit-bot: I haz the power, 2016-11-30 18:20:08]

Description was changed from

==========
[css-ui] Add support for caret-color property

The test basically adds the new CSS property "caret-color":
https://drafts.csswg.org/css-ui/#propdef-caret-color

"caret-color" is a regular color property but it accepts also "auto".
To support this a new class StyleAutoColor has been created,
including a new flag to know if the color is auto, currentColor
or a specified one.

Finally the patches modifies CaretBase::paintCaret() to use
that property instead of "color" as it's been doing until now.

We're using the test suite from the W3C CSS WG,
which was imported on r434986.
This CL manually imports some pixel tests too.

This is based on a patch by Claudio Saavedra <csaavedra@igalia.com>.

BUG=665422
TEST=editing/caret/caret-color-*.html
TEST=imported/csswg-test/css-ui-3/caret-color-*.html
==========

to

==========
[css-ui] Add support for caret-color property

The test basically adds the new CSS property "caret-color":
https://drafts.csswg.org/css-ui/#propdef-caret-color

"caret-color" is a regular color property but it accepts also "auto".
To support this a new class StyleAutoColor has been created,
including a new flag to know if the color is auto, currentColor
or a specified one.

Finally the patches modifies CaretBase::paintCaret() to use
that property instead of "color" as it's been doing until now.

We're using the test suite from the W3C CSS WG,
which was imported on r434986.
This CL manually imports some pixel tests too.

This is based on a patch by Claudio Saavedra <csaavedra@igalia.com>.

BUG=665422
TEST=editing/caret/caret-color-*.html
TEST=imported/csswg-test/css-ui-3/caret-color-*.html
==========

[commit-bot: I haz the power, 2016-11-30 18:20:10]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-11-30 18:27:54]

Description was changed from

==========
[css-ui] Add support for caret-color property

The test basically adds the new CSS property "caret-color":
https://drafts.csswg.org/css-ui/#propdef-caret-color

"caret-color" is a regular color property but it accepts also "auto".
To support this a new class StyleAutoColor has been created,
including a new flag to know if the color is auto, currentColor
or a specified one.

Finally the patches modifies CaretBase::paintCaret() to use
that property instead of "color" as it's been doing until now.

We're using the test suite from the W3C CSS WG,
which was imported on r434986.
This CL manually imports some pixel tests too.

This is based on a patch by Claudio Saavedra <csaavedra@igalia.com>.

BUG=665422
TEST=editing/caret/caret-color-*.html
TEST=imported/csswg-test/css-ui-3/caret-color-*.html
==========

to

==========
[css-ui] Add support for caret-color property

The test basically adds the new CSS property "caret-color":
https://drafts.csswg.org/css-ui/#propdef-caret-color

"caret-color" is a regular color property but it accepts also "auto".
To support this a new class StyleAutoColor has been created,
including a new flag to know if the color is auto, currentColor
or a specified one.

Finally the patches modifies CaretBase::paintCaret() to use
that property instead of "color" as it's been doing until now.

We're using the test suite from the W3C CSS WG,
which was imported on r434986.
This CL manually imports some pixel tests too.

This is based on a patch by Claudio Saavedra <csaavedra@igalia.com>.

BUG=665422
TEST=editing/caret/caret-color-*.html
TEST=imported/csswg-test/css-ui-3/caret-color-*.html

Committed: https://crrev.com/c142daf285cf004dd28c7a88c57920279d123300
Cr-Commit-Position: refs/heads/master@{#435338}
==========

[commit-bot: I haz the power, 2016-11-30 18:27:56]

Patchset 11 (id:??) landed as
https://crrev.com/c142daf285cf004dd28c7a88c57920279d123300
Cr-Commit-Position: refs/heads/master@{#435338}