[css-ui] Update resolved value of caret-color property

[css-ui] Update resolved value of caret-color property

The resolved value of caret-color should be the used value,
like for the rest of color properties.
This was discussed and resolved by the CSS WG
(see https://github.com/w3c/csswg-drafts/issues/781).

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

Review-Url: https://codereview.chromium.org/2577633002
Cr-Commit-Position: refs/heads/master@{#442214}
Committed: https://chromium.googlesource.com/chromium/src/+/1d659157007dc8af77915b4af72f577b324c5e81

[Manuel Rego, 2016-12-14 10:17:15]

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

[chrishtr, 2016-12-19 22:30:24]

chrishtr@chromium.org changed reviewers:
+ yutak@chromium.org

[chrishtr, 2016-12-19 22:30:24]

Going to defer on this code review also. I'm not the best expert on this code.

[Yuta Kitamura, 2016-12-20 09:23:03]

https://codereview.chromium.org/2577633002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/2577633002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/EditingStyle.cpp:510:
m_mutableStyle->removeProperty(CSSPropertyCaretColor);
I don't think this is correct.

* currentColor is the color of "color" CSS property. I believe
  currentColor has nothing to do with your intended change.
* It's not OK to remove "auto" caret-color here. This property is
  needed to ensure the cases such as: some text with "auto" caret-color
  being pasted into a block with an explicit caret-color specified.

[Yuta Kitamura, 2016-12-20 09:35:41]

I'm not totally sure why the CSS semantics change would
affect editing.

[Manuel Rego, 2016-12-22 12:54:06]

Trying to explain this as I agree it's somehow tricky.

The problem of changing the resolved value to be always a rgb(),
makes that in some situations instead of "caret-color: auto"
you end up with "caret-color: rgb(X, X, X)" which causes issues.

One of the tests that were failing without the change
in EditingStyle was "editing/style/5065910.html".
This test inserts a text and makes it red, then it copies it
and paste it in a new paragraph where it marks some parts as
blue and other as green.

The thing is that with the change in resolved value,
this text has a "caret-color: rgb(255, 0, 0)"
(because the "auto" is resolved to rgb(255, 0, 0)
when the red text is copied).
Then as it has a specified caret-color (and not "auto"),
the caret is always red, even if the text is now blue and green.
If we avoid to copy the "caret-color" in this case, we're safe
as by default it'd consider "caret-color: auto".

Just to be more clear about this test,
without the change in EditingStyle the output is:
  <font color="#ff0000">This text should be red.</font>
  <div>
    <span style="caret-color: rgb(255, 0, 0);">
      <font color="#0000ff">This text should be </font>
      <font color="#008000">a combination of green and blue, not </font>
      <font color="#0000ff">red.</font>
    </span>
  </div>

With this change it is:
  <font color="#ff0000">This text should be red.</font>
  <div>
    <font color="#0000ff">This text should be </font>
    <font color="#008000">a combination of green and blue, not </font>
    <font color="#0000ff">red.</font>
  </div>

The very same issue will happen if in this text we'd have
"caret-color: currentColor", it'd be copied as "rgb(255, 0, 0)"
instead of "currentColor" and the caret would be red in all
the blue and green text.

[Manuel Rego, 2016-12-22 13:06:01]

On 2016/12/22 12:54:06, Manuel Rego wrote:
> Trying to explain this as I agree it's somehow tricky.
> 
> The problem of changing the resolved value to be always a rgb(),
> makes that in some situations instead of "caret-color: auto"
> you end up with "caret-color: rgb(X, X, X)" which causes issues.
> 
> One of the tests that were failing without the change
> in EditingStyle was "editing/style/5065910.html".
> This test inserts a text and makes it red, then it copies it
> and paste it in a new paragraph where it marks some parts as
> blue and other as green.
> 
> The thing is that with the change in resolved value,
> this text has a "caret-color: rgb(255, 0, 0)"
> (because the "auto" is resolved to rgb(255, 0, 0)
> when the red text is copied).
> Then as it has a specified caret-color (and not "auto"),
> the caret is always red, even if the text is now blue and green.
> If we avoid to copy the "caret-color" in this case, we're safe
> as by default it'd consider "caret-color: auto".
> 
> Just to be more clear about this test,
> without the change in EditingStyle the output is:
>   <font color="#ff0000">This text should be red.</font>
>   <div>
>     <span style="caret-color: rgb(255, 0, 0);">
>       <font color="#0000ff">This text should be </font>
>       <font color="#008000">a combination of green and blue, not </font>
>       <font color="#0000ff">red.</font>
>     </span>
>   </div>
> 
> With this change it is:
>   <font color="#ff0000">This text should be red.</font>
>   <div>
>     <font color="#0000ff">This text should be </font>
>     <font color="#008000">a combination of green and blue, not </font>
>     <font color="#0000ff">red.</font>
>   </div>

BTW, this is exactly the same thing that happens if you
add "-webkit-text-stroke-width: 2px" to the test.
If we remove the lines:
  if (computedStyle->textStrokeColor().isCurrentColor())
    m_mutableStyle->removeProperty(CSSPropertyWebkitTextStrokeColor);
Then, the stroke is red for all the text.

But with the current code and those lines, the stroke matches
the color of each part of the text.

[Manuel Rego, 2016-12-26 09:54:45]

Description was changed from

==========
[css-ui] Update resolved value of caret-color property

The resolved value of caret-color should be the used value,
like for the rest of color properties.
This was discussed on the CSS WG issue:
https://github.com/w3c/csswg-drafts/issues/781
Note that Firefox WIP implementation follows the same approach.

The tests have been update upstream too:
https://github.com/w3c/csswg-test/pull/1161

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

to

==========
[css-ui] Update resolved value of caret-color property

The resolved value of caret-color should be the used value,
like for the rest of color properties.
This was discussed and resolved by the CSS WG
(see https://github.com/w3c/csswg-drafts/issues/781).

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

[Manuel Rego, 2016-12-26 09:56:37]

Now that the modifications in the tests
have been merged upstream:
https://github.com/w3c/csswg-test/pull/1161
I've uploaded a new version of the patch, rebased against
current master and removing the failing tests from TestExpectations.

PTAL. Thanks! :-)

[Timothy Loh, 2017-01-09 05:33:10]

lgtm

[Manuel Rego, 2017-01-09 07:47:25]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2017-01-09 07:47:30]

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, 2017-01-09 09:14:00]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483948045686680,
"parent_rev": "17c1d0c7a41d7d78443a32b94b8cee31d2d6b86d", "commit_rev":
"1d659157007dc8af77915b4af72f577b324c5e81"}

[commit-bot: I haz the power, 2017-01-09 09:14:29]

Description was changed from

==========
[css-ui] Update resolved value of caret-color property

The resolved value of caret-color should be the used value,
like for the rest of color properties.
This was discussed and resolved by the CSS WG
(see https://github.com/w3c/csswg-drafts/issues/781).

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

to

==========
[css-ui] Update resolved value of caret-color property

The resolved value of caret-color should be the used value,
like for the rest of color properties.
This was discussed and resolved by the CSS WG
(see https://github.com/w3c/csswg-drafts/issues/781).

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

Review-Url: https://codereview.chromium.org/2577633002
Cr-Commit-Position: refs/heads/master@{#442214}
Committed:
https://chromium.googlesource.com/chromium/src/+/1d659157007dc8af77915b4af72f...
==========

[commit-bot: I haz the power, 2017-01-09 09:14:30]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/1d659157007dc8af77915b4af72f...

[Manuel Rego, 2017-01-09 09:34:32]

JFTR, this CL should point to bug #676763 instead of bug #665422.
Sorry about the mistake.