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

Detection for buggy raf implementation #2

Closed
wants to merge 5 commits into from
Closed

Detection for buggy raf implementation #2

wants to merge 5 commits into from

Conversation

BendingBender
Copy link

Currently, the script performs feature detection by executing raf once and if it returns assuming that implementation is not buggy.

However, there are currently 2 issues with raf on iOS 6 + 7 (tested with both):

  • raf doesn't execute at all, which this script can currently handle
  • raf executes only after an ongoing animation/transition on the page has finished, which means that if the animation is infinite or takes some time or the transition takes some time, raf will be delayed as long as the transition/animation takes place or won't be executed at all if animation is an infinite one.

We have currently exactly this behaviour, we try to transition some elements while we are dragging another one. The drag is done via raf and element position change handler is not called as long as transition takes place. Tested with iPad2/iOS6.1.3, iPhone5/iOS 6.1.4 and iPhone4/iOS7.0.

This commit removes the feature detection and introduces a userAgent string check to match iOS6+7 powered browsers.

…have

been called in window scope instead of created AnimationFrame object scope
Conflicts:
	AnimationFrame.js
	AnimationFrame.min.js
Conflicts:
	AnimationFrame.min.js
with static userAgent sniffing due to further bugs in animationFrame
implementation on iOS 6+7
@kof
Copy link
Owner

kof commented Sep 24, 2013

Hmm I have done this to avoid agent sniffing ... lets see how we can solve this using feature test. We can assume that there are other devices out of there we don't even know which have the same problem.

@kof
Copy link
Owner

kof commented Sep 24, 2013

do you have a ready example for this issue?

@BendingBender
Copy link
Author

Well, I hope this plunk will demonstrate the problem. I couldn't test it right now, I don't have an iOS device at hand right now. I'll test it tomorrow.

The point in the plunk is that the draggabilly lib is using RAF (it will use it if it's available and shim it if not, it has no workarounds for iOS bugs). When the draggable element hovers over one of the red boxes, it will cause a transition (2s should be enough to demonstrate the problem) which will prevent the draggable element's position to be updated on iOS 6+7 until transition is over.

@kof
Copy link
Owner

kof commented Sep 24, 2013

Tested on iphone 5 v.6.1.4 safari - works well, chrome - dragging works very bad without to hover over the red boxes.

@BendingBender
Copy link
Author

The script doesn't limit the update frequency, I didn't optimize it at all. I'll compare it tomorrow with the implementation that we are using in our project to figure out what's missing to trigger the bug.

@kof
Copy link
Owner

kof commented Sep 24, 2013

Ok thanks!

@kof
Copy link
Owner

kof commented Sep 27, 2013

any news on this?

@BendingBender
Copy link
Author

Well, I finally managed to trigger the bug on iOS6 (iPhone5) with my plunk. Triggering it on iOS7 (tested on iPhone4) was a breeze but Safari on iOS6 is much more robust. Nevertheless, here is the plunk I prepared for you.

You will have to drag the draggable pretty fast to see the glitches. It is not that RAF is called only after the animation is finished as I described before but from time to time its execution is delayed while other elements are still animating. To be honest, it is far easier to reproduce in our project, but it's not released yet and I can't refer to it to demostrate the bug. In our project, the draggable's position really isn't updated as long as you drag it and other elements are animated, but as soon as you stop dragging, its position is updated immediately.

@kof
Copy link
Owner

kof commented Sep 27, 2013

Thanks, I can reproduce it too. Lets see what can be done.

@BendingBender
Copy link
Author

Any ideas for feature detection on this?

@kof
Copy link
Owner

kof commented Oct 4, 2013

couldn't find time ... will try asap

@kof
Copy link
Owner

kof commented Oct 10, 2013

I think we can't fix this well. The situation you have is where you mix 2 different ways to move objects which seem to block each other. Not using native raf on this devices per default is not an option, since the situation you have is a special case. I think you want to move your boxes using transformations.

@kof
Copy link
Owner

kof commented Oct 10, 2013

It might be just worth a documentation notice with your example to prove it.

@BendingBender
Copy link
Author

It's really hard to say and even harder to detect. Maybe, you should provide an explicit config option for people who want to detect buggy platforms by sniffing UA strings by thremselves, they could then hard enable the RAF shim instead of the native implementation.

@kof
Copy link
Owner

kof commented Oct 14, 2013

Yeah, thought about this too. I will add this option.

@Hypher
Copy link

Hypher commented Oct 20, 2013

Another proof against part of the community decision to rely ONLY on feature detection, wiping out platform detection (as jQuery removing its $.browser). We need both, and more important, we need to let the library user choose.

@kof
Copy link
Owner

kof commented Oct 22, 2013

This doesn't proves anything. Its a very specific case, engine detection is required for a very specific conflict. jQuerys move is correct, people are relying on sniffing where feature detection is possible. Browser sniffing is a very bad thing and should be used ONLY if NO other way is there. Also they have created an official plugin with the old engine sniffing code. They just try to make clear the right way to code.

@Hypher
Copy link

Hypher commented Oct 22, 2013

  • jQuery does not provide any official plugin, there is only jQuery migrate which is intended to help migrating sources and warns about the use of $.browser. Hopefully some people released non official browser detection plugins;
  • "where feature detection is possible" => the FACT is there are many edge cases where we need browser detection (especially iP(hone|ad)) and cannot rely on feature detection;
  • I complain about the removal of browser detection in jQ, I never said it's a good coding standard.

Nevertheless, my last sentence was perfectly clear: "We need both, and more important, we need to let the library user choose."

As library designers, we need to be open minded and putting barriers and force users to think some particular way is dangerous. CSS is a perfect example of awkward design with things like "table are only for tabular content" and stuff. 10 years after it's still horrible to deal with certain layouts in CSS3! We could talk about multiple inheritance and stuff. But I think you got it. Please never consider something as bad as forbidding people to use it, it's a despotic choice.
Finally I'm glad to see yo will accede to @BendingBender request, providing a way to use UA's detection :)

@kof
Copy link
Owner

kof commented Oct 22, 2013

it was there, seems like it has been removed https://github.com/jquery/jquery-browser

Nobody is forbidding this, its just making it clear unrecommended and now also officially unsupported.

@kof kof closed this in 397738b Oct 22, 2013
@kof
Copy link
Owner

kof commented Oct 22, 2013

please check out the latest version

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