Closed Bug 1332206 Opened 9 years ago Closed 8 years ago

steps() should not clamp its input to [0, 1]

Categories

(Core :: DOM: Animation, defect, P3)

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(19 files, 1 obsolete file)

Test case
1.51 KB, text/html
Details
Test case for negative direction
1.54 KB, text/html
Details
WIP patch
2.58 KB, patch
Details | Diff | Splinter Review
Bug 1332206 - Split effect value tests into separate files;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1332206 - Tidy up visibility tests to use single quotes and drop periods at end of test assertions;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1332206 - Move tests for effect easing to timing-model;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1332206 - Rework tests for linear-equivalent cubic-bezier timing functions from effect-easing.html;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1332206 - Rename effect easing tests to easing tests;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1332206 - Simplify invalid easing tests;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1332206 - Add tests for keyframe easing;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1332206 - Move step timing function tests;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1332206 - Drop step timing function for compositor animations;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1332206 - Drop checking width in step timing function tests;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1332206 - Add missing tests for steps-end in delay phase;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1332206 - Move invalid easing tests to the appropriate interface tests;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1332206 - Drop some unnecessary calls to pause();
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1332206 - Add tests for non-clamping step timing function behavior;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1332206 - Move tests for timing functions with inputs outside [0, 1];
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1332206 - Don't clamp steps timing functions outside [0, 1] range;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Attached file Test case —
The step timing function currently clamps its input to the range [0, 1] but that produces unexpected results when combined with timing functions that overshoot the [0, 1] range. For an example, see the attached test case. In the middle animation, due to the way it combines steps() and cubic-bezier() it loses the overshoot behavior it should have.
Demonstrates the same problem in the negative direction.
Attached patch WIP patch — — Splinter Review
Here's a very rough patch that just drops clamping. It needs tests and I suspect it might need special handling for some of the edge cases, especially around zero. (I think when I tested it, even with steps-end it was calculating a current step of zero for negative progress which doesn't seem right.)
Priority: -- → P3
Blocks: 1248340
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
I'll wait to see what try makes of these patches but if they're ok I still need to: * Update the spec to do the special "don't go outside [0, 1] unless the input does" behavior. * Probably re-run --manifest-update on each patch since I manually dropped some of the changes there that appeared to be redundant. I'm really not sure if they should stay or not, however. * Go through and drop the underlying manifest update patch before repushing the series.
Comment on attachment 8833198 [details] Bug 1332206 - Split effect value tests into separate files; https://reviewboard.mozilla.org/r/109410/#review111388
Attachment #8833198 - Flags: review?(hikezoe) → review+
Comment on attachment 8833199 [details] Bug 1332206 - Tidy up visibility tests to use single quotes and drop periods at end of test assertions; https://reviewboard.mozilla.org/r/109412/#review111390
Attachment #8833199 - Flags: review?(hikezoe) → review+
Comment on attachment 8833200 [details] Bug 1332206 - Move tests for effect easing to timing-model; https://reviewboard.mozilla.org/r/109414/#review111392 Yay! I like arrows!
Attachment #8833200 - Flags: review?(hikezoe) → review+
Comment on attachment 8833201 [details] Bug 1332206 - Rework tests for linear-equivalent cubic-bezier timing functions from effect-easing.html; https://reviewboard.mozilla.org/r/109416/#review111394 ::: testing/web-platform/tests/web-animations/resources/effect-easing-tests.js:71 (Diff revision 2) > + { > + desc: 'easing function which produces values less than 1', > + easing: 'cubic-bezier(0, -0.5, 1, -0.5)', > + easingFunction: cubicBezier(0, -0.5, 1, -0.5) > } I don't quite understand why this test case is added in this patch.
Comment on attachment 8833201 [details] Bug 1332206 - Rework tests for linear-equivalent cubic-bezier timing functions from effect-easing.html; https://reviewboard.mozilla.org/r/109416/#review111396 Other parts look good to me.
Attachment #8833201 - Flags: review?(hikezoe) → review+
Comment on attachment 8833201 [details] Bug 1332206 - Rework tests for linear-equivalent cubic-bezier timing functions from effect-easing.html; https://reviewboard.mozilla.org/r/109416/#review111394 > I don't quite understand why this test case is added in this patch. The purpose of these tests appears to be to check that a linear-equivalent cubic-bezier timing function (e.g. 'cubic-bezier(0, 0, 0, 0)') does not affect the result such as clamping values out of the [0, 1] range. However, it fails to test the negative range so this adds a test for that range.
Comment on attachment 8833202 [details] Bug 1332206 - Rename effect easing tests to easing tests; https://reviewboard.mozilla.org/r/109418/#review111398
Attachment #8833202 - Flags: review?(hikezoe) → review+
Comment on attachment 8833203 [details] Bug 1332206 - Simplify invalid easing tests; https://reviewboard.mozilla.org/r/109420/#review111402 Nice!
Attachment #8833203 - Flags: review?(hikezoe) → review+
Comment on attachment 8833204 [details] Bug 1332206 - Add tests for keyframe easing; https://reviewboard.mozilla.org/r/109422/#review111404 ::: testing/web-platform/tests/web-animations/animation-model/keyframe-effects/effect-value-transformed-distance.html:29 (Diff revision 2) > + { width: '100px', easing: params.easing }, > + { width: '200px' } ], > + { duration: 2000, > + fill: 'forwards' }); > + > + [ 0, 999, 1000, 1100, 1500, 2000 ].forEach(sampleTime => { I wonder why 1100 and 1500 were chosen. But the test looks good to me.
Attachment #8833204 - Flags: review?(hikezoe) → review+
Comment on attachment 8833204 [details] Bug 1332206 - Add tests for keyframe easing; https://reviewboard.mozilla.org/r/109422/#review111404 > I wonder why 1100 and 1500 were chosen. But the test looks good to me. 1500 is because it represents a boundary condition for many step timing functions. 1100 is because for some other types of timing functions, the 50% value is equal to a linear timing function (e.g. 'cubic-bezier(0,1,1,0)') so 1500 is not a good value for testing those, but 1100 (10%) is.
Comment on attachment 8833205 [details] Bug 1332206 - Move step timing function tests; https://reviewboard.mozilla.org/r/109424/#review111408
Attachment #8833205 - Flags: review?(hikezoe) → review+
Comment on attachment 8833206 [details] Bug 1332206 - Drop step timing function for compositor animations; https://reviewboard.mozilla.org/r/109426/#review111410
Attachment #8833206 - Flags: review?(hikezoe) → review+
Comment on attachment 8833207 [details] Bug 1332206 - Drop checking width in step timing function tests; https://reviewboard.mozilla.org/r/109428/#review111412
Attachment #8833207 - Flags: review?(hikezoe) → review+
Comment on attachment 8833213 [details] Bug 1332206 - Move tests for timing functions with inputs outside [0, 1]; https://reviewboard.mozilla.org/r/109440/#review111416
Attachment #8833213 - Flags: review?(hikezoe) → review+
Comment on attachment 8833210 [details] Bug 1332206 - Move invalid easing tests to the appropriate interface tests; https://reviewboard.mozilla.org/r/109434/#review111420 ::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/constructor.html:88 (Diff revision 2) > + assert_throws(new TypeError, () => { > + new KeyframeEffectReadOnly(target, null, { easing: invalidEasing }); > + }, `TypeError is thrown for easing '${invalidEasing}'`); > + }); > +}, 'invalid easing values are correctly rejected when passed to the ' + > + 'KeyframeEffectReadOnly constructor in KeyframeTimingOptions'); KeyframeEffectOptions or AnimationEffectTimingProperties is a proper word here?
Attachment #8833210 - Flags: review?(hikezoe) → review+
Comment on attachment 8833211 [details] Bug 1332206 - Drop some unnecessary calls to pause(); https://reviewboard.mozilla.org/r/109436/#review111422
Attachment #8833211 - Flags: review?(hikezoe) → review+
Comment on attachment 8833212 [details] Bug 1332206 - Add tests for non-clamping step timing function behavior; https://reviewboard.mozilla.org/r/109438/#review111434 ::: testing/web-platform/tests/web-animations/animation-model/keyframe-effects/effect-value-transformed-distance.html:107 (Diff revision 2) > { left: '100px' } ], > { duration: 1000, > fill: 'forwards', > easing: 'cubic-bezier(0, 1.5, 1, 1.5)' }); > > // The bezier function produces values greater than 1 in (0.23368794, 1) Please also modify this comment to represent the value is always less than 2. ::: testing/web-platform/tests/web-animations/animation-model/keyframe-effects/effect-value-transformed-distance.html:207 (Diff revision 2) > { left: '100px' } ], > { duration: 1000, > fill: 'forwards', > easing: 'cubic-bezier(0, -0.5, 1, -0.5)' }); > > // The bezier function produces negative values in (0, 0.766312060) Likewise here.
Attachment #8833212 - Flags: review?(hikezoe) → review+
IIUC, these patch set (the last one and 12 of 17?) is also reflected by this spec change: https://github.com/w3c/csswg-drafts/commit/00c780fed05449021eefc644c066b695f3b516ac
Comment on attachment 8833208 [details] Bug 1332206 - Add missing tests for steps-end in delay phase; https://reviewboard.mozilla.org/r/109430/#review111440
Attachment #8833208 - Flags: review?(hikezoe) → review+
Comment on attachment 8834270 [details] Bug 1332206 - Don't clamp steps timing functions outside [0, 1] range; https://reviewboard.mozilla.org/r/110266/#review111442 ::: dom/animation/ComputedTimingFunction.cpp:50 (Diff revision 1) > - return double(step) / double(aSteps); > + double result = double(step) / double(aSteps); > + > + // We should not produce a result outside [0, 1] unless we have an > + // input outside that range. This takes care of steps that would otherwise > + // occur at boundaries. > + if (result < 0.0 && aPortion >= 0.0) { > + return 0.0; > + } > + if (result > 1.0 && aPortion <= 1.0) { > + return 1.0; > + } > + > + return result; Unlike the spec, we are converting to the progress value before clamping values at boundaries. Concerened about calculation error?
Attachment #8834270 - Flags: review?(hikezoe) → review+
Comment on attachment 8834270 [details] Bug 1332206 - Don't clamp steps timing functions outside [0, 1] range; https://reviewboard.mozilla.org/r/110266/#review111442 > Unlike the spec, we are converting to the progress value before clamping values at boundaries. Concerened about calculation error? Yes, I was concerned about comparing step (signed int) and aSteps (unsigned int) and it just end up being simpler to compare the calculated result.
Comment on attachment 8833210 [details] Bug 1332206 - Move invalid easing tests to the appropriate interface tests; https://reviewboard.mozilla.org/r/109434/#review111420 > KeyframeEffectOptions or AnimationEffectTimingProperties is a proper word here? Should be KeyframeEffectOptions. Fixed locally.
Attachment #8833209 - Attachment is obsolete: true
Pushed by bbirtles@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/03f791909447 Split effect value tests into separate files; r=hiro https://hg.mozilla.org/integration/autoland/rev/a873e39adee4 Tidy up visibility tests to use single quotes and drop periods at end of test assertions; r=hiro https://hg.mozilla.org/integration/autoland/rev/63488b90e12a Move tests for effect easing to timing-model; r=hiro https://hg.mozilla.org/integration/autoland/rev/d349491c67e4 Rework tests for linear-equivalent cubic-bezier timing functions from effect-easing.html; r=hiro https://hg.mozilla.org/integration/autoland/rev/1ab8568a47c8 Rename effect easing tests to easing tests; r=hiro https://hg.mozilla.org/integration/autoland/rev/743c0869e332 Simplify invalid easing tests; r=hiro https://hg.mozilla.org/integration/autoland/rev/5f16ee5b6238 Add tests for keyframe easing; r=hiro https://hg.mozilla.org/integration/autoland/rev/1a24ff557466 Move step timing function tests; r=hiro https://hg.mozilla.org/integration/autoland/rev/41114ebea8ef Drop step timing function for compositor animations; r=hiro https://hg.mozilla.org/integration/autoland/rev/d3c587e09aa2 Drop checking width in step timing function tests; r=hiro https://hg.mozilla.org/integration/autoland/rev/3c6c8e38af53 Add missing tests for steps-end in delay phase; r=hiro https://hg.mozilla.org/integration/autoland/rev/3fb9bacf3937 Move tests for timing functions with inputs outside [0, 1]; r=hiro https://hg.mozilla.org/integration/autoland/rev/be72058340fe Move invalid easing tests to the appropriate interface tests; r=hiro https://hg.mozilla.org/integration/autoland/rev/d18d24e941e1 Drop some unnecessary calls to pause(); r=hiro https://hg.mozilla.org/integration/autoland/rev/f85b73bd0790 Add tests for non-clamping step timing function behavior; r=hiro https://hg.mozilla.org/integration/autoland/rev/e624d16a7d8f Don't clamp steps timing functions outside [0, 1] range; r=hiro
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: