Closed Bug 1253507 Opened 8 years ago Closed 8 years ago

Disable Element.animate on Firefox 47

Categories

(Core :: DOM: Animation, defect)

Product:
Core
Component:
DOM: Animation
Version:
47 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla47
Tracking Flags:
Tracking Status
firefox47 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 4 obsolete files)

Disable Element.animate in Firefox 47
14.45 KB, patch
Details | Diff | Splinter Review 
Our implementation of Element.animate breaks properties that use polymer animation due to bug 1252599. See, e.g. bug 1247004 and bug 1252599.

We don't expect bug 1252599 to land before the next uplift which means these sites will break on DevEdition next week. The API will be turned off once it hits beta but we should still probably back it out on 47.

I think we just want to comment out all the API and disable / mark as failing any tests using Element.animate() --- which might be quite a few.
Initial WIP pach
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Note that in devtools, test:
devtools\client\animationinspector\test\browser_animation_playerWidgets_appear_on_panel_init.js
which loads the test page:
devtools\client\animationinspector\test\doc_multiple_animation_types.html
uses `element.animate()` but sets "dom.animations-api.core.enabled" to true before. Not sure if this is important to know in this bug?
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #3)
> Note that in devtools, test:
> devtools\client\animationinspector\test\browser_animation_playerWidgets_appea
> r_on_panel_init.js
> which loads the test page:
> devtools\client\animationinspector\test\doc_multiple_animation_types.html
> uses `element.animate()` but sets "dom.animations-api.core.enabled" to true
> before. Not sure if this is important to know in this bug?

Great, thanks Patrick!

I've fixed that in this updated try run but no doubt it will turn up a lot of other bugs from all the stuff that landed over the weekend:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=65c5e8f0c93e

