Slider content should not be clickable while the user is dragging the slides on Desktop.

fdc-viktor-luft commented 7 years ago

I replicated the issue here: https://jsfiddle.net/20bumb4g/1988/

All pictures were wrapped into a link which "would" link to an external page. (google at this example)

If you start dragging any slide without changing the slide after releasing the click (no index update!) the event will be propagated down to the inner content. This makes dragging act differently and unwanted on desktop.

Please fix this issue. Btw.: your library is awesome.

minooo commented 7 years ago

@akiran Please solve this problem

adamshone commented 7 years ago

Maybe you could pass the event through to the beforeChange callback so that we can call stopPropagation on it?

minooo commented 7 years ago

@adamshone Good idea, but I found that still doesn't work as the beforeChange accepts only two parameters currentSlide and nextSlide.

adamshone commented 7 years ago

Sorry, by "you" I was referring to the project authors, I thought they could add the event as a third param to beforeChange.

Having had a very quick scan of the code I think unfortunately it's not that simple, it looks like it's using some animationEnd callbacks rather than listening to any mouse event that could be passed on to the user :/

adamshone commented 7 years ago

I tried using the beforeChange and afterChange event handlers to toggle a flag this.setState({ignoreClicks: true/false}), and pass state.ignoreClicks to the carousel items as a prop.

Unfortunately triggering a render from the beforeChange event seems to break the carousel, it cuts the animation off and seems to reset some internal state so that afterChange is never fired.

So... I have a workaround but it's very hacky - you can use it if you're desperate:

import Carousel from 'react-slick';

class MyCarousel extends PureComponent {
  midDrag = false;

  toggleMidDrag = () => {
    this.midDrag = !this.midDrag;
  }

  render() {
    return (
      <Carousel beforeChange={this.toggleMidDrag} afterChange={this.toggleMidDrag}>
        <Item key="0" parentCarousel={this} />
        <Item key="1" parentCarousel={this} />
        ...
      />
    );
}

class Item extends PureComponent {
  onClick = () => {
    if (this.props.parentCarousel.midDrag) {
      // ignore clicks
      return;
    }

    // ...your usual click handling code
  }

  render() { ... }
}

erasmo-marin commented 7 years ago

@akiran please, this is critical. I think the easier solution is to stop the event propagation on dragEnd in the event handler mixin.

vTrip commented 7 years ago

@akiran any update's on this issue ?

dmoli commented 7 years ago

@akiran this feature is critically necessary, please update it!

dmoli commented 7 years ago

@fdc-viktor-luft did you found a solution or another library? thanks!

fdc-viktor-luft commented 7 years ago

@skumblue I decided to avoid any bugs on cost of comfort. Therefore I disabled Drag-&-Drop on Desktop by setting "draggable" to "false". Hint: The property "draggable" influences only Desktop behaviour.

In case that this issues gets cleanly solved, I will consider an update.

trumko commented 6 years ago

You can try something like this:

class Carousel extends Component {
  constructor(props) {
    super(props);
    this.state = {isDragging: false};
  }

  shouldComponentUpdate(nextProps, nextState) {
    return this.state.isDragging === nextState.isDragging
  }

  onClickFunction = (e) => {
    if (this.state.isDragging) {
      e.preventDefault()
    }
  };

  render() {
    const settings = {
      ... // the rest of the settings
      beforeChange: () => this.setState({isDragging: true}),
      afterChange: () => this.setState({isDragging: false})
    };

    return (
      <div>
        <Slider {...settings}>
            <div><a href="#" onClick={this.onClickFunction}>one</a></div>
            <div><a href="#" onClick={this.onClickFunction}>two</a></div>
            <div><a href="#" onClick={this.onClickFunction}>three</a></div>
        </Slider>
      </div>
    )
  }
}

dwaltz commented 6 years ago

I have implemented a similar fix to the one above for this issue. Works perfectly.

chrisjcodes commented 6 years ago

I did something similar to @trumko but I set isDragging as a property on the class rather than in state so changing it never causes a re-render. shouldComponentUpdate was causing some weird glitching still

pimmey commented 6 years ago

It seems to be almost perfect with these hacks, although the slides need to slide completely off the screen, as in entire slide width, in order to prevent default click. If you just slide it around slightly, it will click anyway, since beforeChange and afterChange don't event trigger that way 😞

laveesingh commented 6 years ago

The referenced commit should solve the mentioned problem. The changes will be released soon.

JeffWeim commented 6 years ago

@laveesingh

Can this feature (from 7918201) be release as of now?

trumko commented 6 years ago

Looks like this commit partly breaks carousel functionality on mobile devices. because of

clickHandler = e => {
  if (this.clickable === false) {
    e.stopPropagation()
    e.preventDefault()
  }
  this.clickable = true
}

now after swiping user must tap 2 times on slider to trigger link.

alexeyMohnatkin commented 6 years ago

The bug still can be reproduced if I finish dragging on the slide. This happens because clickHandler fires after swipeMove is finished. Mac OS, Chrome 69

lm2almeida commented 5 years ago

Is there any update on this issue?

fmoulton commented 5 years ago

This can be fixed if you don't mind changing some internals of <InnerSlider>

onSwipe() {
    if (this.slider) {
      this.slider.innerSlider.clickable = true;
    }
  }

settings = {
    onSwipe: this.onSwipe.bind(this),
    swipeToSlide: true,
  };

// in render()
const sliderRef = slider => {this.slider = slider;};

// in return
<Slider {...settings} ref={sliderRef}>{this.slides}</Slider>

Fedeorlandau commented 5 years ago

This can be fixed if you don't mind changing some internals of <InnerSlider>

onSwipe() {
    if (this.slider) {
      this.slider.innerSlider.clickable = true;
    }
  }

settings = {
    onSwipe: this.onSwipe.bind(this),
    swipeToSlide: true,
  };

// in render()
const sliderRef = slider => {this.slider = slider;};

// in return
<Slider {...settings} ref={sliderRef}>{this.slides}</Slider>

This doesn't work for me. this.slider.innerSlider is undefined and it's still triggering the onClick on the slides

ZhekaVasil commented 5 years ago

There is another approach. We could use onClickCapture, onMouseUpCapture, onMouseDownCapture events to prevent click propagation during swiping:

export default class Carousel extends React.Component {
  swiping = false;
  carouselRef = React.createRef();

  handleMouseDown = event => {
    console.log('handleMouseDown');
    event.preventDefault();
  };

  handleMouseUp = () => {
    console.log('handleMouseUp');
    console.log(`swiping: ${this.carouselRef.current.innerSlider.state.swiping}`);
    this.swiping = this.carouselRef.current.innerSlider.state.swiping;
  };

  handleClick = event => {
    console.log('handleClick');
    console.log(`swiping: ${this.swiping}`);
    if (this.swiping) {
      event.preventDefault();
    }
  };

  /**
   * @see React.Component.render
   * @return {JSX}
   */
  render() {
    const { children } = this.props;
    const settings = {
      arrows: true,
      slidesToShow: 2,
      slidesToScroll: 2,
      speed: 400,
      dots: true,
      infinite: false,
      variableWidth: true,
      nextArrow: <ArrowButton next />,
      prevArrow: <ArrowButton prev />,
      dotsClass: 'slick-dots',
      responsive: [
        {
          breakpoint: 672,
          settings: {
            arrows: false,
            dots: false,
            slidesToShow: 1,
            slidesToScroll: 1,
            swipeToSlide: true,
          },
        },
      ],
    };
    return (
      <div className={classes.carousel__wrapper}>
        <Slider className={cx(classes.Slick)} ref={this.carouselRef} {...settings}>
          {React.Children.map(children, child => (
            <div onClickCapture={this.handleClick} onMouseUpCapture={this.handleMouseUp} onMouseDownCapture={this.handleMouseDown}>
              {child}
            </div>
          ))}
        </Slider>
      </div>
    );
  }
}

muzishuiji commented 4 years ago

You can try something like this:

class Carousel extends Component {
  constructor(props) {
    super(props);
    this.state = {isDragging: false};
  }

  shouldComponentUpdate(nextProps, nextState) {
    return this.state.isDragging === nextState.isDragging
  }

  onClickFunction = (e) => {
    if (this.state.isDragging) {
      e.preventDefault()
    }
  };

  render() {
    const settings = {
      ... // the rest of the settings
      beforeChange: () => this.setState({isDragging: true}),
      afterChange: () => this.setState({isDragging: false})
    };

    return (
      <div>
        <Slider {...settings}>
            <div><a href="#" onClick={this.onClickFunction}>one</a></div>
            <div><a href="#" onClick={this.onClickFunction}>two</a></div>
            <div><a href="#" onClick={this.onClickFunction}>three</a></div>
        </Slider>
      </div>
    )
  }
}

yes,you are right,thank you very much.

wangxingxing123654 commented 4 years ago

您可以尝试如下操作：

class Carousel extends Component {
  constructor(props) {
    super(props);
    this.state = {isDragging: false};
  }

  shouldComponentUpdate(nextProps, nextState) {
    return this.state.isDragging === nextState.isDragging
  }

  onClickFunction = (e) => {
    if (this.state.isDragging) {
      e.preventDefault()
    }
  };

  render() {
    const settings = {
      ... // the rest of the settings
      beforeChange: () => this.setState({isDragging: true}),
      afterChange: () => this.setState({isDragging: false})
    };

    return (
      <div>
        <Slider {...settings}>
            <div><a href="#" onClick={this.onClickFunction}>one</a></div>
            <div><a href="#" onClick={this.onClickFunction}>two</a></div>
            <div><a href="#" onClick={this.onClickFunction}>three</a></div>
        </Slider>
      </div>
    )
  }
}

是的，你是对的，非常感谢。

no.when drag too much time,it's out of work

Malekz commented 4 years ago

I do still have the same problem. Any help is much appreciated.

`import React, { Component } from 'react'; import PropTypes from 'prop-types'; import SlickSlider from 'react-slick'; import _throttle from 'lodash.throttle'; import _isEmpty from 'lodash.isempty'; import key from 'weak-key';

// components import SlideImage from './SlideImage'; import SlideVideo from './SlideVideo'; import useTracking from '../../../utils/hooks/useTracking'; import { sliderClick } from './trackingActions';

class Slider extends Component { constructor(props) { super(props);

this.slider = null;
this.handleResize = _throttle(this.handleResize.bind(this), 50);
this.handlePlay = this.handlePlay.bind(this);
this.handleCTAButtonClick = this.handleCTAButtonClick.bind(this);

this.state = {
  activePlayer: null,
};

}

componentDidMount() { if (typeof window !== 'undefined') { window.addEventListener('resize', this.handleResize); } }

componentDidUpdate(prevProps) { const { language, content } = this.props;

if (language !== prevProps.language) {
  // update slick slider after language change to get first slide and the right text color
  if (this.slider) {
    const items = content.items ? content.items : content;
    this.setSliderTextColor(items[0].textColor);
  }
}

}

componentWillUnmount() { if (typeof window !== 'undefined') { window.removeEventListener('resize', this.handleResize); } }

handleCTAButtonClick(text) { const track = useTracking(); track.trackEvent(sliderClick(text)); }

/**

set the slider color class regarding textColor attribute
@param textColor */ setSliderTextColor(textColor) { if (textColor === 'white') { this.sliderContainer.classList.add('slider-container--color-white'); } else { this.sliderContainer.classList.remove('slider-container--color-white'); } }

handleResize() { const playerNode = document.querySelector('.mi-content');

if (playerNode) { const videoContentWidth = playerNode.offsetWidth;

Array.from(document.querySelectorAll('.mi-controls')).forEach((element) => { // eslint-disable-next-line no-param-reassign element.style.width = ${videoContentWidth}px; }); } }

handlePlay(player) { const { slideDuration } = this.props;

this.setState({ activePlayer: player, });

this.handleResize();

if (slideDuration > 0 && this.slider) { this.slider.slickPause(); } }

handleStop() { const { slideDuration } = this.props;

if (slideDuration > 0 && this.slider) { this.slider.slickPlay(); } }

renderSlides() { const { catalogName, sliderType, sliderContext, ...content } = this.props; const items = content.items ? content.items : content;

return items.map((slide, index) => { if (slide.video && slide.video.videoId && sliderType !== 'hero-tile') { return ( <SlideVideo key={${key(slide)}-video} slide={slide} onPlay={this.handlePlay} onPause={this.handleStop} catalogName={catalogName} index={index} type={sliderType} context={sliderContext} sliderOnClickCTA={this.handleCTAButtonClick} /> ); } return ( <SlideImage key={${key(slide)}-image} slide={slide} catalogName={catalogName} index={index} type={sliderType} context={sliderContext} sliderOnClickCTA={this.handleCTAButtonClick} /> ); }); }

renderSlider() { const { activePlayer } = this.state; const { sliderType, slideDuration, ...content } = this.props; const items = content.items ? content.items : content;

const slickSettings = { dots: true, arrows: sliderType !== 'hero-tile', beforeChange: (currentSlide, nextSlide) => { if (activePlayer) { activePlayer.pause(); }
```
const nextSlideItem = items[nextSlide];

// should not be triggered on language change because nextSlide is not the right one
if (this.sliderContainer) {
  this.setSliderTextColor(nextSlideItem.textColor);
}
```
}, responsive: [ { breakpoint: 992, // Breakpoint "large" settings: { arrows: false, }, }, ], };

if (slideDuration > 0) { slickSettings.autoplay = true; slickSettings.autoplaySpeed = slideDuration; }

return ( <SlickSlider ref={(slider) => { this.slider = slider; }} {...slickSettings}

{this.renderSlides()} ); }

renderSingleSlide() { return (

{this.renderSlides()}

); }

render() { const { sliderType, items } = this.props; if (!items || _isEmpty(items)) { return null; }

let styleName = ''; if (items[0] && Object.prototype.hasOwnProperty.call(items[0], 'textColor')) { styleName = items[0].textColor === 'white' ? ' slider-container--color-white' : ''; }

const slider = ( <div className={slider-container slider-container--${sliderType}${styleName}} ref={(sliderContainer) => { this.sliderContainer = sliderContainer; }}

{items.length === 1 && this.renderSingleSlide()} {items.length > 1 && this.renderSlider()}

);

return slider; } }

Slider.propTypes = { language: PropTypes.string, content: PropTypes.objectOf(PropTypes.any), sliderType: PropTypes.string, sliderContext: PropTypes.string, catalogName: PropTypes.string, slideDuration: PropTypes.number, items: PropTypes.arrayOf(PropTypes.object), };

Slider.defaultProps = { language: null, content: null, items: null, sliderType: 'small', // or homepage or hero-tile catalogName: 'st_heroSlider', sliderContext: 'default', slideDuration: 6000, };

export default Slider; `

fdc-viktor-luft commented 4 years ago

It's been a long time ago, that I reported this issue. It's pretty sad, that the problem still exists. I recently decided to try it on my own. You can have a look at my library, which I already used many times, but it is much less customizable.

Differences:

Dragging can be disabled but works otherwise on all device types the similar
Clicking works only when not dragging
the package is much smaller (1.8 kB gzipped instead of 14.5 kB)
peer dependency on react > 16.8 (because hook based)
TypeScript support included

Here's a demo

And here you can find further information

But let me summarize: Maybe there are better solutions already out there :v:

logan272 commented 3 years ago

// Here's my work around.

import ReactSlick from 'react-slick'
const List = () => {
  return (
    <ReactSlick {...settings}>
      {array.map((item) => (
        <Item key={item.id} />
      ))}
    </ReactSlick>
  )
}

// if your item root is a `div`
const Item = () => {
  const [isPointMoved, setIsPointMoved] = useState(false)
  return (
    <div
      onPointerDown={() => {
        setIsPointMoved(false)
      }}
      onPointerMove={() => {
        setIsPointMoved(true)
      }}
      onPointerUp={() => {
        // if point moved, don't do any thing
        if (isPointMoved) {
          setIsPointMoved(true)
          return
        }
        // if point haven't moved, handle the click event as you wish
        handleClick()
      }}
    >
      {/* your content */}
    </div>
  )
}

// if your item root is an `a`
const Item = () => {
  const [isPointMoved, setIsPointMoved] = useState(false)
  return (
    <a
      href="https://example.com"
      onClick={(e) => {
        e.preventDefault()
      }}
      onPointerDown={() => {
        setIsPointMoved(false)
      }}
      onPointerMove={() => {
        setIsPointMoved(true)
      }}
      onPointerUp={() => {
        if (isPointMoved) {
          setIsPointMoved(true)
          return
        }
        handleClick()
      }}
    >
      {/* your content */}
    </a>
  )
}

Hope it helps!

jeroenoomsNL commented 2 years ago

// Here's my work around.

import ReactSlick from 'react-slick'
const List = () => {
  return (
    <ReactSlick {...settings}>
      {array.map((item) => (
        <Item key={item.id} />
      ))}
    </ReactSlick>
  )
}

// if your item root is a `div`
const Item = () => {
  const [isPointMoved, setIsPointMoved] = useState(false)
  return (
    <div
      onPointerDown={() => {
        setIsPointMoved(false)
      }}
      onPointerMove={() => {
        setIsPointMoved(true)
      }}
      onPointerUp={() => {
        // if point moved, don't do any thing
        if (isPointMoved) {
          setIsPointMoved(true)
          return
        }
        // if point haven't moved, handle the click event as you wish
        handleClick()
      }}
    >
      {/* your content */}
    </div>
  )
}

// if your item root is an `a`
const Item = () => {
  const [isPointMoved, setIsPointMoved] = useState(false)
  return (
    <a
      href="https://example.com"
      onClick={(e) => {
        e.preventDefault()
      }}
      onPointerDown={() => {
        setIsPointMoved(false)
      }}
      onPointerMove={() => {
        setIsPointMoved(true)
      }}
      onPointerUp={() => {
        if (isPointMoved) {
          setIsPointMoved(true)
          return
        }
        handleClick()
      }}
    >
      {/* your content */}
    </a>
  )
}

Hope it helps!

This is not a solid solution because the click will already be blocked when the pointer is just moving for 1 or 2 pixels, when it could have been a click instead of swiping.

Digirolamo commented 1 year ago

Here is my solution using TS. I just wanted a direct drop-in replacement.

import React, { useRef } from 'react';

import ReactSlickSlider, { Settings as ISliderProps } from 'react-slick';

export const Slider: React.FC<ISliderProps> = ({ ...props }) => {
  const clickableRef = useRef(true);
  const sliderRef = useRef<ReactSlickSlider>(null);

  const handleClick = (event: MouseEvent) => {
    if (!clickableRef.current) {
      event.stopPropagation();
      event.preventDefault();
    }
    clickableRef.current = true;
  };

  const swipeEvent = () => {
    if (sliderRef?.current?.innerSlider?.list) {
      sliderRef.current.innerSlider.list.onclick = handleClick;
      clickableRef.current = false;
    }
  };

  return <ReactSlickSlider ref={sliderRef} {...{ swipeEvent, ...props }} />;
};

export type { ISliderProps };
export default Slider;

Don't forget to set swipeToSlide=true if you want to slide more than one at a time. Also, just pass a swipeEvent prop to overwrite the behavior.

Anyone notice anything missing here?

benmotyka2 commented 1 year ago

// Here's my work around.

import ReactSlick from 'react-slick'
const List = () => {
  return (
    <ReactSlick {...settings}>
      {array.map((item) => (
        <Item key={item.id} />
      ))}
    </ReactSlick>
  )
}

// if your item root is a `div`
const Item = () => {
  const [isPointMoved, setIsPointMoved] = useState(false)
  return (
    <div
      onPointerDown={() => {
        setIsPointMoved(false)
      }}
      onPointerMove={() => {
        setIsPointMoved(true)
      }}
      onPointerUp={() => {
        // if point moved, don't do any thing
        if (isPointMoved) {
          setIsPointMoved(true)
          return
        }
        // if point haven't moved, handle the click event as you wish
        handleClick()
      }}
    >
      {/* your content */}
    </div>
  )
}

// if your item root is an `a`
const Item = () => {
  const [isPointMoved, setIsPointMoved] = useState(false)
  return (
    <a
      href="https://example.com"
      onClick={(e) => {
        e.preventDefault()
      }}
      onPointerDown={() => {
        setIsPointMoved(false)
      }}
      onPointerMove={() => {
        setIsPointMoved(true)
      }}
      onPointerUp={() => {
        if (isPointMoved) {
          setIsPointMoved(true)
          return
        }
        handleClick()
      }}
    >
      {/* your content */}
    </a>
  )
}

Hope it helps!

Perfect solution, many thanks!

I also believe that

if (isPointMoved) {
          setIsPointMoved(true)
          return
        }

can be changed to

if (isPointMoved) return

is that correct?

akiran / react-slick

Slider content should not be clickable while the user is dragging the slides on Desktop. #604