Skip to content
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

Revert to previous RAF shim, support window.performance.now() #36

Merged
merged 2 commits into from
Dec 5, 2012

Conversation

danro
Copy link
Contributor

@danro danro commented Dec 3, 2012

Tests passing in Chrome 25, but I haven't tried IE10.

This should hopefully add support for the new timers with minimal impact on current performance.
And would close #35 as well.

(also added a .jshintrc for Sublime love)

@danro
Copy link
Contributor Author

danro commented Dec 4, 2012

Tests passing in IE10.

@danro
Copy link
Contributor Author

danro commented Dec 4, 2012

Wait, scratch that- all tests were passing and then I reloaded it a few times. Now the Colors, long hex + short hex are failing. Good old IE.

@danro
Copy link
Contributor Author

danro commented Dec 4, 2012

OK phew. Somehow IE10 had switched to "Compatibility View" which caused the breaking.

IE10 tests passing. 🔨

@ded
Copy link
Owner

ded commented Dec 5, 2012

👉 👌

@ded
Copy link
Owner

ded commented Dec 5, 2012

this looks solid

ded added a commit that referenced this pull request Dec 5, 2012
Revert to previous RAF shim, support window.performance.now()
@ded ded merged commit df1fc9e into ded:master Dec 5, 2012
@rvagg
Copy link
Collaborator

rvagg commented Dec 9, 2012

Thanks muchly for this @danro, saved me going insane with the latest Chrome.

But, FF doesn't appear to use the performance timings for rAF, a +new Date would be a better choice, an even better choice would be to use window.mozAnimationStartTime.

