Skip to content

New Flag: jQuery.support.raf - Allow disabling requestAnimationFrame for people having issues with #9381 #436

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

gnarf
Copy link
Member

@gnarf gnarf commented Jul 16, 2011

Effects: Adding jQuery.support.raf boolean to allow disabling the use of requestAnimationFrame to allow people to work around #9381

http://jqbug.com/9381

Just submitting for some discussion. The general idea here is you would set jQuery.support.raf = false in a codebase where you haven't gotten around to properly implementing design patterns necessary to avoid queuing new animations while they aren't running.

I feel that no matter how much we try to educate the best practices mentioned in the ticket, there will still be situations where the end user can't manage to set up the animation queueing properly.

As coded, this flag can not be changed while animations are running, and thus, should only be set once (and only to false) before any animations are queued.

It is likely possible to replace the current requestAnimationFrame & setInterval code with a "recursive setTimeout pattern" that more closely approximates the rAF implementations. If we were to do that, you could actually probably shorten the whole thing quite effectively, and allow toggling this flag while running.

…equestAnimationFrame to allow people to work around #9381
@dmethvin
Copy link
Member

dmethvin commented Aug 5, 2011

So I think it was @jaubourg who suggested we create jQuery.support.requestAnimationFrame and allow the user to clobber that with false if they wanted to return to the stone age. Kind of like we do with jQuery.support.cors. Would that be a better solution? It would be good to land something in 1.6.3.

@timmywil
Copy link
Member

timmywil commented Aug 5, 2011

Have we decided that the flag will happen? We should probably discuss this pull, louisremi's pull, and, I don't see this one in the queue anymore, but one that runs a separate timer to handle the queue. I'm not sure which I'm in favor of or if we should do some combination. There's also another option that we haven't discussed yet: remove requestAnimationFrame altogether.

@jaubourg
Copy link
Member

jaubourg commented Aug 5, 2011

It's quite obvious we need to seriously discuss the issue because whatever decision we take we'll have a huge impact on the future of animations in jQuery. We can add jQuery.support.raf (I prefer things short ;)) right now to deal with the immediate problem. It's somehow clean in the sense it won't give us a gigantic headache if and when we find a proper solution and actually refactor the animation system properly (we will still have to feature test raf anyway).

@addyosmani
Copy link
Member

jQuery.suport.raf/jQuery.support.requestAnimationFrame sounds like an acceptable stop-gap solution, but given that (as Timmy mentions) there are a number of available pulls which alternative solutions, perhaps the best idea would be to schedule this for discussion in one of the next -dev meetings. Let's make sure whatever is decided is documented well..it can get confusing enough as it is at the moment :)

@louisremi
Copy link
Contributor

Although I am definitely in favor of having jQuery.support.raf (was in my initial implementation of raf), this does not solve #9381. (again, because setInterval "slows down" in inactive tabs Firefox and Chrome).

@dmethvin
Copy link
Member

dmethvin commented Aug 5, 2011

Yeah we're dealing with several problems here. For 1.6.3 the ability to just plain disable rAF would be fine, but we may want to do something more ambitious for 1.7 -- if we can just figure out what that is!

@louisremi
Copy link
Contributor

Here is the link to my alternative fix: #446

@jaubourg
Copy link
Member

jaubourg commented Aug 5, 2011

I'm still convinced we need to talk with browser vendors about this setTimeout/setDelay slow down. It just doesn't make sense and not just in the context of animations. I'd really like if we could see about fighting this atrocity before attempting to circumvent it, because if we start and circumvent it (in a very blind-man-trying-to-see kind of way btw), then we're basically condonning it.

This slow down is part of no standard (I'll be so bold as to say it actually contradicts existing standards) and will cause more troubles in the long run than what it is trying to achieve. We're not in the IE6 days anymore: we can and will voice our opinion if and when browsers go haywire.

@addyosmani
Copy link
Member

Given that we've said 1.7 is probably not going to get wrapped up until some time in the fall, I think we definitely have enough scope to engage in some level of discussions with browser vendors about the issues here - it certainly wouldn't hurt. I guess the question is, is the general feeling that this should be done or that we should internally decide on a workaround/solution based on the numerous pulls?. I share some of Julian's concerns with the slow-down causing more headaches in the long-term.

@dmethvin
Copy link
Member

dmethvin commented Aug 5, 2011

This slow down is part of no standard ...

@jaubourg, read it and weep: http://www.w3.org/TR/html5/timers.html

I think we need to accept this will happen, work around it as well as we can, and get the word out about what it means.

@timmywil
Copy link
Member

timmywil commented Aug 5, 2011

I agree with jaubourg. The argument is reasonable. That will probably mean waiting on louisremi's pull so we can see if anything will come out of a discussion with browser vendors. As far as the queue timer, it sounds like that will have a better chance at being efficient in the effects rewrite jaubourg and gnarf and I want to push for 1.8. So, maybe for 1.6.3 AND 1.7, we can add jQuery.support.raf and tackle this problem more thoroughly when 1.8 rolls around.

@timmywil
Copy link
Member

timmywil commented Aug 5, 2011

Just now seeing dave's comment. Well, that may change everything I just said.

@louisremi
Copy link
Contributor

The "thread" bug about sites broken by this change is here: https://bugzilla.mozilla.org/show_bug.cgi?id=652472
Feel free to contribute.

Currently the policy is to contact developers and ask them to fix it. If dozens of websites are reported to break, the change might well be reverted. But this behavior has been in Firefox and Chrome "dev builds" for quite some time now, and the Web still runs.

@dmethvin
Copy link
Member

dmethvin commented Aug 5, 2011

I doubt the browser vendors realized in advance what a mess this makes of existing code. It has always been true that setTimeout(fn, 30) doesn't guarantee you're called exactly 30ms later, but 1000ms is a lot later. :)

Still, it's a very good idea to do this by default for the reasons mentioned by the W3C page; it saves lots of battery life. I would prefer that browser makers hold their ground and we work together on ways to unbreak the old code.

@gnarf
Copy link
Member Author

gnarf commented Aug 5, 2011

@timmywil et all - The "other" solution that @jaubourg recommended is still just a piss poor untested draft branch I threw together here https://github.com/gnarf37/jquery/compare/ticket_9381_2

If we like this approach I can turn it into a better method and put it into a pull request.

@gnarf
Copy link
Member Author

gnarf commented Aug 5, 2011

Regarding jQuery.support vs jQuery.fx for the location of this flag, let me quickly copy this discussion from -dev:

13:36 gnarf: DaveMethvin: you think I should alter that pull to put the flag on jQuery.support instead of jQuery.fx ?
13:37 gnarf: i think putting it on support might be a little more code
13:37 gnarf: just cuz all that raf stuff is already in fx.js
13:37 DaveMethvin: we'll just move the detect to support.js tho, right?
13:38 DaveMethvin: it seems natural to put the detect in support
13:39 DaveMethvin: trying to think of downsides...
13:42 gnarf: the detect is the same method to get the actual function tho
13:43 DaveMethvin: oic gnarf, you're saying if the detect is a boolean we need a new variable?
13:43 gnarf: so unless you want to duplicate the effort, you're going to end up storing a function on support instead of true/false
13:43 DaveMethvin: yeah
13:44 DaveMethvin: i guess by convention we have been doing booleans for support
13:44 DaveMethvin: can't think of a benefit to assigning a function
13:44 gnarf: if we knew effects was catted after support
13:44 gnarf: effects could add to support
13:44 DaveMethvin: unless someone wanted to provide a custom rAF function ... er is that a feature? :P
13:46 DaveMethvin: I'd keep the .support.raf as a boolean and then set up a var for it, but yeah we need to be sure the .support stuff runs earlier
13:47 DaveMethvin: do they get included in the order of BASE_FILES in the Makefile? If so we're good
13:47 gnarf: my reasoning for dumping it on .fx. now becomes more clear
13:47 gnarf: DaveMethvin: they should
13:47 DaveMethvin: yeah looks like it in the ${JQ} target
13:48 gnarf: i figured the flag is a part of the effects module, it makes sense for it to be on the effects module
13:48 gnarf: jQuery.fx.off ... jQuery.fx.raf ... jQuery.fx.interval are all kinda user configurable
13:49 DaveMethvin: i was gonna say it's easier to document on .support since we already have an api page for it :)
13:49 DaveMethvin: but otoh we do document those
13:50 DaveMethvin: but then it will need its own private new page like http://api.jquery.com/jQuery.fx.off/
13:50 DaveMethvin: rather than being a paragraph in an existing page
13:50 DaveMethvin: pro and con i guess, it's more visible as its own page
13:51 DaveMethvin: but easier to disavow if it's a paragraph in .support
13:52 DaveMethvin: ya know, i'm good with jQuery.fx.raf
13:53 DaveMethvin: really the only thing we will support on it is setting to null i guess to turn off rAF
13:53 DaveMethvin: and document it needs to be done before any animations i guess?
13:53 gnarf: yup and yup

The TLDR: It makes sense in either place, and if you guys think .support is smarter, I can easily update this pull for that. I was leaning towards .fx since its whats already in scope when we are doing the check for rAF in the first place.

@dmethvin
Copy link
Member

dmethvin commented Sep 2, 2011

I'm going to close this PR since it's likely we'll need to rethink and rework the animation timer/rAF issue when it finally lands.

@dmethvin dmethvin closed this Sep 2, 2011
@lock lock bot locked as resolved and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants