Bug 181978 – [Web Animations] Expose getKeyframes() and parsing of remaining keyframe properties

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Antoine Quint]

There are a few properties left on keyframes (composite and easing) that we don't support yet as well as the getKeyframes() method to expose the parsed keyframes.

[Radar WebKit Bug Importer]

<rdar://problem/36772586>

[Antoine Quint]

Created attachment 332019 [details]
Patch

[EWS Watchlist]

Attachment 332019 [details] did not pass style-queue:


ERROR: Source/WebCore/animation/KeyframeEffect.cpp:456:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
WARNING: This machine could support 4 simulators, but is only configured for 3.
WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>.
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antoine Quint]

Created attachment 332034 [details]
Patch

[EWS Watchlist]

Attachment 332034 [details] did not pass style-queue:


ERROR: Source/WebCore/animation/KeyframeEffect.cpp:456:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
WARNING: This machine could support 4 simulators, but is only configured for 3.
WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>.
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Comment on attachment 332034 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332034&action=review

> Source/WebCore/ChangeLog:10
> +        parsig the remaining properties that can be exposed on keyframes: "easing" and "composite". And since we parse

parsig

> Source/WebCore/animation/KeyframeEffect.cpp:82
> +    for (size_t i = 0; i < keyframes.size(); ++i) {

Use for : loop

> Source/WebCore/platform/animation/TimingFunction.cpp:106
> +ExceptionOr<Ref<TimingFunction>> TimingFunction::createFromCSSText(const String& cssText)

If this returned a RefPtr<TimingFunction> you wouldn't need the &foo.get() at the calling sites.

> Source/WebCore/platform/animation/TimingFunction.cpp:172
> +        if (function.x1() == 0.25 && function.y1() == 0.1 && function.x2() == 0.25 && function.y2() == 1.0)
> +            return "ease";
> +        if (function.x1() == 0.42 && !function.y1() && function.x2() == 1.0 && function.y2() == 1.0)
> +            return "ease-in";
> +        if (!function.x1() && !function.y1() && function.x2() == 0.58 && function.y2() == 1.0)
> +            return "ease-out";
> +        if (function.x1() == 0.42 && !function.y1() && function.x2() == 0.58 && function.y2() == 1.0)
> +            return "ease-in-out";

Are these accurate enough? (no rounding errors?)

> Source/WebCore/platform/animation/TimingFunction.cpp:180
> +    if (m_type == TimingFunction::StepsFunction) {
> +        auto& function = downcast<StepsTimingFunction>(*this);
> +        if (!function.stepAtStart())
> +            return String::format("steps(%d)", function.numberOfSteps());
> +    }

We don't do step-start or step-end?

[Antoine Quint]

(In reply to Dean Jackson from comment #6)
> Comment on attachment 332034 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332034&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        parsig the remaining properties that can be exposed on keyframes: "easing" and "composite". And since we parse
> 
> parsig

Will fix in commit.

> > Source/WebCore/animation/KeyframeEffect.cpp:82
> > +    for (size_t i = 0; i < keyframes.size(); ++i) {
> 
> Use for : loop

Will fix in commit.

> > Source/WebCore/platform/animation/TimingFunction.cpp:106
> > +ExceptionOr<Ref<TimingFunction>> TimingFunction::createFromCSSText(const String& cssText)
> 
> If this returned a RefPtr<TimingFunction> you wouldn't need the &foo.get()
> at the calling sites.

Will look into that.

> > Source/WebCore/platform/animation/TimingFunction.cpp:172
> > +        if (function.x1() == 0.25 && function.y1() == 0.1 && function.x2() == 0.25 && function.y2() == 1.0)
> > +            return "ease";
> > +        if (function.x1() == 0.42 && !function.y1() && function.x2() == 1.0 && function.y2() == 1.0)
> > +            return "ease-in";
> > +        if (!function.x1() && !function.y1() && function.x2() == 0.58 && function.y2() == 1.0)
> > +            return "ease-out";
> > +        if (function.x1() == 0.42 && !function.y1() && function.x2() == 0.58 && function.y2() == 1.0)
> > +            return "ease-in-out";
> 
> Are these accurate enough? (no rounding errors?)

Not that I have found in my testing.

> > Source/WebCore/platform/animation/TimingFunction.cpp:180
> > +    if (m_type == TimingFunction::StepsFunction) {
> > +        auto& function = downcast<StepsTimingFunction>(*this);
> > +        if (!function.stepAtStart())
> > +            return String::format("steps(%d)", function.numberOfSteps());
> > +    }
> 
> We don't do step-start or step-end?

We should! Will fix in commit.

[Antoine Quint]

Created attachment 332040 [details]
Patch for landing

[Antoine Quint]

Created attachment 332042 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 332042 [details]
Patch for landing

Rejecting attachment 332042 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 332042, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/6184537

[Antoine Quint]

Committed r227428: <https://trac.webkit.org/changeset/227428>

[Matt Lewis]

On macOS WK1 the test http/wpt/web-animations/interfaces/AnimationEffectTiming/easing.html is failing consistently. It looks like the expectations changed for the test were not correct for this.

Build:
https://build.webkit.org/results/Apple%20High%20Sierra%20Release%20WK1%20(Tests)/r227428%20(2865)/results.html

Diff:
--- /Volumes/Data/slave/highsierra-release-tests-wk1/build/layout-test-results/http/wpt/web-animations/interfaces/AnimationEffectTiming/easing-expected.txt
+++ /Volumes/Data/slave/highsierra-release-tests-wk1/build/layout-test-results/http/wpt/web-animations/interfaces/AnimationEffectTiming/easing-actual.txt
@@ -1,11 +1,11 @@
 
 PASS Test default value 
-FAIL step-start function assert_approx_equals: The progress of the animation should be approximately 1 at 0ms expected a number but got a "object"
-FAIL steps(1, start) function assert_approx_equals: The progress of the animation should be approximately 1 at 0ms expected a number but got a "object"
+FAIL step-start function assert_equals: expected "steps(1, start)" but got "step-start"
+FAIL steps(1, start) function assert_equals: expected "steps(1, start)" but got "step-start"
 FAIL steps(2, start) function assert_approx_equals: The progress of the animation should be approximately 0.5 at 0ms expected a number but got a "object"
-FAIL step-end function assert_approx_equals: The progress of the animation should be approximately 0 at 0ms expected a number but got a "object"
-FAIL steps(1) function assert_approx_equals: The progress of the animation should be approximately 0 at 0ms expected a number but got a "object"
-FAIL steps(1, end) function assert_approx_equals: The progress of the animation should be approximately 0 at 0ms expected a number but got a "object"
+FAIL step-end function assert_equals: expected "steps(1)" but got "step-end"
+FAIL steps(1) function assert_equals: expected "steps(1)" but got "step-end"
+FAIL steps(1, end) function assert_equals: expected "steps(1)" but got "step-end"
 FAIL steps(2, end) function assert_approx_equals: The progress of the animation should be approximately 0 at 0ms expected a number but got a "object"
 FAIL frames function assert_approx_equals: The progress of the animation should be approximately 0 at 0ms expected a number but got a "object"
 FAIL linear function assert_approx_equals: The progress of the animation should be approximately 0 at 0ms expected a number but got a "object"



Would this just need a rebaseline or is this something deeper?

[Antoine Quint]

(In reply to Matt Lewis from comment #12)

Fixing it now.

[Antoine Quint]

Committed r227441: <https://trac.webkit.org/changeset/227441>