In addition to the dependent bugs, before shipping we also need to: * Fix relevant spec issues[1] * Move more of our tests to web-platform-tests * Discuss incompatibilities with Google (e.g. cancel events) -- I am meeting with Google next week for this * Work out how to discard forwards-filling animations * Create a separate pref for the subset of functionality we intend to ship here * I also need to check if there are other timing properties (like iterationStart) that Google support and we don't [1] https://github.com/w3c/web-animations/issues?q=is%3Aissue+is%3Aopen+-label%3A%22level+2%22+-label%3A%22TAG+feedback%22+label%3AElement.animate

The subset of the API we intend to ship is roughly the same as what Google is shipping, specifically: enum AnimationPlayState { "idle", "pending", "running", "paused", "finished" }; [NoInterfaceObject] interface Animation : EventTarget { attribute DOMString id; attribute double? startTime; attribute double? currentTime; attribute double playbackRate; readonly attribute AnimationPlayState playState; readonly attribute Promise<Animation> ready; readonly attribute Promise<Animation> finished; attribute EventHandler onfinish; attribute EventHandler oncancel; void cancel (); void finish (); void play (); void pause (); void reverse (); }; enum FillMode { "none", "forwards", "backwards", "both", "auto" }; enum PlaybackDirection { "normal", "reverse", "alternate", "alternate-reverse" }; dictionary AnimationEffectTimingProperties { // I'd like to drop endDelay and iterationStart, assuming Chrome doesn't implement these double delay = 0.0; double endDelay = 0.0; FillMode fill = "auto"; double iterationStart = 0.0; unrestricted double iterations = 1.0; (unrestricted double or DOMString) duration = "auto"; PlaybackDirection direction = "normal"; DOMString easing = "linear"; }; // I'd prefer to leave this out if possible: the values are ignored dictionary KeyframeEffectOptions : AnimationEffectTimingProperties { IterationCompositeOperation iterationComposite = "replace"; CompositeOperation composite = "replace"; DOMString spacing = "distribute"; }; dictionary KeyframeAnimationOptions : KeyframeEffectOptions { DOMString id = ""; }; [NoInterfaceObject] interface Animatable { Animation animate(object? frames, optional (unrestricted double or KeyframeAnimationOptions) options); }; Element implements Animatable; Differences to what we expose on non-release channels: * Drops Animation constructor and marks Animation interface [NoInterfaceObject] * Drops Animation.timeline and Animation.effect (and corresponding interfaces) * Drops Animatable.getAnimations * Drops Document.timeline and Document.getAnimations Differences to what Chrome is shipping.[1] * Adds Animation.ready * Adds Animation.finished * Adds Animation.oncancel * Adds cancel events [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/animation/Animation.idl

Boris, I'd like your advice on how we should ship the Animation interface. The constraints we have are: * We don't know if shipping the Animation interface and constructor is Web-compatible because Chrome ships that interface as [NoInterfaceObject][1] * I'd be happy to try and ship it but I wouldn't want to have to back out (pref-off) the whole Element.animate implementation if we encountered compatibility issues * In non-release channels where the dom.animations-api.core.enabled pref is true, or the caller is chrome, we'd like to continue supporting the Animation constructor * We'd might like to ship Element.animate implementation behind a separate pref in case we need to turn it off after it hits beta (is this necessary?). i.e. the subset of features we're shipping should be available if *either* dom.animations-api.core.enabled is true or this new pref is true. Does any of that make sense? I guess my ideal solution is that we can switch between exposing Animation as either [NoInterfaceObject] or a regular interface with a constructor based on either of two prefs being true, or the caller being chrome. However, I suspect there's a much easier way. [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/animation/Animation.idl

I can't see an easier way: You want to enable all the stuff if chrome, but otherwise be able to enable/disable Animation and Element.animate independently of each other, right? Seems like you can just make the Func annotation on Animation be a function that checks for the state of the world you want (e.g. having it check for chrome caller or either of two prefs is easy enough), right?

(In reply to Boris Zbarsky [:bz] from comment #3) > I can't see an easier way: You want to enable all the stuff if chrome, but > otherwise be able to enable/disable Animation and Element.animate > independently of each other, right? > > Seems like you can just make the Func annotation on Animation be a function > that checks for the state of the world you want (e.g. having it check for > chrome caller or either of two prefs is easy enough), right? The part I'm most concerned about is switching between [NoInterfaceObject] and [Constructor ...] based on prefs. I didn't think that was possible, even with a Func annotation. Cameron mentioned we might just be able to use #ifdef to make it [NoInterfaceObject] on release channels but that will mean it is impossible to use the constructor on release channels so might break DevTools/test infrastructure. I'll see what I can do.

> The part I'm most concerned about is switching between [NoInterfaceObject] and [Constructor ...] based on > prefs. You can't do that. Luckily, I don't think you need to. Let's consider this snippet of IDL: [NoInterfaceObject] interface A {}; [Constructor, Func="alwaysReturnFalse"] interface B {}; window.A is undefined, because A is [NoInterfaceObject]. window.B is undefined, because the [Func] listed returns false. The only difference I can think of here is that if you have an instance of A, call it "insta", then |new insta.__proto__.constructor()| will throw, while if you have an instance of B, call it "instb", then |new instb.__proto__.constructor()| will create a B. If you don't care about that, then Func will do what you want.

(In reply to Boris Zbarsky [:bz] from comment #5) > > The part I'm most concerned about is switching between [NoInterfaceObject] and [Constructor ...] based on > > prefs. > > You can't do that. Luckily, I don't think you need to. > > Let's consider this snippet of IDL: > > [NoInterfaceObject] interface A {}; > [Constructor, Func="alwaysReturnFalse"] interface B {}; > > window.A is undefined, because A is [NoInterfaceObject]. window.B is > undefined, because the [Func] listed returns false. > > The only difference I can think of here is that if you have an instance of > A, call it "insta", then |new insta.__proto__.constructor()| will throw, > while if you have an instance of B, call it "instb", then |new > instb.__proto__.constructor()| will create a B. If you don't care about > that, then Func will do what you want. Great. That's perfect. I was under the impression that a failing Func annotationon the interface would turn off the entire interface but it seems like that's not the case.

Ah, no. All the Func does on an interface is control whether it's exposed as a property on the global. That's it.

Update on the API we intend to expose: we have decided not to ship the 'finished' promise due to concern that we should wait for cancelable promises.[1] [1] https://github.com/w3c/web-animations/issues/141 Also, the issue raised in comment 1 about endDelay and iterationStart can be ignored since Chrome implements them and now so do we. Chrome has implemented the other missing pieces from comment 1.

Hi Boris, I'm trying to add a preference to conditionally turn on Element.animate. I'll file a separate patch to turn the pref on for release channels after we resolve the outstanding issues with the polyfill and after sending an Intent to Ship (assuming there are no blocking issues!). This preference turns on a subset of the existing dom.animations-api.core.enabled pref so if that pref is set we ignore the setting of dom.animations-api.element-animate.enabled (since trying to support the rest of the API *without* Element.animate doesn't seem worthwhile). One issue I've noticed, however, is that with: dom.animations-api.core.enabled = false dom.animations-api.element-animate.enabled = true I see the following behavior in DevTools console window: >> window.Animation <- undefined >> $0.animate(....) <- Animation { ... } >> window.Animation <- function () Is that expected?

Comment on attachment 8741587 [details] [diff] [review] Add a preference for enabling Element.animate The behaviour you see is somewhat expected, yes. Once you allow creation of a Foo object, there's no real point in hiding the window.Foo interface object, especially because consumers can just get it by doing foo.constructor. So once you create the Foo object, we go ahead and define window.Foo, unless the interface is [NoInterfaceObject]. Which also means that this patch is somewhat backward: The exposure function for Animation should really be returning true any time the exposure function for Element.animate will.... Now what we could do is have the Animation constructor throw if it's called by the relevant prefs are not set or something.

(In reply to Boris Zbarsky [:bz] from comment #10) > Comment on attachment 8741587 [details] [diff] [review] > Add a preference for enabling Element.animate > > The behaviour you see is somewhat expected, yes. Once you allow creation of > a Foo object, there's no real point in hiding the window.Foo interface > object, especially because consumers can just get it by doing > foo.constructor. So once you create the Foo object, we go ahead and define > window.Foo, unless the interface is [NoInterfaceObject]. > > Which also means that this patch is somewhat backward: The exposure function > for Animation should really be returning true any time the exposure function > for Element.animate will.... Ok, my main concern is just that I'd like to *not* expose window.Animation on release channels yet in case there is a library out there defining window.Animation. However, if a site calls elem.animate() then it's probably less likely to be also defining window.Animation. As a result defining window.Animation *after* a call to elem.animate() like we do here is probably less of a compatibility concern. Chrome ships Element.animate without defining window.Animation by using [NoInterfaceObject] since they don't implement the Animation constructor yet.[1] [1] https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Source/core/animation/Animation.idl#38 > Now what we could do is have the Animation constructor throw if it's called > by the relevant prefs are not set or something. I think I'm missing something. I don't know if that helps with the compatibility issue?

Ah, I see. Yes, having the constructor throw would not help with the compat issue. It would help with What might help is that if the site defines window.Animation before Element.animate is called, we will NOT override its definition. So the only issue would be if a site calls elem.animate() and then checks whether window.Animation is defined.

(In reply to Boris Zbarsky [:bz] from comment #12) > Ah, I see. Yes, having the constructor throw would not help with the compat > issue. It would help with > > What might help is that if the site defines window.Animation before > Element.animate is called, we will NOT override its definition. So the only > issue would be if a site calls elem.animate() and then checks whether > window.Animation is defined. Ok, so perhaps the patch as-is is ok? That latter case seems fairly unlikely. I think allowing code to access anim.constructor is probably fine although I can see that throwing an exception there could help if we think that constructor might change (and there is an open issue[1] on that constructor in the spec although the suggested change would be backwards compatible). [1] http://w3c.github.io/web-animations/#issue-0d7a37da

Yeah, if we're not planning to keep shipping the weird setup for a while the patch is probably OK as is. We'd want to throw when the constructor is actually called, if we decide to do that, not on .constructor access.

(In reply to Boris Zbarsky [:bz] from comment #14) > Yeah, if we're not planning to keep shipping the weird setup for a while the > patch is probably OK as is. Ok, I'll re-request review on that patch (after appending 'part 1' to the title) > We'd want to throw when the constructor is actually called, if we decide to > do that, not on .constructor access. I decided to add a patch for this. The main concern is that even if that spec doesn't change, the implementation of the constructor is not quite complete. In particular, it doesn't currently support a null argument for the target (currently being fixed in bug 1067769). That's not a big deal I suppose but perhaps it's best to just throw for now and then ship it properly all at once later.

MozReview-Commit-ID: H3HPYWeyGCL

MozReview-Commit-ID: 1pCgfw4PFHY

Comment on attachment 8741645 [details] [diff] [review] part 2 - Throw if Animation constructor is called when dom.animations-api.core.enabled is not true (or the caller is not chrome) Actually, scratch that. That won't work because Element.animate() calls the constructor. I don't think we need to worry about this after all since the constructor takes a nullable AnimationEffectReadOnly argument as its first parameter and when dom.animations-api.core.enabled is false, it won't be possible create a suitable object and we don't support passing null for that argument yet so no matter what you do, calling anim.constructor will throw already.

Comment on attachment 8741644 [details] [diff] [review] part 1 - Add a preference for enabling Element.animate r=me

What are the needs for testing this (manual, automation)?

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #22) > What are the needs for testing this (manual, automation)? We have automated tests in web-platform-tests and other mochitests/crashtests. I have contact the fuzzing team to ask for their assistance with fuzzing this API but have yet to receive any concrete commitment. Any help getting fuzzing happening would be much appreciated! In particular we only need to test the configuration where we have: dom.animations-api.core.enabled = false dom.animations-api.element-animate.enabled = true

Like Animation.finished, this will likely change to a cancelable promise in the future (assuming such things materialize) so we should not ship it for the time being.

Intent to ship: https://groups.google.com/d/topic/mozilla.dev.platform/2INRr96R3IU/discussion

Comment on attachment 8742624 [details] [diff] [review] part 2 - Don't ship Animation.ready r=me

Comment on attachment 8742625 [details] [diff] [review] part 3 - Turn on Element.animate in release channels too r=me, but we should sort out the constructor thing that smaug is having issues with before we actually ship this...

> Yeah, if we're not planning to keep shipping the weird setup for a while the patch is probably OK as is. To be clear, do we have an estimate for when we would ship the constructor by default? Are we talking another cycle or two? Or are we talking a long time?

(In reply to Boris Zbarsky [:bz] from comment #28) > > Yeah, if we're not planning to keep shipping the weird setup for a while the patch is probably OK as is. > > To be clear, do we have an estimate for when we would ship the constructor > by default? Are we talking another cycle or two? Or are we talking a long > time? I'm happy to ship it at the same time under a separate pref. There are no specific compatibility concerns. It's just a very generic name and a number of people like Dean Jackson have commented, "Are you sure you won't have clashes with the name 'Animation'?". I personally don't have any kind of sense for how likely this is. However, it would be unfortunate to have to disable the whole Element.animate feature if compatibility issues did arise once this hit, say, beta. From what I understand, the reason Chrome shipped as [NoInterfaceObject] was not due to compatibility concerns as much as simply not having implemented the constructor. I'm following up with them now to confirm.

I think we should either ship without ctor and with NoInterfaceObject, or with ctor and normal interface. The setup where there is ctor and NoInterfaceObject is just super weird API, and I don't see what we would achieve by shipping that. It doesn't help with testing compatibility or anything. We could just comment out the Constructor part in the .webidl, no?

(In reply to Olli Pettay [:smaug] from comment #30) > I think we should either ship without ctor and with NoInterfaceObject, or > with ctor and normal interface. The setup where there is ctor and > NoInterfaceObject is just super weird API, and I don't see what we would > achieve by shipping that. It doesn't help with testing compatibility or > anything. > We could just comment out the Constructor part in the .webidl, no? Yes, I just followed up on dev-platform to that effect. The purpose of the weird setup was not to test compatibility but just to ship Element.animate to web developers without the risk of having to turn it off if compatibility issues do arise. Unfortunately commenting out the Constructor will prevent us from testing a number of features so I'd rather not do that on trunk. I'm going to see what I can find out about possible compatibility risks and if there's nothing too compelling I think we should just ship with the ctor and normal interface.

Still waiting to see if I can find out any specific compatibility issues from the blink guys. There is nothing on their tracker suggesting compatibility was a concern when they went with [NoInterfaceObject]: https://codereview.chromium.org/238633002#msg4 Looking at GitHub there are a few places that test for window.Animation but none that I think would break if window.Animation were defined: https://github.com/jamiegilmartin/Animator/blob/7d0ef95441e74e572d60ad9f23821e35eadc26f1/assets/js/animation.coffee#L1 https://github.com/BasalticStudio/Walrus-vs-Slime/blob/36b96ec01ac5ced98bf529d28fb2389920b2ce63/js/Animation.js#L5 Also, we have been shipping window.Animation on Nightly since mid-January 2016 and Aurora since 46 (late January 2016) and I haven't heard of any compatibility issues. One of the Chrome guys who might know is away today so I'll see what I can find out from him tomorrow.

Hi Boris, what do you think of this? I still haven't been able to get in touch with my counterpart in the Chrome team but assuming I do, and assuming there are no specific concerns, I'm thinking to just ship the Animation constructor along with Element.animate and hope for the best. I've tidied up the error messages for the constructor a little although the constructor itself is still completely useless without the other interfaces turned on (we don't expose the constructor for creating an effect nor a timeline, or even document.timeline) and we don't yet support passing null for those arguments.

Comment on attachment 8743702 [details] [diff] [review] part 4 - Enable the Animation constructor when Element.animate is enabled r=me

Using the links Olli dug up on #whatwg I tried querying HAR to find sites that are testing for window.Animation being defined. Initially I used a very naive query, SELECT page, url FROM ( SELECT page, url, JSON_EXTRACT(payload, '$._body') AS hasBody, JSON_EXTRACT(payload, '$._contentType') AS contentType, JSON_EXTRACT(payload, '$.response.content.text') AS content FROM [httparchive:har.chrome_feb_1_2016_requests] ) WHERE hasBody = 'true' AND (contentType CONTAINS 'html' OR contentType CONTAINS 'javascript') AND content CONTAINS 'window.Animation' However, that turned up a lot of matches for things like window.AnimationUpdater. When I went to refine the query with regexp I found I had already reached my query quote. Nevertheless I analyzed the results of the initial query here: https://docs.google.com/spreadsheets/d/1oSjpxDzZmCqlHjQtrUTT5u9Ej-GE_ANOjlNSIyuE7Zw/edit?usp=sharing Of the 90 results, only 5 actually assign to window.Animation. 3 of them are using the Web Animations polyfill and all of them blindly assign to window.Animation without testing if it is defined first and so, presumably, would not be broken if this were already defined. This doesn't include tests for 'if (Animation)' etc. however. Only for 'window.Animation'. It would be interesting to test for 'if (Animation)', 'if (window.Animation)' etc. too. I'll see if I can find someone to run a query or two on my behalf. So far, however, there aren't any obvious compatibility concerns so I might just land the remaining patches in this bug and keep probing.

(In reply to Brian Birtles (:birtles) from comment #35) > However, that turned up a lot of matches for things like > window.AnimationUpdater. When I went to refine the query with regexp I found > I had already reached my query quote. I got some help from Chrome folks to run an updated query matching /window\.Animation\W/ and it returned the same give results I analyzed above. Also, querying for "if (Animation)" returned 0 results. That's not all the possibilities, of course, but it gives me some confidence that a globally-defined Animation is not common.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5d652fb84d1 https://hg.mozilla.org/integration/mozilla-inbound/rev/59aa5395fff1 https://hg.mozilla.org/integration/mozilla-inbound/rev/aa19babdbac3

https://hg.mozilla.org/mozilla-central/rev/c5d652fb84d1 https://hg.mozilla.org/mozilla-central/rev/59aa5395fff1 https://hg.mozilla.org/mozilla-central/rev/aa19babdbac3

This was documented at https://developer.mozilla.org/en-US/docs/Web/API/Element/animate and added to the release notes at https://developer.mozilla.org/en-US/Firefox/Releases/48. Sebastian

Release Note Request (optional, but appreciated) [Why is this notable]: Major new API (the first piece of the Web Animations API) which allows authors to create animations from Javascript with the same performance characteristics as CSS animations/transitions (can be run on the compositor etc.) and perform playback control on them. [Suggested wording]: Added support for Element.animate() [Links (documentation, blog post, etc)]: https://developer.mozilla.org/en-US/docs/Web/API/Element/animate

Added to Fx48 Aurora release notes.