181585 – Add support for the frames() timing function

RESOLVED FIXED 181585

Add support for the frames() timing function

https://bugs.webkit.org/show_bug.cgi?id=181585

Summary Add support for the frames() timing function

Antoine Quint

Reported 2018-01-12 01:04:22 PST

Add support for the frames() timing function

Attachments
Patch (29.66 KB, patch) 2018-01-12 01:11 PST, Antoine Quint	ews-watchlist: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews101 for mac-sierra (2.21 MB, application/zip) 2018-01-12 02:21 PST, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.62 MB, application/zip) 2018-01-12 02:26 PST, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.18 MB, application/zip) 2018-01-12 02:50 PST, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews112 for mac-sierra (2.91 MB, application/zip) 2018-01-12 04:55 PST, EWS Watchlist	no flags	Details
View All Add attachment proposed patch, testcase, etc.

Antoine Quint

Comment 1 2018-01-12 01:07:36 PST

As specified in https://www.w3.org/TR/css-timing-1/#frames-timing-functions.

Radar WebKit Bug Importer

Comment 2 2018-01-12 01:07:48 PST

<rdar://problem/36463317>

Antoine Quint

Comment 3 2018-01-12 01:11:23 PST

Created attachment 331185 [details] Patch

Dean Jackson

Comment 4 2018-01-12 01:58:58 PST

Comment on attachment 331185 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331185&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1572 > + auto& function = static_cast<const FramesTimingFunction&>(timingFunction); Why isn't this a downcast? > Source/WebCore/css/CSSTimingFunctionValue.h:104 > + CSSFramesTimingFunctionValue(int frames) Why is this an int? Use unsigned. Also, validate that it is > 0. Actually > 1. > Source/WebCore/css/parser/CSSPropertyParser.cpp:1493 > + RefPtr<CSSPrimitiveValue> frames = consumePositiveInteger(args); > + if (!frames) > + return nullptr; > + > + int numberOfFrames = frames->intValue(); > + if (numberOfFrames < 2) > + return nullptr; Can use auto here. > Source/WebCore/platform/animation/TimingFunction.cpp:85 > + auto numberOfFrames = function.numberOfFrames(); put in an ASSERT(numberOfFrames) > Source/WebCore/platform/animation/TimingFunction.h:49 > bool isLinearTimingFunction() const { return m_type == LinearFunction; } > bool isCubicBezierTimingFunction() const { return m_type == CubicBezierFunction; } > bool isStepsTimingFunction() const { return m_type == StepsFunction; } > + bool isFramesTimingFunction() const { return m_type == FramesFunction; } > bool isSpringTimingFunction() const { return m_type == SpringFunction; } We should add traits for all these. > Source/WebCore/platform/animation/TimingFunction.h:228 > + static Ref<FramesTimingFunction> create(int frames) Again, maybe use unsigned? > Source/WebCore/platform/animation/TimingFunction.h:235 > + static Ref<FramesTimingFunction> create() > + { > + return adoptRef(*new FramesTimingFunction(2)); > + } Is this ever used? > LayoutTests/transitions/frames-timing-function.html:4 > +<!DOCTYPE html> > + > +<html> > +<head> Not needed. > LayoutTests/transitions/frames-timing-function.html:13 > + -webkit-transform: translateX(0px); > + -webkit-transition-duration: 1s; > + -webkit-transition-timing-function: frames(5); > + -webkit-transition-property: -webkit-transform; Remove prefix. > LayoutTests/transitions/frames-timing-function.html:17 > + <script type="text/javascript"> type not needed > LayoutTests/transitions/frames-timing-function.html:28 > + var box = document.getElementById('box'); use const > LayoutTests/transitions/frames-timing-function.html:35 > +</head> > +<body> Remove > LayoutTests/transitions/frames-timing-function.html:38 > +The box should move horizontally 200px over 1s, in 4 equal increments. If so, why are you only testing 3? > LayoutTests/transitions/frames-timing-function.html:45 > +</body> > +</html> Remove. > LayoutTests/transitions/transitions-parsing.html:493 > +style.transitionTimingFunction = "frames(1)"; > +shouldBe("style.transitionTimingFunction", "''"); > +shouldBe("computedStyle.transitionTimingFunction", "'ease'"); > +shouldBe("style.webkitTransitionTimingFunction", "''"); > +shouldBe("computedStyle.webkitTransitionTimingFunction", "'ease'"); Add frames(0), frames(2.5), frames(eggs), frames(-10), "frames(" + Number.MAX_SAFE_INTEGER + ")" etc

Antoine Quint

Comment 5 2018-01-12 02:05:39 PST

