Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

ngAnimate + $$RAFProvider : crashes when browser doesn't support requestAnimationFrame or is prefixed with 'moz' #6535

Closed
atomrc opened this issue Mar 4, 2014 · 9 comments

Comments

@atomrc
Copy link
Contributor

atomrc commented Mar 4, 2014

The RAFProvider is adding a supported property to indicate whether the browser supports or not the requestAnimationFrame (see

raf.supported = !!requestAnimationFrame;
) but the ngAnimate doesn't check this property before using it (see https://github.com/angular/angular.js/blob/master/src/ngAnimate/animate.js#L261).

I would be happy to make a pull request on this but my question is :
is it best to add a fallback on a timeout directly in the RAFProvider or to check for the supportedproperty every time it is used ?

I think just adding the mozRequestAnimationFrame would be a good start.

@matsko
Copy link
Contributor

matsko commented Mar 4, 2014

Yes let's add mozRequestAnimationFrame. All browsers that support CSS animations/transitions also support RAF, therefore, there is no reason to fallback to timeout anymore.

I didn't think that Firefox still needed it. I guess the error showed up on an older version of FF.

@matsko
Copy link
Contributor

matsko commented Mar 4, 2014

Can you verify that adding mozRequestAnimationFrame and mozCancelAnimationFrame fixes the issue?

@atomrc
Copy link
Contributor Author

atomrc commented Mar 4, 2014

Yes indeed the error occurs in an old version of FF (20).
The fact is that if the browser doesn't support RAF (I am thinking of IE8 for example) the code will simply crash.
What is weird is that there is the property supported (false if the browser doesn't support rAF) but it is never checked wherever the rAF service is used.

atomrc pushed a commit to atomrc/angular.js that referenced this issue Mar 4, 2014
…refox

The recent $$RAFProvider which is a wrapper for the native
requestAnimationFrame method doesn't use the mozRequestAnimationFrame.
Old versions of FF (20 for example) crash if ngAnimate is included

No breaking changes and fix issue angular#6535
atomrc pushed a commit to atomrc/angular.js that referenced this issue Mar 4, 2014
…refox

The recent $$RAFProvider which is a wrapper for the native
requestAnimationFrame method doesn't use the mozRequestAnimationFrame.
Old versions of FF (20 for example) crash if ngAnimate is included

No breaking changes and fix issue angular#6535
atomrc pushed a commit to atomrc/angular.js that referenced this issue Mar 5, 2014
…refox

The recent $$RAFProvider which is a wrapper for the native
requestAnimationFrame method doesn't use the mozRequestAnimationFrame.
Old versions of FF (20 for example) crash if ngAnimate is included

No breaking changes and fix issue angular#6535
@tbosch tbosch self-assigned this Mar 6, 2014
@tbosch tbosch added this to the 1.3.x milestone Mar 6, 2014
@tbosch tbosch removed their assignment Mar 6, 2014
@mgol
Copy link
Member

mgol commented Mar 14, 2014

@matsko

All browsers that support CSS animations/transitions also support RAF

That's not true. Opera has had transitions since 10.5 (which is really old) and even the latest Presto-based version, 12.16 doesn't have rAF. Opera Presto is still popular in some countries like Russia.

@matsko
Copy link
Contributor

matsko commented Mar 14, 2014

Right. Yes. My mistake on this. I'm putting together a patch that uses the $timeout code with a low value: $timeout(callback, 1000 / 60); and updating the tests to support it.

@mgol
Copy link
Member

mgol commented Mar 14, 2014

@matsko If you're putting the timeout in, maybe the moz-prefixed version doesn't matter anymore? Hardly anyone uses old Fx versions so it's mostly so that they work, not necessarily in the most performant way.

Angular 2.0 is a rewrite anyway so the $timeout fallback will live in Angular 1.x forever.

@tbosch
Copy link
Contributor

tbosch commented Mar 14, 2014

I agree with @mzgol, e.g. Android 2.3 browsers don't support rAF but do support CSS transitions/animations.

@mgol
Copy link
Member

mgol commented Mar 14, 2014

@tbosch Not only 2.3. :) No Android Browser version supports requestAnimationFrame, only Chrome mobile does: http://caniuse.com/requestanimationframe

vojtajina pushed a commit that referenced this issue Mar 14, 2014
The recent $$RAFProvider which is a wrapper for the native
requestAnimationFrame method doesn't use the mozRequestAnimationFrame.
Old versions of FF (20 for example) crash if ngAnimate is included

No breaking changes and fix issue #6535

Closes #6535
Closes #6540
@dcherman
Copy link
Contributor

Why are we adding this? Per the browser support policy at https://docs.angularjs.org/misc/faq#what-browsers-does-angular-work-with-, only the latest versions of Firefox are supported; it's a hard sell to consider 20 as a new version.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants