akiran / react-slick

React carousel component
http://react-slick.neostack.com/
MIT License
11.77k stars 2.1k forks source link

iOS: Can only swipe once, doesn't transition (state.animating locks to true) #25

Closed grrowl closed 9 years ago

grrowl commented 9 years ago

The problem is from line 203 of lib/mixins/helpers.js

    this.setState({
      animating: true,
      currentSlide: currentSlide,
      currentLeft: currentLeft,
      trackStyle: this.getAnimateCSS(targetLeft)
    }, function () {
      ReactTransitionEvents.addEndEventListener(this.refs.track.getDOMNode(), function() {
        this.setState({
          animating: false,
          trackStyle: this.getCSS(currentLeft),
          swipeLeft: null
        });
      }.bind(this));
    });

For whatever reason iOS doesn't get the css transition class (either not in time or doesn't support?), therefore the ReactTransitionEvents.addEndEventListener never fires and animating is stuck to true, which blocks subsequent actions.

akiran commented 9 years ago

Thanks for posting the issue.

This week I am little busy. I will fix all the pending issues of react-slick next week.

grrowl commented 9 years ago

No worries, I'm applying little workarounds to my local build (in this case, commenting out the line animating: true :sweat_smile:).

Also, here's a repro video: https://www.dropbox.com/s/uzesxuacxur7qnc/react-slick-ios.mov?dl=0

grrowl commented 9 years ago

Confirmed the transition class doesn't get applied in time. (adding a permanent transition: transform; works as expected.) (for reference, my changes are going here: https://github.com/grrowl/react-slick/commits/bugfixes-and-bowerless)

also, this issue causes afterChange() not to fire on iOS at all. sorry, I was wrong on this one.

akiran commented 9 years ago

Thanks, I will review this.

kriswallsmith commented 9 years ago

I'm seeing this issue also and may not be able to use this module because of it… shame because it's great work otherwise!

grrowl commented 9 years ago

If you're okay with transitions not working on iPhone (but otherwise functional), you can add my up to date fork to your package.json:

  "react-slick": "grrowl/react-slick#bugfixes-and-bowerless",
akiran commented 9 years ago

If animating state is stuck to true, then for some reason callback attached to transition event is not called.

grrowl commented 9 years ago

That's the fix, in the fork we skip setting state.animating to true, so it doesn't matter is the callback is fired or not. I haven't noticed any drawbacks (apart from the effect being kinda kludgy on iOS)

Really, the proper fix would set the css transition property, wait 16ms, then set the css for position/translate. That way, it'll transition as expected, then fire the transitionEnd event, and everything would be right in the universe.

akiran commented 9 years ago

I will spend some time today on this issue and fix it properly.

Really, the proper fix would set the css transition property, wait 16ms, then set the css for position/translate.

Is this specific to iOS

yuzeh commented 9 years ago

Yep. From a user's perspective, the library works great on Android Chrome, but definitely does not work on iOS.

FarmerKing commented 9 years ago

The issue also happens on WindowsPhone and Safari on Mac. (Chrome is fine)

akiran commented 9 years ago

This looks like a hard to debug bug. I couldn't understand why transition event in not fired on iOS.

For now, I added setTimeout to disable animation after specified time https://github.com/akiran/react-slick/blob/master/lib/mixins/helpers.js#L222 This will make sure that slider is not stuck after first swipe.

But it still misses animation effect on iOS. I will try and fix this as soon as possible.

grrowl commented 9 years ago

The hint lay in this stackoverflow post: the main takeaway is "Safari has no idea and follows no spec".

Turns out, the root cause was that it applied -webkit-transform but the transition value pointed to unprefixed transform. Additionally, it won't accept transition properties with comma-separated values (such as saying "transition -webkit-transform AND transform"), so the solution is supplying a webkit-specific transition for the webkit-specific transform.

Tested in Chrome stable, iOS 8.1 Safari and Desktop Safari 8.0.3

akiran commented 9 years ago

@kriswallsmith, @yuzeh, @FarmerKing This issue on iOS is fixed by @grrowl in #35 pull request.
I tested it on iphone and its working well. You can also test it.