Issue 851693007: Prepare for responsive CSS animations.

Issue 851693007: Prepare for responsive CSS animations. (Closed)

Created:
5 years, 11 months ago by shend

Modified:
5 years, 10 months ago

Reviewers:
dstockwell, Timothy Loh, shans, alancutter (OOO until 2018)

CC:
dstockwell, darktears, blink-reviews, blink-reviews-animation_chromium.org, Eric Willigers, Mike Lawther (Google), rjwright, Steve Block

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Target Ref:
refs/heads/master

Project:
blink

Visibility:
Public.

More Reviews

Description

Prepare for responsive CSS animations. When a animated property is absent in the 0% or 100% keyframe, a neutral keyframe is created which takes the value of the computed style. When the computed style changes, the spec requires that the neutral keyframe is updated accordingly to match the new style. This patch adds code necessary to make neutral keyframes in CSS animations responsive to style changes. The follow up patch which actually switches over to responsive behaviour is here: https://codereview.chromium.org/920653002/ BUG=361948 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190387

Patch Set 1 #

Patch Set 2 : Use DeferredLegacyStyleInterpolation for neutral keyframes #

Patch Set 3 : Basic layout test #

Patch Set 4 : Neutral keyframes run on compositor #

Patch Set 5 : Attempt to re-snapshot only if needed #

Total comments: 18

Patch Set 6 : Address comments #

Patch Set 7 : Rebase + Simplify code #

Patch Set 8 : Compositor restarts after style change #

Total comments: 10

Patch Set 9 : Address comments #

Patch Set 10 : Fix timing function layout tests #

Total comments: 1

Patch Set 11 : Rebase and Add ems/font size test #

Patch Set 12 : Rebase + Fix simultaneous keyframes & style change crash #

Patch Set 13 : Change UpdatedAnimationStyle to class for consistency #

Total comments: 4

Patch Set 14 : Clean up includes #

Total comments: 13

Patch Set 15 : Address comments #

Total comments: 13

Patch Set 16 : Address comments #

Patch Set 17 : Rebase and tidy up #

Total comments: 17

Patch Set 18 : Address comments #

Patch Set 19 : Revert string keyframes and responsive changes #

Patch Set 20 : Rebase #

Total comments: 1

Created: 5 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+253 lines, -45 lines)			Patch
A	LayoutTests/animations/timing-functions-neutral-keyframe.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16	1 chunk	+21 lines, -0 lines	0 comments	Download
M	Source/core/animation/ActiveAnimations.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16	1 chunk	+2 lines, -1 line	0 comments	Download
M	Source/core/animation/CompositorAnimations.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16	1 chunk	+2 lines, -0 lines	0 comments	Download
M	Source/core/animation/CompositorAnimations.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16	2 chunks	+6 lines, -1 line	0 comments	Download
M	Source/core/animation/DeferredLegacyStyleInterpolation.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	2 chunks	+10 lines, -0 lines	0 comments	Download
M	Source/core/animation/DeferredLegacyStyleInterpolation.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17	2 chunks	+24 lines, -7 lines	0 comments	Download
M	Source/core/animation/InterpolationEffect.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17	1 chunk	+7 lines, -0 lines	0 comments	Download
M	Source/core/animation/KeyframeEffectModel.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17	5 chunks	+37 lines, -10 lines	0 comments	Download
M	Source/core/animation/KeyframeEffectModel.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17	5 chunks	+49 lines, -6 lines	1 comment	Download
M	Source/core/animation/StringKeyframe.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	2 chunks	+3 lines, -0 lines	0 comments	Download
M	Source/core/animation/StringKeyframe.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	4 chunks	+19 lines, -0 lines	0 comments	Download
M	Source/core/animation/StyleInterpolation.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/animation/css/CSSAnimationUpdate.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17	6 chunks	+53 lines, -0 lines	0 comments	Download
M	Source/core/animation/css/CSSAnimations.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18	6 chunks	+19 lines, -20 lines	0 comments	Download

Messages

Total messages: 37 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Timothy Loh

https://codereview.chromium.org/851693007/diff/80001/Source/core/animation/ActiveAnimations.h File Source/core/animation/ActiveAnimations.h (right): https://codereview.chromium.org/851693007/diff/80001/Source/core/animation/ActiveAnimations.h#newcode101 Source/core/animation/ActiveAnimations.h:101: friend class DeferredLegacyStyleInterpolation; unused? https://codereview.chromium.org/851693007/diff/80001/Source/core/animation/DeferredLegacyStyleInterpolation.cpp File Source/core/animation/DeferredLegacyStyleInterpolation.cpp (right): https://codereview.chromium.org/851693007/diff/80001/Source/core/animation/DeferredLegacyStyleInterpolation.cpp#newcode8 ...

