Bug 181978 - [Web Animations] Expose getKeyframes() and parsing of remaining keyframe properties
Summary: [Web Animations] Expose getKeyframes() and parsing of remaining keyframe prop...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-23 04:35 PST by Antoine Quint
Modified: 2018-01-23 13:40 PST (History)
8 users (show)

See Also:


Attachments
Patch (95.52 KB, patch)
2018-01-23 05:09 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (95.08 KB, patch)
2018-01-23 09:56 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for landing (36.16 KB, patch)
2018-01-23 11:03 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for landing (95.18 KB, patch)
2018-01-23 11:05 PST, Antoine Quint
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2018-01-23 04:35:21 PST
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.
Comment 1 Radar WebKit Bug Importer 2018-01-23 04:35:57 PST
<rdar://problem/36772586>
Comment 2 Antoine Quint 2018-01-23 05:09:25 PST
Created attachment 332019 [details]
Patch
Comment 3 EWS Watchlist 2018-01-23 05:11:15 PST
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.
Comment 4 Antoine Quint 2018-01-23 09:56:57 PST
Created attachment 332034 [details]
Patch
Comment 5 EWS Watchlist 2018-01-23 09:58:53 PST
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.
Comment 6 Dean Jackson 2018-01-23 10:24:00 PST
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?
Comment 7 Antoine Quint 2018-01-23 10:27:18 PST
(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.
Comment 8 Antoine Quint 2018-01-23 11:03:34 PST
Created attachment 332040 [details]
Patch for landing
Comment 9 Antoine Quint 2018-01-23 11:05:13 PST
Created attachment 332042 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2018-01-23 11:24:53 PST
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
Comment 11 Antoine Quint 2018-01-23 12:04:05 PST
Committed r227428: <https://trac.webkit.org/changeset/227428>
Comment 12 Matt Lewis 2018-01-23 13:26:35 PST
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?
Comment 13 Antoine Quint 2018-01-23 13:35:06 PST
(In reply to Matt Lewis from comment #12)

Fixing it now.
Comment 14 Antoine Quint 2018-01-23 13:40:50 PST
Committed r227441: <https://trac.webkit.org/changeset/227441>