WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
158403
Add experimental support for spring based CSS animations
https://bugs.webkit.org/show_bug.cgi?id=158403
Summary
Add experimental support for spring based CSS animations
Sam Weinig
Reported
2016-06-05 15:11:08 PDT
Add experimental support for spring based CSS animations
Attachments
Patch
(68.49 KB, patch)
2016-06-05 15:39 PDT
,
Sam Weinig
no flags
Details
Formatted Diff
Diff
Patch
(73.09 KB, patch)
2016-06-05 18:13 PDT
,
Sam Weinig
no flags
Details
Formatted Diff
Diff
Patch
(79.99 KB, patch)
2016-06-06 15:43 PDT
,
Sam Weinig
no flags
Details
Formatted Diff
Diff
Patch
(80.01 KB, patch)
2016-06-06 18:56 PDT
,
Sam Weinig
no flags
Details
Formatted Diff
Diff
Patch
(80.00 KB, patch)
2016-06-06 19:23 PDT
,
Sam Weinig
dino
: review+
buildbot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from ews103 for mac-yosemite
(924.62 KB, application/zip)
2016-06-06 20:14 PDT
,
Build Bot
no flags
Details
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Sam Weinig
Comment 1
2016-06-05 15:39:59 PDT
Created
attachment 280564
[details]
Patch
Darin Adler
Comment 2
2016-06-05 16:42:59 PDT
Comment on
attachment 280564
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=280564&action=review
Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:380:12: error: enumeration value 'SpringFunction' not handled in switch [-Werror=switch]
> Source/WebCore/css/CSSParser.h:218 > + bool parseSpringTimingFunctionValue(CSSParserValueList& args, double& result);
I’d use Optional<double> instead, in this new code. I know the old code does not work that way.
> Source/WebCore/css/CSSTimingFunctionValue.cpp:46 > + StringBuilder builder; > + builder.appendLiteral("cubic-bezier("); > + builder.appendNumber(m_x1); > + builder.appendLiteral(", "); > + builder.appendNumber(m_y1); > + builder.appendLiteral(", "); > + builder.appendNumber(m_x2); > + builder.appendLiteral(", "); > + builder.appendNumber(m_y2); > + builder.append(')'); > + > + return builder.toString();
So makeString can’t do this kind of job with the numeric conversion without temporary strings? I’d leave out the blank line before calling toString.
> Source/WebCore/page/animation/AnimationBase.cpp:77 > + SpringSolver solver(mass, stiffness, damping, initialVelocity); > + return solver.solve(t * duration);
I would be tempted to write this without the local variable, but maybe the syntax would be ugly.
> Source/WebCore/platform/animation/TimingFunction.h:244 > + return adoptRef(*new SpringTimingFunction(1, 100, 10, 0));
Not clear why these are the default values. Might need a comment. Maybe just have this one call the other create function.
> Source/WebCore/platform/animation/TimingFunction.h:249 > + virtual ~SpringTimingFunction() > + { > + }
I don’t see any reason to define this explicitly. Sometimes there could be value in that if we defined it in a cpp file, but if we are writing this inline version, I think just not writing anything is a better way to express it.
> Source/WebCore/platform/animation/TimingFunction.h:284 > + PassRefPtr<TimingFunction> clone() const override
Maybe just final instead of override.
> Source/WebCore/platform/graphics/SpringSolver.h:34 > + m_w0 = sqrt(stiffness / mass);
Better to use std::sqrt; we normally do that so we get overloading for types. But since this is double doesn’t matter in practice.
> Source/WebCore/platform/graphics/SpringSolver.h:53 > + t = exp(-t * m_zeta * m_w0) * (m_A * cos(m_wd * t) + m_B * sin(m_wd * t));
std::exp, std::cos, std::sin
> Source/WebCore/platform/graphics/SpringSolver.h:56 > + t = (m_A + m_B * t) * exp(-t * m_w0);
std::exp
> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2919 > + RefPtr<PlatformCAAnimation> basicAnim = createPlatformCAAnimation(PlatformCAAnimation::Spring, keyPath);
How about auto?
> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:245 > + PassRefPtr<PlatformCAAnimation> createSpringAnimation(const Animation*, const String&, bool additive);
Can the return type be RefPtr instead of PassRefPtr?
Sam Weinig
Comment 3
2016-06-05 17:38:20 PDT
(In reply to
comment #2
)
> Comment on
attachment 280564
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=280564&action=review
> > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders. > cpp:380:12: error: enumeration value 'SpringFunction' not handled in switch > [-Werror=switch] > > > Source/WebCore/css/CSSParser.h:218 > > + bool parseSpringTimingFunctionValue(CSSParserValueList& args, double& result); > > I’d use Optional<double> instead, in this new code. I know the old code does > not work that way.
Done.
> > > Source/WebCore/css/CSSTimingFunctionValue.cpp:46 > > + StringBuilder builder; > > + builder.appendLiteral("cubic-bezier("); > > + builder.appendNumber(m_x1); > > + builder.appendLiteral(", "); > > + builder.appendNumber(m_y1); > > + builder.appendLiteral(", "); > > + builder.appendNumber(m_x2); > > + builder.appendLiteral(", "); > > + builder.appendNumber(m_y2); > > + builder.append(')'); > > + > > + return builder.toString(); > > So makeString can’t do this kind of job with the numeric conversion without > temporary strings?
I can't remember where we landed on this, but I couldn't find support in makeString for numbers. It seems like a good area to look into for me.
> > I’d leave out the blank line before calling toString.
Done
> > > Source/WebCore/page/animation/AnimationBase.cpp:77 > > + SpringSolver solver(mass, stiffness, damping, initialVelocity); > > + return solver.solve(t * duration); > > I would be tempted to write this without the local variable, but maybe the > syntax would be ugly.
I like it on two lines, and it matches other similar idioms we have elsewhere (e.g. UnitBezier).
> > > Source/WebCore/platform/animation/TimingFunction.h:244 > > + return adoptRef(*new SpringTimingFunction(1, 100, 10, 0)); > > Not clear why these are the default values. Might need a comment.
Done.
> > Maybe just have this one call the other create function.
Done.
> > > Source/WebCore/platform/animation/TimingFunction.h:249 > > + virtual ~SpringTimingFunction() > > + { > > + } > > I don’t see any reason to define this explicitly. Sometimes there could be > value in that if we defined it in a cpp file, but if we are writing this > inline version, I think just not writing anything is a better way to express > it.
Done.
> > > Source/WebCore/platform/animation/TimingFunction.h:284 > > + PassRefPtr<TimingFunction> clone() const override > > Maybe just final instead of override.
Since the whole class is marked final, what value is added in marking this one final?
> > > Source/WebCore/platform/graphics/SpringSolver.h:34 > > + m_w0 = sqrt(stiffness / mass); > > Better to use std::sqrt; we normally do that so we get overloading for > types. But since this is double doesn’t matter in practice.
Done.
> > > Source/WebCore/platform/graphics/SpringSolver.h:53 > > + t = exp(-t * m_zeta * m_w0) * (m_A * cos(m_wd * t) + m_B * sin(m_wd * t)); > > std::exp, std::cos, std::sin
Done.
> > > Source/WebCore/platform/graphics/SpringSolver.h:56 > > + t = (m_A + m_B * t) * exp(-t * m_w0); > > std::exp
Done.
> > > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2919 > > + RefPtr<PlatformCAAnimation> basicAnim = createPlatformCAAnimation(PlatformCAAnimation::Spring, keyPath); > > How about auto?
Done.
> > > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:245 > > + PassRefPtr<PlatformCAAnimation> createSpringAnimation(const Animation*, const String&, bool additive); > > Can the return type be RefPtr instead of PassRefPtr?
Done.
Sam Weinig
Comment 4
2016-06-05 18:13:54 PDT
Created
attachment 280568
[details]
Patch
Dean Jackson
Comment 5
2016-06-05 21:31:31 PDT
Comment on
attachment 280568
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=280568&action=review
> Source/WebCore/ChangeLog:13 > + Adds experimental support for a new CSS animation timing function that uses > + spring to model the time function. To use it you replace your normal timing > + function, be it cubic-bezier or steps, with a new function called spring(). > + For instance, for a transition you would write: > + > + transition-timing-function: spring(1 100 10 0);
In our documentation for this, which I'll start on tomorrow, we need to explain how duration relates to the spring function.
> Source/WebCore/ChangeLog:120 > 2016-06-03 Gavin & Ellie Barraclough <
barraclough@apple.com
>
Unrelated. Nice!
> Source/WebCore/css/CSSParser.cpp:5239 > + // For cubic bezier, 4 values must be specified (comma-separated).
We should propose making the commas optional.
> Source/WebCore/css/CSSParser.cpp:5249 > + if (!x1) > + return nullptr; > + if (x1.value() < 0 || x1.value() > 1) > + return nullptr;
Do these in one condition?
> Source/WebCore/css/CSSParser.cpp:5258 > + if (!x2) > return nullptr; > - if (!parseCubicBezierTimingFunctionValue(*args, y1)) > + if (x2.value() < 0 || x2.value() > 1)
And here?
> Source/WebCore/css/CSSParser.cpp:5270 > + // For a spring, 4 values must be specified (space-separated). > + if (!args || args->size() != 4)
Add FIXME that says we'll make some/all of the arguments optional.
> Source/WebCore/css/CSSParser.cpp:5285 > + if (!stiffness) > + return nullptr; > + if (stiffness.value() <= 0) > + return nullptr;
By now I'm guessing you like having explicit tests, so ignore my suggestions above.
> Source/WebCore/platform/animation/TimingFunction.h:254 > + const SpringTimingFunction& otherString = *static_cast<const SpringTimingFunction*>(&other); > + > + return m_mass == otherString.m_mass && m_stiffness == otherString.m_stiffness && m_damping == otherString.m_damping && m_initialVelocity == otherString.m_initialVelocity;
Remove blank line.
> Source/WebCore/platform/graphics/SpringSolver.h:41 > + m_B = (m_zeta * m_w0 + -initialVelocity) / m_wd;
Weird that this isn't .... m_w0 - initialVelocity
> Source/WebCore/platform/graphics/SpringSolver.h:45 > + m_B = -initialVelocity + m_w0;
And this isn't m_w0 - initialVelocity
> LayoutTests/animations/spring-function.html:4 > +<html lang="en"> > +<head> > + <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
Not needed.
> LayoutTests/animations/spring-function.html:44 > +</head> > +<body>
Ditto.
> LayoutTests/animations/spring-function.html:51 > +</body> > +</html>
Ditto.
> LayoutTests/animations/script-tests/spring-computed-style.js:33 > +testComputedSpring("Basic", "spring(1 100 10 0)", "spring(1 100 10 0)"); > +testComputedSpring("Negative Velocity", "spring(1 100 10 -10)", "spring(1 100 10 -10)"); > +testComputedSpring("Positive Velocity", "spring(1 100 10 10)", "spring(1 100 10 10)"); > +testComputedSpring("Zero Damping", "spring(1 100 0 10)", "spring(1 100 0 10)"); > +testComputedSpring("Minimum Values", "spring(1 1 0 -999999)", "spring(1 1 0 -999999)");
Test floats (makes sure we didn't look only for integers)? Test exponential numbers?
> LayoutTests/animations/script-tests/spring-computed-style.js:46 > +testComputedSpring("No parameters", "spring()", "ease"); > +testComputedSpring("Not enough parameters", "spring(1 100 10)", "ease"); > +testComputedSpring("Too many parameters", "spring(1 100 10 0 0)", "ease"); > +testComputedSpring("Illegal Mass (< 0)", "spring(-1 100 10 0)", "ease"); > +testComputedSpring("Illegal Mass (== 0)", "spring(0 100 10 0)", "ease"); > +testComputedSpring("Illegal Stiffness (< 0)", "spring(1 -1 10 0)", "ease"); > +testComputedSpring("Illegal Stiffness (== 0)", "spring(1 0 10 0)", "ease"); > +testComputedSpring("Illegal Damping (< 0)", "spring(1 100 -1 0)", "ease");
Test junk? spring(a b c d).
WebKit Commit Bot
Comment 6
2016-06-05 21:55:14 PDT
Comment on
attachment 280568
[details]
Patch Clearing flags on attachment: 280568 Committed
r201706
: <
http://trac.webkit.org/changeset/201706
>
WebKit Commit Bot
Comment 7
2016-06-05 21:55:17 PDT
All reviewed patches have been landed. Closing bug.
Csaba Osztrogonác
Comment 8
2016-06-06 01:20:48 PDT
(In reply to
comment #6
)
> Comment on
attachment 280568
[details]
> Patch > > Clearing flags on attachment: 280568 > > Committed
r201706
: <
http://trac.webkit.org/changeset/201706
>
It broke the Apple Mac Yosemite build, see build.webkit.org for details.
WebKit Commit Bot
Comment 9
2016-06-06 10:43:15 PDT
Re-opened since this is blocked by
bug 158427
Ryan Haddad
Comment 10
2016-06-06 13:30:22 PDT
(In reply to
comment #8
)
> (In reply to
comment #6
) > > Comment on
attachment 280568
[details]
> > Patch > > > > Clearing flags on attachment: 280568 > > > > Committed
r201706
: <
http://trac.webkit.org/changeset/201706
> > > It broke the Apple Mac Yosemite build, see build.webkit.org for details.
https://build.webkit.org/builders/Apple%20Yosemite%20Release%20%28Build%29/builds/15774
Sam Weinig
Comment 11
2016-06-06 15:43:21 PDT
Created
attachment 280639
[details]
Patch
Sam Weinig
Comment 12
2016-06-06 18:56:57 PDT
Created
attachment 280659
[details]
Patch
Sam Weinig
Comment 13
2016-06-06 19:23:31 PDT
Created
attachment 280661
[details]
Patch
Build Bot
Comment 14
2016-06-06 20:14:18 PDT
Comment on
attachment 280661
[details]
Patch
Attachment 280661
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.webkit.org/results/1455786
New failing tests: animations/spring-function.html animations/spring-parsing.html animations/spring-computed-style.html
Build Bot
Comment 15
2016-06-06 20:14:23 PDT
Created
attachment 280665
[details]
Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Sam Weinig
Comment 16
2016-06-07 09:26:17 PDT
(In reply to
comment #14
)
> Comment on
attachment 280661
[details]
> Patch > >
Attachment 280661
[details]
did not pass mac-ews (mac): > Output:
http://webkit-queues.webkit.org/results/1455786
> > New failing tests: > animations/spring-function.html > animations/spring-parsing.html > animations/spring-computed-style.html
Failures are due to Xcode not recompiling InternalSettingsGenerated, not a real failure.
Sam Weinig
Comment 17
2016-06-07 11:14:04 PDT
Committed
r201759
: <
http://trac.webkit.org/changeset/201759
>
Ryan Haddad
Comment 18
2016-06-07 12:44:50 PDT
(In reply to
comment #17
)
> Committed
r201759
: <
http://trac.webkit.org/changeset/201759
>
The Yosemite build still appears to be failing after the attempted build fix in
https://trac.webkit.org/changeset/201764
https://build.webkit.org/builders/Apple%20Yosemite%20Release%20%28Build%29/builds/15825
/Volumes/Data/slave/yosemite-release/build/Source/WebKit2/WebProcess/WebPage/mac/PlatformCAAnimationRemote.mm:783:34: error: instance method '-setInitialVelocity:' not found (return type defaults to 'id') [-Werror,-Wobjc-method-access]
Alex Christensen
Comment 19
2016-06-07 13:13:39 PDT
Yosemite builders don't like this: /Volumes/Data/slave/yosemite-release/build/Source/WebKit2/WebProcess/WebPage/mac/PlatformCAAnimationRemote.mm:783:34: error: instance method '-setInitialVelocity:' not found (return type defaults to 'id') [-Werror,-Wobjc-method-access] [springAnimation setInitialVelocity:function.initialVelocity()];
Alex Christensen
Comment 20
2016-06-07 13:28:09 PDT
CAAnimation.h has this: CA_CLASS_AVAILABLE (10.11, 9.0, 9.0, 2.0) @interface CASpringAnimation : CABasicAnimation ... @property CGFloat initialVelocity;
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug