Closed Bug 1247004 Opened 9 years ago Closed 9 years ago

Google play music playlist popup doesn't close (polymer animations are broken)

Categories

(Core :: DOM: Animation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- unaffected
firefox48 --- fixed

People

(Reporter: mossop, Assigned: birtles)

References

Details

Attachments

(2 files)

On current nightly if you are playing music in google play you can't close the playlist. Clicking the playlist button displays this error in the console: NotSupportedError: Animation with a target not bound to a document is not yet supported. I suspect this is related to the new Element.animate support.
Right. That error is only temporary until we fix bug 1245748 (which blocks shipping Element.animate).
Depends on: 1245748
Expanding the title of this bug to cover other uses of polymer animation.
Summary: Google play music playlist popup doesn't close → Google play music playlist popup doesn't close (polymer animations are broken)
In bug 1253507 I'm suggesting we disable Element.animate in Firefox 47 since we probably won't be able to fix bug 1245748 in 47.
Bug 1245748 has just entered m-c and should be in tomorrow's Nightly build. Is there some way I can test this or do you need to sign up for an account?
Flags: needinfo?(dtownsend)
I think a free account gives you some limited access to music but I can test it tomorrow.
This still seems to be failing but with a different error in the console now: TypeError: Invalid easing 'function (a){return a}'.
Flags: needinfo?(dtownsend)
Yeah, that's a bug in the polyfill that assumed you could set and retain invalid timing functions, because the Chrome implementation allowed it. I spoke to the Chrome guys and they agreed to drop that from the polyfill but it looks like it's more widespread than we realized. We might have to change the spec and our implementation in order not to break those sites. See bug 1260878.
Marking this as blocking shipping Element.animate since I think at least we need to not throw for easing = 'function(a) { return a; }' since the polyfill currently sets that for any animation using groups. That's being fixed in the polyfill but Chrome currently special cases that one function until they collect enough data to be sure they can throw for that.
Blocks: 1245000
Some links on the status of Chrome. At the time of writing (I couldn't find the syntax for permalinks into chromium source), the code in chromium for *not* throwing for "function (a){return a}" is here: https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Source/core/animation/AnimationInputHelpers.cpp#218 Issue is here: https://bugs.chromium.org/p/chromium/issues/detail?id=601672 So I suspect we want to line up and *not* throw for the exact string "function (a){return a}". That seems kind of messy to me---I'd be open to simply *always* storing the invalid easing and just reporting a console warning. I've mentioned that to Google however and they didn't seem interested in doing that. Google will be collecting data and if enough people update their version of the polyfill and usage of "function (a){return a}" drops then they will drop that hack. Presumably we can do the same at that point so maybe it's ok to add this exception for now.
Hi Cameron, sorry to bother you with all these reviews but these two should be quite easy.
Attachment #8741627 - Flags: review?(cam)
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
MozReview-Commit-ID: CgwsJQKB1qj
Attachment #8741632 - Flags: review?(cam)
Attachment #8741627 - Flags: review?(cam) → review+
Comment on attachment 8741632 [details] [diff] [review] part 2 - Don't throw if easing is the string "function (a){return a}" Review of attachment 8741632 [details] [diff] [review]: ----------------------------------------------------------------- Wow, awful. :( r=me
Attachment #8741632 - Flags: review?(cam) → review+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Hi Dave, I think this should be fixed now but would you mind checking? Thanks.
Flags: needinfo?(dtownsend)
Yep, this is working now, thanks.
Flags: needinfo?(dtownsend)
Dup bug 1252599 shows that 47 is affected. Does this fix need to be uplifted to Beta?
Flags: needinfo?(bbirtles)
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #18) > Dup bug 1252599 shows that 47 is affected. Does this fix need to be uplifted > to Beta? No, this API isn't turned on for release channels until 48 (and we actually disabled it while 47 was in Aurora anyway).
Flags: needinfo?(bbirtles)
Blocks: 1295866
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: