Add try-catch guard to object-form loading check #434
Conversation
The object-form-keyframes source file uses a call to Element.animate to decide whether to load the polyfill for the object-form syntax. If the native browser code supports Element.animate without full support for object-form syntax, this test can throw a TypeError (see web-animations/web-animations-js#72). In this case, the polyfill should proceed to load the polyfill. This patch implements this behaviour with a try-catch guard around the Element.animate loading check.
| var animation = element.animate({'opacity': ['1', '0']}, | ||
| {duration: 1, fill: 'forwards'}); | ||
| animation.finish(); | ||
| animated = getComputedStyle(element).getPropertyValue('opacity') == '0'; |
There was a problem hiding this comment.
I'm concerned that if the browser throws an exception here the entire document will be hidden. I think the check should use opacity: [0, 1], not specify fill: 'forwards' and seek currentTime to 0 instead.
There was a problem hiding this comment.
We should ensure that if an animation is created, it is cancelled, regardless of whether an exception is thrown or not.
There was a problem hiding this comment.
Rewritten, with an extensive comment, because I felt the reasoning behind the structure was not clear. PTAL.
In response to review comments, this patch rewrites the test animation in the object-form-keyframes load check, to be more definite that it will leave the element opacity untouched regardless of the outcome. Since the logic is now not obvious, a lengthy explanatory comment is also added.
|
lgtm. The comment is a bit more "what" than "why" for my liking but it's not a huge deal. |
|
lgtm, but I'll leave a few nits that you might want to clean up. |
| var animated = getComputedStyle(element).getPropertyValue('opacity') == '0'; | ||
| animation.cancel(); | ||
| var animation = null; | ||
| var animated = true; |
|
Thanks for the reviews, all done. If you have any specific suggestions for how to improve the comment, I'll incorporate them, but I'm not sure what else to add to clarify the 'why'. |
The object-form-keyframes source file uses a call to Element.animate
to decide whether to load the polyfill for the object-form syntax.
If the native browser code supports Element.animate without full
support for object-form syntax, this test can throw a TypeError (see
web-animations/web-animations-js#72). In
this case, the polyfill should proceed to load the polyfill. This
patch implements this behaviour with a try-catch guard around the
Element.animate loading check.