I haven't bothered looking at IE, Opera or Safari at this stage but we need to make sure that if a browser has window.performance that it actually uses that clock as the reference for rAF callbacks; Gecko has it but doesn't use it for rAF. (I'm running Aurora fwiw so window.performance may not be in the release channel yet.)

@rvagg
Copy link
Collaborator

rvagg commented Dec 9, 2012

This works for Chrome & Aurora: https://github.com/rvagg/morpheus/commit/1c51a1e51ae9bf87e0be83a6ba56f9f71e85b085
Thoughts?

@rvagg
Copy link
Collaborator

rvagg commented Dec 9, 2012

I'm having a git-derp day today, see this commit instead: https://github.com/rvagg/morpheus/commit/d46f43904eb955888a1a4c1e1a79de65238bf661

@danro
Copy link
Contributor Author

danro commented Dec 9, 2012

Ahh thank you @rvagg, I was having my own special firefox moment today w/ morpheus. This explains it.

The tests were passing, so perhaps a new test is in order?

@rvagg
Copy link
Collaborator

rvagg commented Dec 9, 2012

ok, next derpage.. IE9 has window.performance but not window.performance.now so it completely barfs. Next iteration that gets it working in Chrome dev, Aurora and IE9: https://github.com/rvagg/morpheus/compare/animationstart

@danro
Copy link
Contributor Author

danro commented Dec 9, 2012

Another thing we could do to make it even safer moving forward would be to completely ignore the parameter being passed to the RAF callback, and just use your startTime() (maybe that would make it time()) on each run to get the current time. I think any perceived perf losses from the function wrapper would be regained from having more accurate timers in Chrome & FF.

@danro
Copy link
Contributor Author

danro commented Dec 9, 2012

@rvagg I'm gonna pull from you and write the code for that if u like?

@rvagg
Copy link
Collaborator

rvagg commented Dec 9, 2012

Could do, except for Gecko, mozAnimationStartTime is an absolute reference for the current page refresh so it doesn't change, so it works a little different to the others. I think that +new Date will still work for Gecko (for now) so we could just ignore their special property.
My OS X VM isn't working at the moment so I can't check this on latest Safari, could someone do some Safari tests on this stuff to see what it's doing? My fear is that Chrome is picking up the use of the window.performance.now() value as its reference while the others are just using an epoch reference value, what happens when others start to implement window.performance the same way that Firefox has? We're going to end up in feature-detection hell.

@rvagg
Copy link
Collaborator

rvagg commented Dec 9, 2012

And yes, do what you want with my branch, you're welcome to turn it into a PR if you think you can make it sensible across all browsers. I just need something that works right now.

@rvagg
Copy link
Collaborator

rvagg commented Dec 9, 2012

My code is good in IE7 to IE9, Chrome 23 & 24, Opera, Firefox Aurora & Release, Safari 5.1 (Win) but I can't test anything on OS X and I also don't have IE10 handy right now.

@danro
Copy link
Contributor Author

danro commented Dec 9, 2012

Well, in FF 15 it looks like they're supporting window.performance.now() so maybe we just roll with that and ignore their non-standard mozAnimationStartTime ?

Just tested in Safari 6.0.2 and it doesn't have window.performance yet.

@rvagg
Copy link
Collaborator

rvagg commented Dec 9, 2012

can't use window.performance.now() on FF as it's not used as the base for rAF, only Chrome does that; so far anyway.

@danro
Copy link
Contributor Author

danro commented Dec 9, 2012

I'll pull and sketch out what i'm thinking. Stand by..

@rvagg
Copy link
Collaborator

rvagg commented Dec 9, 2012

in FF, open a Web Console and run this: console.log(window.performance.now(), window.mozAnimationStartTime, +new Date)

/me pulls hair

But this is the combined fault of the spec (for not mandating a window.animationStartTime) and Chrome for going it alone on the window.performance.now() as a base.

@rvagg
Copy link
Collaborator

rvagg commented Dec 9, 2012

btw, a few alternatives I considered:

  1. leave start=null then if (!start) start = +new Date in the loop
  2. add an additional check in the loop that resets start = +new Date if start < now or delta < 0, or perhaps delta < -10 just to be sure.
  3. ignore the parameter passed to the rAF callback completely.

The problem with (1) is that we lose the first iteration cause we don't have a reference time, I'm not sure how much that matters though, at least we get to ignore these cross-browser issues though. The problem with (2) is just more cycles for the check, perhaps not a big deal though. (3) would presumably mean we lose out on the slight performance gains offered by not having to do a Date each iteration when we have rAF.

Each solution would mostly future-proof any implementation changes; we can guarantee that at some point in the (near?) future the current code is going to break.

Thoughts @ded?

@danro
Copy link
Contributor Author

danro commented Dec 9, 2012

Here's what I was thinking for ignoring the RAF callback param.

https://github.com/rvagg/morpheus/pull/1/files

@danro
Copy link
Contributor Author

danro commented Dec 9, 2012

Another plus to ignoring the rAF callback param, we could additionally shim in support for webkitNow and such other prefixed nonsense (meaning high res timers for current builds).

@danro
Copy link
Contributor Author

danro commented Dec 9, 2012

Edited.

@danro
Copy link
Contributor Author

danro commented Dec 9, 2012

Pushed another commit to https://github.com/rvagg/morpheus/pull/1.

I moved the now() call to only occur once per render loop. (meaning only once per however many morpheus tweens are happening on that frame).

This feels like the safest approach to me, and perhaps we can revisit later w/ stress tests to see if it's negatively impacting the old set of rAF browsers. Morpheus could probably use some animation demos anyway.

@rvagg
Copy link
Collaborator

rvagg commented Dec 9, 2012

I've taken a totally different tack with an uber-feature-detect, heavily documented to save confusion, see code here: https://github.com/rvagg/morpheus/compare/timer, let me know what you think. It's a multi-stage feature-detect that should be robust enough to last for a while.

I've also included a test case that should probably make it in regardless, it'll ensure that an animation is actually happening--or at least it's not just jumping straight to the final state or doing something else weird.

@rvagg
Copy link
Collaborator

rvagg commented Dec 9, 2012

And I've given you commit access to my fork @danro so feel free to mess around in there. I see you're doing some more interesting work with the possible performance.*now() variations that I haven't touched in my latest.

@danro
Copy link
Contributor Author

danro commented Dec 9, 2012

Oh, interesting feature detect work there!

I'm currently on deadline and won't be able to come back to this for a few days, so I may stick with my safe + small edit for now, but I like the feature detect idea on page load.

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

Successfully merging this pull request may close these issues.

3 participants