Not sure what that ESLint errors are, either.
Attachment #8726605 - Attachment is obsolete: true
(In reply to Brian Birtles (:birtles) from comment #4)
> Not sure what that ESLint errors are, either.
Ah, indeed, ESLint doesn't yet know that KeyframeEffect and Animation are globals in a browser environment, so it complains about them being undefined (the rest of the ESLint job are just warnings, you an ignore them).
So here:
https://hg.mozilla.org/try/file/02011e0bc919/devtools/client/animationinspector/test/doc_multiple_animation_types.html#l52
and here:
https://hg.mozilla.org/try/file/02011e0bc919/devtools/client/animationinspector/test/doc_multiple_animation_types.html#l56
ESLint complains about KeyframeEffect and Animation not being defined.

I think our only option is just to add a special ESLint comment at the top of the <script> tag:

/* globals KeyframeEffect, Animation */
Did we ship Element.animate in 47?
Flags: needinfo?(bbirtles)
(In reply to Andrew Overholt [:overholt] from comment #7)
> Did we ship Element.animate in 47?

Currently Element.animate is turned on in 47 (along with the rest of the Web Animations API) but will be turned off automatically when it hits beta. I am planning on turning off just Element.animate in Aurora to avoid the compatibility issues with polymer.
Flags: needinfo?(bbirtles)
MozReview-Commit-ID: CJm4SUEw832
Attachment #8727272 - Attachment is obsolete: true
Comment on attachment 8731093 [details] [diff] [review]
Disable Element.animate in Firefox 47

Boris, does this make sense?

Patrick, can you check the changes to the DevTools tests?
Attachment #8731093 - Flags: review?(pbrosset)
Attachment #8731093 - Flags: review?(bzbarsky)
Comment on attachment 8731093 [details] [diff] [review]
Disable Element.animate in Firefox 47

Review of attachment 8731093 [details] [diff] [review]:
-----------------------------------------------------------------

R+ for the changes in devtools/client/animationinspector/test/doc_multiple_animation_types.html
Attachment #8731093 - Flags: review?(pbrosset) → review+
Comment on attachment 8731093 [details] [diff] [review]
Disable Element.animate in Firefox 47

So this is a patch for Aurora only?  If so, r=me with one nit: the bug# listed for "disabled" annotations for web platform tests should be the bug that needs to be fixed to reenable the test, generally.
Attachment #8731093 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #12)
> Comment on attachment 8731093 [details] [diff] [review]
> Disable Element.animate in Firefox 47
> 
> So this is a patch for Aurora only?  If so, r=me with one nit: the bug#
> listed for "disabled" annotations for web platform tests should be the bug
> that needs to be fixed to reenable the test, generally.

Yes, Aurora only. We don't plan to reenable the test on Aurora so I'm not sure if there is a suitable bug number to be used here?

The situation is we have the Web Animations API enabled by default on non-release channels. However, we implemented Element.animate on trunk during the 47 cycle but it has some limitations that conflict with sites that use Polymer which uses the native implementation of Element.animate if it finds it. We'll fix those limitations in the 48 cycle (bug 1245748 or possibly a follow-up) but we don't plan to uplift that fix to 47 since the whole API will be switched off when 47 hits beta and we don't plan to switch on Element.animate in release channels until 48.

So this is just a patch to allow developers using Developer Edition to continue to browse sites that use polymer during the current cycle.
> so I'm not sure if there is a suitable bug number to be used here?

Use the bug that caused us to disable (e.g. bug 1245748)?
MozReview-Commit-ID: CJm4SUEw832
Attachment #8731093 - Attachment is obsolete: true
Comment on attachment 8731552 [details] [diff] [review]
Disable Element.animate in Firefox 47

Approval Request Comment
[Feature/regressing bug #]: bug 1096773
[User impact if declined]: Sites like Google play music that use polymer may fail to animate or break due to an unhandled Javascript exception (e.g. bug 1252599, bug 1247004, bug 1252599)
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1f928ba8952
[Risks and why]: Low. There may be unintended compatibility issues but they are hard to imagine. Any site expecting Firefox 47 to support Element.animate will be disappointed when Firefox 47 hits beta.
[String/UUID change made/needed]: None
Attachment #8731552 - Flags: approval-mozilla-aurora?
Brian, will this not be disabled in Nightly 48?
Flags: needinfo?(bbirtles)
(In reply to Ritu Kothari (:ritu) from comment #17)
> Brian, will this not be disabled in Nightly 48?

No, just 47. We will fix the limitation that affects Polymer in 48.
Flags: needinfo?(bbirtles)
Comment on attachment 8731552 [details] [diff] [review]
Disable Element.animate in Firefox 47

Taking it based on user impact related to google play music. Aurora47+
Attachment #8731552 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/77d83926c43d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2196702&repo=mozilla-aurora
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
These are ESLint failures:
 TEST-UNEXPECTED-ERROR | devtools/client/animationinspector/test/doc_multiple_animation_types.html:52:22 | "KeyframeEffect" is not defined. (no-undef)

TEST-UNEXPECTED-ERROR | devtools/client/animationinspector/test/doc_multiple_animation_types.html:56:25 | "Animation" is not defined. (no-undef) 

Which can be easily fixed by adding the following comment in the <script> tag in this html file:

/* globals KeyframeEffect, Animation */

Indeed, ESLint isn't aware that these are available globals in a window environment, so it considers them as undefined.
Which, I realized now, is what I had said in comment 6.
(In reply to Patrick Brosset [:pbro] from comment #23)
> Which, I realized now, is what I had said in comment 6.

And which I fixed in attachment 8731093 [details] [diff] [review] and ran through try (https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1f928ba8952) and verified it passed, only to somehow lose that change in attachment 8731552 [details] [diff] [review]. I'm sorry.
This time for sure. Please land on aurora.
Attachment #8731552 - Attachment is obsolete: true
Is there any flag I need to set to make sure this gets picked up for landing on aurora?
Flags: needinfo?(cbook)
(In reply to Brian Birtles (:birtles) from comment #26)
> Is there any flag I need to set to make sure this gets picked up for landing
> on aurora?

yeah normally we would have picked up when the flag is approval-mozilla-aurora+
but since this is lost with the new patch the needinfo is great :) 

landed this as https://hg.mozilla.org/releases/mozilla-aurora/rev/1587aca122f0
Flags: needinfo?(cbook)
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
I've already updated MDN to say that this API will ship in Firefox 48.
Is there anything else we need to cover? This was a new API originally slated for 47, correct? If so, then :birtles' update should be enough, I think.
(In reply to Eric Shepherd [:sheppy] from comment #29)
> Is there anything else we need to cover? This was a new API originally
> slated for 47, correct? If so, then :birtles' update should be enough, I
> think.

That's correct. I'm not aware of any other docs that would need updating.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: