Throw TypeErrors for invalid keyframe inputs #471
Conversation
|
I wonder if we should be deprecating these first before throwing TypeErrors. |
| 'Element.animate() does not accept keyframes with an out-of-bounded positive offset': | ||
| 'assert_throws: function "function () {\n"use strict";\n\n div.animate(subtest.input, 2000);\n }" did not throw', | ||
|
|
||
| // Seems to be a bug in Firefox 47? The TypeError is thrown but disappears by the time it bubbles up to assert_throws(). |
There was a problem hiding this comment.
Has a bug been filed against Firefox for this?
There was a problem hiding this comment.
It's a bit convoluted to reproduce in a bug report, given that Firefox is going to ship element.animate() in the next version I'm not sure it's worth the effort.
|
I don't think deprecation is necessary before the TypeErrors you're adding here, particularly since people would need to deliberately upgrade their version of the polyfill to get them (as opposed to just getting them by a push from us). |
| @@ -297,6 +301,7 @@ | |||
| return keyframes; | |||
| } | |||
|
|
|||
| shared.easingFunctionSymbol = window.Symbol ? Symbol('easingFunction') : '_easingFunction'; | |||
There was a problem hiding this comment.
As discussed offline, given that nothing else in the polyfill uses window.Symbol in this way, and it is not always available, I would rather just use _easingFunction throughout.
|
I found myself taking a careful look at the code in keyframe-interpolations.js to check whether each "easingFunction" was supposed to have an underscore to start or not. This suggests to me that we need to either (a) refactor the code to make it obvious when things are being mixed up (e.g. by having Interpolations and PropertySpecificKeyframes defined as separate classes in their own files), or (b) make them all the same, or (c) add tests to help catch stupid mistakes when changing this code. WDYT? |
|
It kind of is separated by files already. normalize-keyframes.js is processing user input and its output should in theory be able to be used again as user input, this is why we underscore it as it may get exposed to the user again. You've made me realise a potential bug where normalizeKeyframes() doesn't handle the presence of _easingFunction in a keyframe very well. |
|
Also I originally did mix up the use of underscore in this code accidentally, the mistake was caught by multiple tests as keyframe easing no longer functioned. |
|
Discussed offline that instead of storing _easingFunction on the normalised keyframe we can store it in a global cache that interpolations look up later on to reuse the parsing work. Reminder that parsing must happen in normalizeKeyframes() in order for easing TypeErrors to be thrown at the correct time. |
|
Turns out there's no need for caching. I've split toTimingFunction() up into normalizeEasing() and parseEasingFunction() which are called during normalisation and interpolation creation respectively as separate actions. PTAL. |
| if (preset) { | ||
| return preset; | ||
| } | ||
| // Easing is invalid, this should never be reached. |
There was a problem hiding this comment.
Nit: Elaborate this comment slightly (or add a comment to the start of the function) to explain that this is because parseEasingFunction has as a precondition that it be passed a valid, normalized easing string.
There was a problem hiding this comment.
Updated comment.
|
lgtm with nit |
Throw TypeErrors for the following scenarios:
This change builds upon #470.