5 years, 11 months ago (2015-01-20 05:29:32 UTC) #2

shend

5 years, 11 months ago (2015-01-20 23:13:04 UTC) #3

dstockwell

https://codereview.chromium.org/851693007/diff/140001/Source/core/animation/CompositorAnimations.cpp File Source/core/animation/CompositorAnimations.cpp (right): https://codereview.chromium.org/851693007/diff/140001/Source/core/animation/CompositorAnimations.cpp#newcode151 Source/core/animation/CompositorAnimations.cpp:151: if ((keyframe->composite() != AnimationEffect::CompositeReplace && !isNeutralKeyframe) || !keyframe->getAnimatableValue() || ...

5 years, 10 months ago (2015-01-27 02:22:59 UTC) #6

shend

5 years, 10 months ago (2015-01-28 02:44:50 UTC) #7

shans

Approach looks good. I think the description is wrong - AFAIK Firefox doesn't handle dependent ...

5 years, 10 months ago (2015-01-30 12:38:14 UTC) #9

shend

On 2015/01/30 12:38:14, shans wrote: > Approach looks good. > > I think the description ...

5 years, 10 months ago (2015-02-01 22:50:47 UTC) #10

shans

On 2015/02/01 22:50:47, shend wrote: > On 2015/01/30 12:38:14, shans wrote: > > Approach looks ...

5 years, 10 months ago (2015-02-02 00:02:40 UTC) #11

shend

On 2015/02/02 00:02:40, shans wrote: > On 2015/02/01 22:50:47, shend wrote: > > On 2015/01/30 ...

5 years, 10 months ago (2015-02-02 00:33:11 UTC) #12

On 2015/02/02 00:02:40, shans wrote:
> On 2015/02/01 22:50:47, shend wrote:
> > On 2015/01/30 12:38:14, shans wrote:
> > > Approach looks good.
> > > 
> > > I think the description is wrong - AFAIK Firefox doesn't handle dependent
> > > updates (em values changing when font size does) but some version
(possibly
> > > unreleased) of IE does.
> > > 
> > >
> >
>
https://codereview.chromium.org/851693007/diff/180001/LayoutTests/animations/...
> > > File LayoutTests/animations/responsive-neutral-keyframes.html (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/851693007/diff/180001/LayoutTests/animations/...
> > > LayoutTests/animations/responsive-neutral-keyframes.html:11: width: 100px;
> > > What happens if this is specified in em and the font size changes? Can we
> have
> > a
> > > test that demonstrates what this behaviour is?
> > 
> > When an animation has ems and the font size changes, the animation doesn't
> > change.
> 
> It certainly does. I just tested in canary.
> 
> > There's already a bug report for this:
> > https://code.google.com/p/chromium/issues/detail?id=437689. It appears to be
> an
> > issue with the animation engine in general, not just CSS animations. This
> patch
> > only considers direct changes to the animated property, not dependent
updates
> > via font size. I'll make it clearer in the patch description.
> 
> This is an issue with the legacy AnimatableValue pipeline. You'll find that
the
> em/font size update has been working in Web Animations for some time, and
> support for other dependencies is coming on line with Eve and Jade's work.
> 
> You need to either ensure that this patch deals with this issue or provide a
> reasonable story about how a follow-up patch will address it. There won't be a
> fix to 437689 that magically makes CSS Animations become responsive,
> unfortunately :(

I tested this fiddle http://jsfiddle.net/ua0q9pqt/ on Canary and div height
doesn't seem to respond to the change in fontSize. Perhaps we're talking about
different things?

shans

On 2015/02/02 00:33:11, shend wrote: > On 2015/02/02 00:02:40, shans wrote: > > On 2015/02/01 ...

5 years, 10 months ago (2015-02-02 00:46:09 UTC) #13

On 2015/02/02 00:33:11, shend wrote:
> On 2015/02/02 00:02:40, shans wrote:
> > On 2015/02/01 22:50:47, shend wrote:
> > > On 2015/01/30 12:38:14, shans wrote:
> > > > Approach looks good.
> > > > 
> > > > I think the description is wrong - AFAIK Firefox doesn't handle
dependent
> > > > updates (em values changing when font size does) but some version
> (possibly
> > > > unreleased) of IE does.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/851693007/diff/180001/LayoutTests/animations/...
> > > > File LayoutTests/animations/responsive-neutral-keyframes.html (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/851693007/diff/180001/LayoutTests/animations/...
> > > > LayoutTests/animations/responsive-neutral-keyframes.html:11: width:
100px;
> > > > What happens if this is specified in em and the font size changes? Can
we
> > have
> > > a
> > > > test that demonstrates what this behaviour is?
> > > 
> > > When an animation has ems and the font size changes, the animation doesn't
> > > change.
> > 
> > It certainly does. I just tested in canary.
> > 
> > > There's already a bug report for this:
> > > https://code.google.com/p/chromium/issues/detail?id=437689. It appears to
be
> > an
> > > issue with the animation engine in general, not just CSS animations. This
> > patch
> > > only considers direct changes to the animated property, not dependent
> updates
> > > via font size. I'll make it clearer in the patch description.
> > 
> > This is an issue with the legacy AnimatableValue pipeline. You'll find that
> the
> > em/font size update has been working in Web Animations for some time, and
> > support for other dependencies is coming on line with Eve and Jade's work.
> > 
> > You need to either ensure that this patch deals with this issue or provide a
> > reasonable story about how a follow-up patch will address it. There won't be
a
> > fix to 437689 that magically makes CSS Animations become responsive,
> > unfortunately :(
> 
> I tested this fiddle http://jsfiddle.net/ua0q9pqt/ on Canary and div height
> doesn't seem to respond to the change in fontSize. Perhaps we're talking about
> different things?

"When an animation has ems and the font size changes, the animation doesn't
change"

Try testing that instead (i.e. animate width with 'em' start and end points,
then change the font size part way through). Unfortunately, animations will
probably never be able to be the source of dependencies due to the way the style
engine is set up. We can and should honor dependencies going in the other
direction though.

shend

Added test with ems/font size. Did you mean something like that? Currently this patch fails ...

5 years, 10 months ago (2015-02-02 02:27:32 UTC) #14

shans

On 2015/02/02 02:27:32, shend wrote: > Added test with ems/font size. Did you mean something ...

5 years, 10 months ago (2015-02-02 02:55:38 UTC) #15

shend

On 2015/02/02 02:55:38, shans wrote: > On 2015/02/02 02:27:32, shend wrote: > > Added test ...

5 years, 10 months ago (2015-02-02 02:58:42 UTC) #16

shend

Rebased onto dynamic keyframes patch (https://codereview.chromium.org/814083003/). PTAL. Are there any more cases I should test ...

5 years, 10 months ago (2015-02-03 00:15:48 UTC) #17

shend

https://codereview.chromium.org/851693007/diff/240001/Source/core/animation/KeyframeEffectModel.cpp File Source/core/animation/KeyframeEffectModel.cpp (right): https://codereview.chromium.org/851693007/diff/240001/Source/core/animation/KeyframeEffectModel.cpp#newcode96 Source/core/animation/KeyframeEffectModel.cpp:96: for (size_t i = 0; i < keyframes.size(); i++) ...

5 years, 10 months ago (2015-02-03 00:51:28 UTC) #18

Timothy Loh

https://codereview.chromium.org/851693007/diff/260001/Source/core/animation/KeyframeEffectModel.cpp File Source/core/animation/KeyframeEffectModel.cpp (right): https://codereview.chromium.org/851693007/diff/260001/Source/core/animation/KeyframeEffectModel.cpp#newcode84 Source/core/animation/KeyframeEffectModel.cpp:84: else if (!keyframe.getAnimatableValue()) assert there's no animatable value? only ...

5 years, 10 months ago (2015-02-04 05:31:45 UTC) #19

shend

5 years, 10 months ago (2015-02-04 22:51:34 UTC) #20

alancutter (OOO until 2018)

https://codereview.chromium.org/851693007/diff/260001/Source/core/animation/DeferredLegacyStyleInterpolation.h File Source/core/animation/DeferredLegacyStyleInterpolation.h (right): https://codereview.chromium.org/851693007/diff/260001/Source/core/animation/DeferredLegacyStyleInterpolation.h#newcode40 Source/core/animation/DeferredLegacyStyleInterpolation.h:40: void setOutdated() { m_outdated = true; } I'm tempted ...

5 years, 10 months ago (2015-02-05 04:59:02 UTC) #21

alancutter (OOO until 2018)

https://codereview.chromium.org/851693007/diff/260001/Source/core/animation/InterpolationEffect.h File Source/core/animation/InterpolationEffect.h (right): https://codereview.chromium.org/851693007/diff/260001/Source/core/animation/InterpolationEffect.h#newcode33 Source/core/animation/InterpolationEffect.h:33: void setDeferredInterpolationsOutdated(); On 2015/02/05 04:59:02, alancutter wrote: > Just ...

5 years, 10 months ago (2015-02-05 22:04:28 UTC) #22

dstockwell

Description should mention where we are changing from AnimatableValueKeyframes to StringKeyframes, and what the remaining ...

5 years, 10 months ago (2015-02-05 23:04:53 UTC) #23

shend

Updated description and created a manual test. I've changed a lot of code, so PTAL. ...

5 years, 10 months ago (2015-02-06 03:16:25 UTC) #24

Timothy Loh

https://codereview.chromium.org/851693007/diff/320001/LayoutTests/animations/interpolation/box-shadow-interpolation-expected.txt File LayoutTests/animations/interpolation/box-shadow-interpolation-expected.txt (right): https://codereview.chromium.org/851693007/diff/320001/LayoutTests/animations/interpolation/box-shadow-interpolation-expected.txt#newcode20 LayoutTests/animations/interpolation/box-shadow-interpolation-expected.txt:20: PASS: box-shadow from [10px 20px yellow, 5px 10px green] ...

5 years, 10 months ago (2015-02-11 03:54:36 UTC) #25

Timothy Loh

https://codereview.chromium.org/851693007/diff/320001/Source/core/animation/InterpolationEffect.cpp File Source/core/animation/InterpolationEffect.cpp (right): https://codereview.chromium.org/851693007/diff/320001/Source/core/animation/InterpolationEffect.cpp#newcode9 Source/core/animation/InterpolationEffect.cpp:9: #include "core/animation/StyleInterpolation.h" unused includes? https://codereview.chromium.org/851693007/diff/320001/Source/core/animation/InterpolationEffect.h File Source/core/animation/InterpolationEffect.h (right): https://codereview.chromium.org/851693007/diff/320001/Source/core/animation/InterpolationEffect.h#newcode33 ...

5 years, 10 months ago (2015-02-11 04:09:01 UTC) #26

Timothy Loh

https://codereview.chromium.org/851693007/diff/320001/Source/core/animation/KeyframeEffectModel.cpp File Source/core/animation/KeyframeEffectModel.cpp (right): https://codereview.chromium.org/851693007/diff/320001/Source/core/animation/KeyframeEffectModel.cpp#newcode71 Source/core/animation/KeyframeEffectModel.cpp:71: if (affects(property)) { On 2015/02/11 04:09:00, Timothy Loh wrote: ...

5 years, 10 months ago (2015-02-11 04:16:10 UTC) #27

alancutter (OOO until 2018)

I would not be surprised if this change got reverted due to performance regressions. The ...

5 years, 10 months ago (2015-02-11 05:16:10 UTC) #28

shend

Addressed comments, will be splitting the patch into two. https://codereview.chromium.org/851693007/diff/320001/LayoutTests/animations/responsive-neutral-keyframes.html File LayoutTests/animations/responsive-neutral-keyframes.html (right): https://codereview.chromium.org/851693007/diff/320001/LayoutTests/animations/responsive-neutral-keyframes.html#newcode48 LayoutTests/animations/responsive-neutral-keyframes.html:48: ...

5 years, 10 months ago (2015-02-12 01:01:42 UTC) #29

Timothy Loh

First line of the patch description should be a copy of the patch title

5 years, 10 months ago (2015-02-13 02:35:16 UTC) #31

dstockwell

lgtm https://codereview.chromium.org/851693007/diff/380001/Source/core/animation/KeyframeEffectModel.cpp File Source/core/animation/KeyframeEffectModel.cpp (right): https://codereview.chromium.org/851693007/diff/380001/Source/core/animation/KeyframeEffectModel.cpp#newcode105 Source/core/animation/KeyframeEffectModel.cpp:105: // FIXME: Handle neutral keyframes that are not ...

5 years, 10 months ago (2015-02-18 02:27:03 UTC) #34

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/851693007/380001

5 years, 10 months ago (2015-02-18 02:31:32 UTC) #36

commit-bot: I haz the power

5 years, 10 months ago (2015-02-18 04:31:27 UTC) #37

Message was sent while issue was closed.

Committed patchset #20 (id:380001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190387

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages