Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(17)

Issue 2537373005: [css-ui] Make caret-color animatable (Closed)

Created:
4 years ago by Manuel Rego
Modified:
4 years ago
CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-animation_chromium.org, blink-reviews-css, blink-reviews-style_chromium.org, blink-reviews-w3ctests_chromium.org, chromium-reviews, dglazkov+blink, Eric Willigers, florian_rivoal.net, rjwright, rwlbuis, shans, tfarina
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[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}

Patch Set 1 #

Total comments: 1

Patch Set 2 : New version with new tests #

Patch Set 3 : Fix interpolation test using assertNoInterpolation() #

Patch Set 4 : Update W3C tests as they've been accepted upstream #

Patch Set 5 : Rebased patch #

Total comments: 4

Patch Set 6 : Rebased patch fixing build modifying CSSInterpolationTypesMap #

Patch Set 7 : Apply suggested changes and add new composition test #

Total comments: 2

Patch Set 8 : Apply suggested changes #

Total comments: 3

Patch Set 9 : Come back to previous version but keep DCHECK in toStyleColor() #

Total comments: 2

Patch Set 10 : Patch for landing adding TODO #

Unified diffs Side-by-side diffs Delta from patch set Stats (+246 lines, -10 lines) Patch
M third_party/WebKit/LayoutTests/TestExpectations View 1 2 3 4 5 6 7 8 1 chunk +1 line, -2 lines 0 comments Download
A third_party/WebKit/LayoutTests/animations/composition/caret-color-composition.html View 1 2 3 4 5 6 8 1 chunk +43 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/animations/interpolation/caret-color-interpolation.html View 1 2 8 1 chunk +123 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/imported/csswg-test/css-ui-3/caret-color-021.html View 1 2 3 4 5 6 7 8 1 chunk +33 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/animation/CSSColorInterpolationType.cpp View 1 2 3 4 5 6 8 1 chunk +4 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/animation/CSSInterpolationTypesMap.cpp View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/animation/ColorPropertyFunctions.h View 1 2 3 4 5 6 8 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/animation/ColorPropertyFunctions.cpp View 1 2 3 4 5 6 7 8 9 5 chunks +24 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/animation/css/CSSAnimatableValueFactory.cpp View 1 2 3 4 5 1 chunk +6 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/css/CSSProperties.in View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/css/StyleAutoColor.h View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/css/resolver/AnimatedStyleBuilder.cpp View 1 2 3 4 5 1 chunk +5 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/style/ComputedStyle.cpp View 1 2 3 4 5 6 7 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 31 (10 generated)
Manuel Rego
This patch is pretty simple, but it has a difference with a regular color due ...
4 years ago (2016-12-01 00:22:30 UTC) #2
Timothy Loh
+alancutter from animations team
4 years ago (2016-12-01 02:00:46 UTC) #4
alancutter (OOO until 2018)
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 ...
4 years ago (2016-12-01 02:05:31 UTC) #5
Manuel Rego
Hi @alancutter, thanks for the review. Ok, I'll create one of those tests and remove ...
4 years ago (2016-12-01 10:16:54 UTC) #6
alancutter (OOO until 2018)
On 2016/12/01 at 10:16:54, rego wrote: > Hi @alancutter, thanks for the review. > > ...
4 years ago (2016-12-04 22:33:44 UTC) #7
Manuel Rego
So it seems the things have been clarified on the CSS WG issue: https://github.com/w3c/csswg-drafts/issues/781 Regarding ...
4 years ago (2016-12-14 15:50:24 UTC) #8
alancutter (OOO until 2018)
On 2016/12/14 at 15:50:24, rego wrote: > So it seems the things have been clarified ...
4 years ago (2016-12-14 23:44:31 UTC) #10
Manuel Rego
On 2016/12/14 23:44:31, alancutter wrote: > Use assertNoInterpolation(), that will handle transitions' differing behaviour. Thanks ...
4 years ago (2016-12-15 11:12:22 UTC) #11
Manuel Rego
The tests caret-color-019.html and caret-color-020.html have been accepted and merged at csswg-tests repo. I've just ...
4 years ago (2016-12-15 17:34:33 UTC) #12
alancutter (OOO until 2018)
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: ...
4 years ago (2016-12-15 22:27:11 UTC) #13
alancutter (OOO until 2018)
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 ...
4 years ago (2016-12-15 22:31:15 UTC) #14
Manuel Rego
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): ...
4 years ago (2016-12-19 12:28:31 UTC) #15
alancutter (OOO until 2018)
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 ...
4 years ago (2016-12-20 05:45:53 UTC) #17
Manuel Rego
Thanks for the review, I've applied the suggested changes. However I've issues now in caret-color-interpolation.html, ...
4 years ago (2016-12-20 17:18:17 UTC) #18
alancutter (OOO until 2018)
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 ...
4 years ago (2016-12-21 00:49:05 UTC) #19
Manuel Rego
@alancutter thanks for all your help on this, really appreciated. :-) Uploaded a new version, ...
4 years ago (2016-12-21 13:14:38 UTC) #20
alancutter (OOO until 2018)
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; ...
4 years ago (2016-12-21 23:24:03 UTC) #21
Manuel Rego
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 ...
4 years ago (2016-12-22 09:03:01 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2537373005/180001
4 years ago (2016-12-22 09:03:18 UTC) #26
commit-bot: I haz the power
Committed patchset #10 (id:180001)
4 years ago (2016-12-22 10:42:44 UTC) #29
commit-bot: I haz the power
4 years ago (2016-12-22 10:45:04 UTC) #31
Message was sent while issue was closed.
Patchset 10 (id:??) landed as
https://crrev.com/3ddca0e44883c2ae38bcc45b48853d0db31e9691
Cr-Commit-Position: refs/heads/master@{#440377}

Powered by Google App Engine
This is Rietveld 408576698