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

Issue 38823002: Web Animations CSS: Support animation of {text,box,-webkit-box}-shadow and fix blur clamping (Closed)

Created:
7 years, 2 months ago by Timothy Loh
Modified:
7 years, 2 months ago
Reviewers:
dstockwell, mithro-old
CC:
blink-reviews, shans, rjwright, alancutter (OOO until 2018), Mike Lawther (Google), zoltan1, eae+blinkwatch, dglazkov+blink, leviw+renderwatch, dstockwell, Timothy Loh, apavlov+blink_chromium.org, jchaffraix+rendering, darktears, Steve Block, dino_apple.com, Eric Willigers
Base URL:
https://chromium.googlesource.com/chromium/blink@master
Visibility:
Public.

Description

Web Animations CSS: Support animation of {text,box,-webkit-box}-shadow and fix blur clamping This patch adds support for animating the shadow properties in the Web Animations implementation. We fix the interpolation of blur to clamp to zero, but leave the existing incorrect behaviour in which shadow lists with mismatched shadows will interpolate anyway, even though the spec suggests (it is poorly written!) that we shouldn't be doing any interpolation here. The code for interpolating ShadowLists in CSSPropertyAnimation has been moved to ShadowList and ShadowData, so we can re-use it, with minor stylistic changes. Note that some of the test expectation failures are due to integer rounding, which https://codereview.chromium.org/23241010/ probably will resolve. BUG=257591 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=160421

Patch Set 1 #

Total comments: 11

Patch Set 2 : address comments #

Total comments: 6

Patch Set 3 : use 0 instead of PassRefPtr<..>() #

Unified diffs Side-by-side diffs Delta from patch set Stats (+226 lines, -92 lines) Patch
A LayoutTests/animations/interpolation/box-shadow-interpolation.html View 1 1 chunk +62 lines, -0 lines 0 comments Download
A LayoutTests/animations/interpolation/box-shadow-interpolation-expected.txt View 1 1 chunk +19 lines, -0 lines 0 comments Download
A LayoutTests/animations/interpolation/text-shadow-interpolation.html View 1 chunk +31 lines, -0 lines 0 comments Download
A LayoutTests/animations/interpolation/text-shadow-interpolation-expected.txt View 1 chunk +7 lines, -0 lines 0 comments Download
A LayoutTests/virtual/web-animations-css/animations/interpolation/box-shadow-interpolation-expected.txt View 1 1 chunk +19 lines, -0 lines 0 comments Download
A + Source/core/animation/AnimatableShadow.h View 1 chunk +14 lines, -20 lines 0 comments Download
A + Source/core/animation/AnimatableShadow.cpp View 1 chunk +11 lines, -10 lines 0 comments Download
M Source/core/animation/AnimatableValue.h View 2 chunks +2 lines, -0 lines 0 comments Download
M Source/core/animation/css/CSSAnimatableValueFactory.cpp View 1 3 chunks +6 lines, -0 lines 0 comments Download
M Source/core/core.gypi View 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/css/resolver/AnimatedStyleBuilder.cpp View 1 3 chunks +8 lines, -0 lines 0 comments Download
M Source/core/frame/animation/CSSPropertyAnimation.cpp View 3 chunks +1 line, -62 lines 0 comments Download
M Source/core/rendering/style/ShadowData.h View 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/rendering/style/ShadowData.cpp View 2 chunks +14 lines, -0 lines 0 comments Download
M Source/core/rendering/style/ShadowList.h View 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/rendering/style/ShadowList.cpp View 1 2 1 chunk +26 lines, -0 lines 0 comments Download

Messages

Total messages: 9 (0 generated)
Timothy Loh
7 years, 2 months ago (2013-10-24 00:42:28 UTC) #1
mithro-old
Could we get a link to the spec in the comment description? Shouldn't there be ...
7 years, 2 months ago (2013-10-24 01:04:15 UTC) #2
mithro-old
Can you run it on one of the try bots so we can see if ...
7 years, 2 months ago (2013-10-24 01:05:16 UTC) #3
Timothy Loh
https://codereview.chromium.org/38823002/diff/1/LayoutTests/animations/interpolation/box-shadow-interpolation.html File LayoutTests/animations/interpolation/box-shadow-interpolation.html (right): https://codereview.chromium.org/38823002/diff/1/LayoutTests/animations/interpolation/box-shadow-interpolation.html#newcode27 LayoutTests/animations/interpolation/box-shadow-interpolation.html:27: {at: 0.3, is: '6px 4px 11px 2px rgb(77, 50, ...
7 years, 2 months ago (2013-10-24 03:04:53 UTC) #4
dstockwell
lgtm Please file a bug for the remaining issues and add it to the BUG= ...
7 years, 2 months ago (2013-10-24 03:23:38 UTC) #5
mithro-old
LGTM pending dstockwell's finally say. https://codereview.chromium.org/38823002/diff/30001/LayoutTests/virtual/web-animations-css/animations/interpolation/box-shadow-interpolation-expected.txt File LayoutTests/virtual/web-animations-css/animations/interpolation/box-shadow-interpolation-expected.txt (right): https://codereview.chromium.org/38823002/diff/30001/LayoutTests/virtual/web-animations-css/animations/interpolation/box-shadow-interpolation-expected.txt#newcode7 LayoutTests/virtual/web-animations-css/animations/interpolation/box-shadow-interpolation-expected.txt:7: FAIL: box-shadow from [10px ...
7 years, 2 months ago (2013-10-24 03:24:58 UTC) #6
Timothy Loh
https://codereview.chromium.org/38823002/diff/30001/LayoutTests/virtual/web-animations-css/animations/interpolation/box-shadow-interpolation-expected.txt File LayoutTests/virtual/web-animations-css/animations/interpolation/box-shadow-interpolation-expected.txt (right): https://codereview.chromium.org/38823002/diff/30001/LayoutTests/virtual/web-animations-css/animations/interpolation/box-shadow-interpolation-expected.txt#newcode7 LayoutTests/virtual/web-animations-css/animations/interpolation/box-shadow-interpolation-expected.txt:7: FAIL: box-shadow from [10px 20px rgba(255, 255, 0, 0.5), ...
7 years, 2 months ago (2013-10-24 04:16:23 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/timloh@chromium.org/38823002/180001
7 years, 2 months ago (2013-10-24 04:20:56 UTC) #8
commit-bot: I haz the power
7 years, 2 months ago (2013-10-24 06:13:01 UTC) #9
Message was sent while issue was closed.
Change committed as 160421

Powered by Google App Engine
This is Rietveld 408576698