876282 - Add unprefixed cancelAnimationFrame

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Tom S [:evilpie]]

requestAnimationFrame was unprefixed in Bug 704063.

Comment 1

[Masatoshi Kimura [:emk]]

Huh, only unprefixed requestAnimationFrame has been added without adding cancalAnimationFrame!?

Comment 2

[Michał Gołębiowski-Owczarek [:mgol]]

You're in for a lot of broken sites if you don't backport this change to Firefox 23...

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Boris, do you agree this needs to go in the same release as unprefixing requestAnimationFrame?  (And do you want to do it, or should we find someone else?)

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

tracking until we get an answer to comment 3 - will untrack if this doesn't have to ship at the same time as requestAnimationFrame.

Comment 5

[Michał Gołębiowski-Owczarek [:mgol]]

Just to add one reason: the following polyfill from Paul Irish's blog breaks when only requestAnimationFrame is unprefixed:
http://www.paulirish.com/2011/requestanimationframe-for-smart-animating/
You can find this polyfill in many places all over the internet, like:
http://my.opera.com/emoller/blog/2011/12/20/requestanimationframe-for-smart-er-animating
https://gist.github.com/paulirish/1579671

This is a really bad idea to unprefix only one of them.

Comment 6

[Boris Zbarsky [:bzbarsky]]

This was just an oversight, yeah.  We need to add an unprefixed cancelAnimationFrame.  Luckily, this should be trivial to implement: both the prefixed and unprefixed cancel should have the same behavior.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Will need a uuid rev for trunk and probably a new interface for backports

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1.1

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 754795 [details] [diff] [review]
Patch v1.1

r=me with that uuid rev.

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e1c63a4904d1

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch for aurora

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to :Ms2ger from comment #11)
> Created attachment 754977 [details] [diff] [review]
> Patch for aurora

https://tbpl.mozilla.org/?tree=Try&rev=5427e53156ff

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 754977 [details] [diff] [review]
Patch for aurora

r=me

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e1c63a4904d1

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 754977 [details] [diff] [review]
Patch for aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 704063
User impact if declined: sites may break
Testing completed (on m-c, etc.): a simpler patch that does change uuids is on m-c. This one passes try.
Risk to taking this patch (and alternatives if risky): pretty low risk. Alternative is unsupporting unprefixed requestAnimationFrame
String or IDL/UUID changes made by this patch: none

Comment 16

[Masatoshi Kimura [:emk]]

(In reply to Michał Gołębiowski from comment #5)
> Just to add one reason: the following polyfill from Paul Irish's blog breaks
> when only requestAnimationFrame is unprefixed:
> http://www.paulirish.com/2011/requestanimationframe-for-smart-animating/
> You can find this polyfill in many places all over the internet, like:
> http://my.opera.com/emoller/blog/2011/12/20/requestanimationframe-for-smart-
> er-animating
> https://gist.github.com/paulirish/1579671

Actually those polyfills will work because we didn't remove prefixed mozRequestAnimationFrame, mozCancelAnimationFrame and mozCancelRequestAnimationFrame.
But those polyfills will not use unprefixed functions if prefixed one is present. It is bad because the prefixed version doesn't support high-resolution timers. In short, the polyfills are broken anyway.

Comment 17

[Michał Gołębiowski-Owczarek [:mgol]]

(In reply to Masatoshi Kimura [:emk] from comment #16)
> Actually those polyfills will work because we didn't remove prefixed
> mozRequestAnimationFrame, mozCancelAnimationFrame and
> mozCancelRequestAnimationFrame.
I'm sorry but you're wrong. Specifically, this polyfill:
https://gist.github.com/paulirish/1579671
would cause Firefox to use unprefixed requestAnimationFrame (the first `for` loop would exit immediately) but cancelAnimationFrame would be defined as a function clearing a timeout that wasn't even set (since it assumes requestAnimationFrame is implemented using timeouts, too).

> But those polyfills will not use unprefixed functions if prefixed one is
> present. It is bad because the prefixed version doesn't support
> high-resolution timers. In short, the polyfills are broken anyway.
You're wrong about that, too. Read the code again.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Do we really need to worry about changing UUIDs on Aurora?  I thought we didn't promise stability until beta.  But maybe I'm misremembering.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Do we really need to worry about changing UUIDs on Aurora?

We may not have to, but it's easier to play it safe.

Comment 20

[Masatoshi Kimura [:emk]]

(In reply to Michał Gołębiowski from comment #17)
> (the first `for` loop would exit immediately)

Ah, I overlooked |&& !window.requestAnimationFrame| part, sorry.

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 754977 [details] [diff] [review]
Patch for aurora

Dbaron: we don't 'worry' about UUIDs on Aurora, but still good to know when there are changes in uplifted patches.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4e643d5826ac

Comment 23

[Kohei Yoshino [:kohei]]

https://developer.mozilla.org/en-US/docs/Web/API/window.cancelAnimationFrame
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/23
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_23

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to [e-mail] [:lsblakk] from comment #21)
> Comment on attachment 754977 [details] [diff] [review]
> Patch for aurora
> 
> Dbaron: we don't 'worry' about UUIDs on Aurora, but still good to know when
> there are changes in uplifted patches.

Should we land the other patch on aurora, then, since it doesn't have a (small) codesize, runtime memory use, and performance hit?