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)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox47 | --- | unaffected |
firefox48 | --- | fixed |
People
(Reporter: mossop, Assigned: birtles)
References
Details
Attachments
(2 files)
(deleted),
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
Right. That error is only temporary until we fix bug 1245748 (which blocks shipping Element.animate).
Depends on: 1245748
Assignee | ||
Comment 3•9 years ago
|
||
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)
Assignee | ||
Comment 4•9 years ago
|
||
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.
Assignee | ||
Comment 5•9 years ago
|
||
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)
Reporter | ||
Comment 6•9 years ago
|
||
I think a free account gives you some limited access to music but I can test it tomorrow.
Reporter | ||
Comment 7•9 years ago
|
||
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)
Assignee | ||
Comment 8•9 years ago
|
||
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.
Assignee | ||
Comment 9•9 years ago
|
||
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
Assignee | ||
Comment 10•9 years ago
|
||
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.
Assignee | ||
Comment 11•9 years ago
|
||
Hi Cameron, sorry to bother you with all these reviews but these two should be quite easy.
Attachment #8741627 -
Flags: review?(cam)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Assignee | ||
Comment 12•9 years ago
|
||
MozReview-Commit-ID: CgwsJQKB1qj
Attachment #8741632 -
Flags: review?(cam)
Updated•9 years ago
|
Attachment #8741627 -
Flags: review?(cam) → review+
Comment 13•9 years ago
|
||
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+
Comment 14•9 years ago
|
||
Comment 15•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/87509ba26631
https://hg.mozilla.org/mozilla-central/rev/d80d1bee4c35
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Comment 16•9 years ago
|
||
Hi Dave, I think this should be fixed now but would you mind checking? Thanks.
Flags: needinfo?(dtownsend)
Comment 18•9 years ago
|
||
Dup bug 1252599 shows that 47 is affected. Does this fix need to be uplifted to Beta?
status-firefox47:
--- → ?
Flags: needinfo?(bbirtles)
Assignee | ||
Comment 19•9 years ago
|
||
(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)
Updated•9 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•