Closed Bug 876282 Opened 7 years ago Closed 7 years ago

Add unprefixed cancelAnimationFrame

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox22 --- unaffected
firefox23 + fixed
firefox24 + fixed

People

(Reporter: evilpie, Assigned: Ms2ger)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, regression, Whiteboard: [parity-IE][parity-webkit])

Attachments

(2 files, 1 obsolete file)

requestAnimationFrame was unprefixed in Bug 704063.
Huh, only unprefixed requestAnimationFrame has been added without adding cancalAnimationFrame!?
You're in for a lot of broken sites if you don't backport this change to Firefox 23...
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?)
Flags: needinfo?(bzbarsky)
tracking until we get an answer to comment 3 - will untrack if this doesn't have to ship at the same time as requestAnimationFrame.
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.
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.
Blocks: 704063
No longer depends on: 704063
Flags: needinfo?(bzbarsky)
Keywords: regression
Attached patch Patch v1 (obsolete) — Splinter Review
Will need a uuid rev for trunk and probably a new interface for backports
Assignee: nobody → Ms2ger
Status: NEW → ASSIGNED
Attachment #754791 - Flags: review?(bzbarsky)
Attached patch Patch v1.1Splinter Review
Attachment #754791 - Attachment is obsolete: true
Attachment #754791 - Flags: review?(bzbarsky)
Attachment #754795 - Flags: review?(bzbarsky)
Comment on attachment 754795 [details] [diff] [review]
Patch v1.1

r=me with that uuid rev.
Attachment #754795 - Flags: review?(bzbarsky) → review+
Attached patch Patch for auroraSplinter Review
Attachment #754977 - Flags: review?(bzbarsky)
Comment on attachment 754977 [details] [diff] [review]
Patch for aurora

r=me
Attachment #754977 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/e1c63a4904d1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
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
Attachment #754977 - Flags: approval-mozilla-aurora?
(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.
(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.
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.
> Do we really need to worry about changing UUIDs on Aurora?

We may not have to, but it's easier to play it safe.
(In reply to Michał Gołębiowski from comment #17)
> (the first `for` loop would exit immediately)

Ah, I overlooked |&& !window.requestAnimationFrame| part, sorry.
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.
Attachment #754977 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to lsblakk@mozilla.com [: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?
You need to log in before you can comment on or make changes to this bug.