(In reply to Dean Jackson from comment #4) > Comment on attachment 331185 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=331185&action=review > > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1572 > > + auto& function = static_cast<const FramesTimingFunction&>(timingFunction); > > Why isn't this a downcast? I was just following the existing style of that function. I'll update the function. > > Source/WebCore/css/CSSTimingFunctionValue.h:104 > > + CSSFramesTimingFunctionValue(int frames) > > Why is this an int? Use unsigned. Also, validate that it is > 0. Actually > > 1. This is validated at parsing time. > > Source/WebCore/css/parser/CSSPropertyParser.cpp:1493 > > + RefPtr<CSSPrimitiveValue> frames = consumePositiveInteger(args); > > + if (!frames) > > + return nullptr; > > + > > + int numberOfFrames = frames->intValue(); > > + if (numberOfFrames < 2) > > + return nullptr; > > Can use auto here. Will fix. > > Source/WebCore/platform/animation/TimingFunction.cpp:85 > > + auto numberOfFrames = function.numberOfFrames(); > > put in an ASSERT(numberOfFrames) OK. > > Source/WebCore/platform/animation/TimingFunction.h:49 > > bool isLinearTimingFunction() const { return m_type == LinearFunction; } > > bool isCubicBezierTimingFunction() const { return m_type == CubicBezierFunction; } > > bool isStepsTimingFunction() const { return m_type == StepsFunction; } > > + bool isFramesTimingFunction() const { return m_type == FramesFunction; } > > bool isSpringTimingFunction() const { return m_type == SpringFunction; } > > We should add traits for all these. Will do. > > Source/WebCore/platform/animation/TimingFunction.h:228 > > + static Ref<FramesTimingFunction> create(int frames) > > Again, maybe use unsigned? Sure. > > Source/WebCore/platform/animation/TimingFunction.h:235 > > + static Ref<FramesTimingFunction> create() > > + { > > + return adoptRef(*new FramesTimingFunction(2)); > > + } > > Is this ever used? I don't think so actually, will remove. > > LayoutTests/transitions/transitions-parsing.html:493 > > +style.transitionTimingFunction = "frames(1)"; > > +shouldBe("style.transitionTimingFunction", "''"); > > +shouldBe("computedStyle.transitionTimingFunction", "'ease'"); > > +shouldBe("style.webkitTransitionTimingFunction", "''"); > > +shouldBe("computedStyle.webkitTransitionTimingFunction", "'ease'"); > > Add frames(0), frames(2.5), frames(eggs), frames(-10), "frames(" + > Number.MAX_SAFE_INTEGER + ")" etc OK.

EWS Watchlist

Comment 6 2018-01-12 02:21:03 PST

Comment on attachment 331185 [details] Patch Attachment 331185 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6047705 New failing tests: imported/w3c/web-platform-tests/css-timing-1/frames-timing-functions-output.html imported/w3c/web-platform-tests/css-timing-1/frames-timing-functions-syntax.html

EWS Watchlist

Comment 7 2018-01-12 02:21:04 PST

Created attachment 331188 [details] Archive of layout-test-results from ews101 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6

EWS Watchlist

Comment 8 2018-01-12 02:26:45 PST

Comment on attachment 331185 [details] Patch Attachment 331185 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6047722 New failing tests: imported/w3c/web-platform-tests/css-timing-1/frames-timing-functions-output.html imported/w3c/web-platform-tests/css-timing-1/frames-timing-functions-syntax.html

EWS Watchlist

Comment 9 2018-01-12 02:26:46 PST

Created attachment 331189 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6

EWS Watchlist

Comment 10 2018-01-12 02:50:56 PST

Comment on attachment 331185 [details] Patch Attachment 331185 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/6047815 New failing tests: imported/w3c/web-platform-tests/css-timing-1/frames-timing-functions-output.html imported/w3c/web-platform-tests/css-timing-1/frames-timing-functions-syntax.html

EWS Watchlist

Comment 11 2018-01-12 02:50:57 PST

Created attachment 331190 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6

EWS Watchlist

Comment 12 2018-01-12 04:55:04 PST

Comment on attachment 331185 [details] Patch Attachment 331185 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/6048400 New failing tests: imported/w3c/web-platform-tests/css-timing-1/frames-timing-functions-output.html imported/w3c/web-platform-tests/css-timing-1/frames-timing-functions-syntax.html

EWS Watchlist

Comment 13 2018-01-12 04:55:06 PST

Created attachment 331191 [details] Archive of layout-test-results from ews112 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-sierra Platform: Mac OS X 10.12.6

Antoine Quint

Comment 14 2018-01-12 05:28:27 PST

https://trac.webkit.org/changeset/226886/webkit

Ahmad Saleem

Comment 15 2022-09-03 14:27:00 PDT

This landed and didn't backed out and I searched on Webkit Github using Bug ID: https://github.com/WebKit/WebKit/commit/71e35289d63db49d4e60c7a6c8ba920b1f50c5b4 and (from Comment 14) https://trac.webkit.org/changeset/226886/webkit I am going to mark this as "RESOLVED FIXED", if something else is required, please reopen or open dependent bugs. Thanks!

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component Animations

Assignee

Antoine Quint

Reported

2018-01-12 01:04 PST

Modified

2022-09-03 14:27 PDT History

CC List

5 users Show

URL

Keywords InRadar

Depends on

Blocks