search

davidjerleke / embla-carousel

A lightweight carousel library with fluid motion and great swipe precision.
https://www.embla-carousel.com
MIT License
6.32k stars 188 forks source link

[Bug]: scrollNext() is unreliable #953

Closed Gitol closed 4 months ago

Gitol commented 4 months ago

Which variants of Embla Carousel are you using?

Steps to reproduce

I have several issues with scrollNext() in my app, making the experience with the EMBLA carousel often unusable. The issue is random, so it does not always reproduce. It can be due to timing. Pressing F12 in and out (dev tools with smaller resolutions) can help reproduce the issue, too.

For my app, the buttons are triggered by onMouseDown() and onTouchStart() because iOS and Android behave a bit differently, and the standard click loses too many inputs.

There are two main issues that I have seen that I cannot repro as-is on the sandbox: 1) A quick click on scrollNext() starts scrolling down, stops at 10%, and comes back up. Random (but easy to repro) 2) A quick click on scrollNext() does not skip 1 but 2 slides (easy to check as I can swipe up to see the slide that got skipped). Random. It's a bit harder to repro, but I still have it consistently. I am unsure why.

The Sandbox is showing a variant of the problem (1), but not to its full extent. I can't repro problem (2) yet on the sandbox, but it's probably an issue in the same area.

Repro case:

When you are using a mobile device with very flaky input, such as touch screens, these kinds of behaviors are unforgiving. But I can have this on the mouse, too (and I am pretty accurate with my click not to move, so there could be some other stuff involved).

Thanks!

Expected Behavior

I call nextSlide() and/or prevSlide(), and EMBLA carousel slide to the next/prev slide, uninterrupted by any mouse/swipe motion, until it finishes the full swipe.

Also when calling nextSlide() / prevSlide(), it moves by only one slide, not two slides.

Additional Context

I added some console.log(), and no, we call nextSlide() only once, but still, it often skips zero or 2 slides.

What browsers are you seeing the problem on?

All browsers

Version

v8.1.6

CodeSandbox

https://codesandbox.io/p/sandbox/embla-carousel-y-axis-react-forked-tfx29s

Before submitting

davidjerleke commented 4 months ago

@Gitol thanks for you report.

Embla has many users, over 50 000 repositories are using Embla and this feature has automatic tests so any breaking changes will cause the tests to fail. If this was a blocker bug like you describe it, it would have been caught a long time ago.

Please share screen recordings of the behavior you describe so I can better understand the behavior you’re describing.

davidjerleke commented 4 months ago

@Gitol after testing your sandbox I found the following problem: Most of your buttons aren't even clickable because they're placed beneath a layer that covers it. So this is a CSS problem on your side. Add the following styles to your buttons and you will be able to click on every button:

.my-button-class {
  position: relative;
  z-index: 1;
}
Gitol commented 4 months ago

Thanks! Updated the sandbox to work on all buttons (instead of just the first).

Here is how to simply repro a variant of issue 1: Click on the "Next" button and move your mouse by 1 pixel simultaneously.

Instead of smoothly going to the next slide, it directly skips to the next slide.

I am still trying to repro the original issues (1) and (2) in a simpler sandbox than in my app.

davidjerleke commented 4 months ago

@Gitol this is most likely a faulty implementation on your side. Please share screen recordings of the behavior you describe. Otherwise, this issue will be closed.

Thanks for cooperating.

Gitol commented 4 months ago

I just shared the sandbox representing one of the issues. That can be looked into in the meantime, no?

davidjerleke commented 4 months ago

@Gitol the problem in this specific case is that I don’t understand your description of the problems you’re experiencing. Screen recordings may help.

Gitol commented 4 months ago

Great. I was able to repro both issues (1) and (2) on a much simpler case.

(1) is because of using onTouchStart() - Not much way around it for iOS, though (without losing clicks).

(2) is because of using onTouchStart() and onMouseDown(). One of my engs removed the debounce logic, so we get the two events in a row onTouchStart() and onMouseDown(), thus 2 nextSlide() in a row in some cases. That one, I can fix easily on my side.

And (3) with the earlier issue in the sandbox.

(1) repros on Chrome and Safari (iOS), not in the sandbox. (3) repros only in the sandbox version.

Going to record the video now for (1). May do one for (3) as well as it could repro in some other browsers.

Gitol commented 4 months ago

Repro Issue 1.

https://github.com/user-attachments/assets/8046906e-3552-472e-a2d1-54752547c8dc

Sorry for the potato quality. It has to be less than 10 MiB. You can repro the issue in Chrome (and iOS) if you do a quick click. For a longer click, it will scroll down. And if that's too long, you get the right menu (not getting captured in the video).

The code:

            <section className="embla">
              <div className="embla__viewport" ref={emblaRef}>
                <div className="embla__container">
                  {slides.map((d, index) => {
                    return (
                      <div className="embla__slide" key={index}>
                        <LazyLoadImage
                          src={`https://picsum.photos/200/${300+index}`}
                          className="carousel-image position-absolute"
                          threshold={10000}
                        />
                        <div className="carousel-content-bottom">
                          <div className="feed-location-zone">
                            <Button
                              className="feed-location-button"
                              onTouchStart={(e) =>
                                nextSlide('onTouchStart')
                              }
                            >
                              <b>
                                SOME TEXT
                              </b>
                            </Button>
                          </div>
                          <div className="feed-swipe-zone">
                            <Button
                              onTouchStart={(e) =>
                                nextSlide('onTouchStart')
                              }
                            >
                              SWIPE
                            </Button>
                          </div>
                        </div>
                      </div>
                    );
                  })}
                </div>
              </div>
            </section>

With:

  const nextSlide = (text) => {
    console.log('nextSlide(%s) - %s', counter, text);
    counter += 1;
    emblaApi.scrollNext();
  };
Gitol commented 4 months ago

Repro for Issue 3. I first click without moving the mouse the first few times. Good animation. Then I do the same clicking a few times, but this time, I move the mouse barely as I click (~1 pixel). And we can see the slide transition being completely skipped and the swipe animation taking over. Looks buggy for people clicking not as precisely.

https://github.com/user-attachments/assets/11028592-de67-4c31-962f-db1bcb13d6b4

Code in this sandbox: https://codesandbox.io/p/sandbox/embla-carousel-y-axis-react-forked-tfx29s

Gitol commented 4 months ago

Thank you for the help!

davidjerleke commented 4 months ago

@Gitol why are you using touchStart instead of click events?

Gitol commented 4 months ago

Because iOS drops a bunch of clicks with the click events. I have roughly 30+% of click losses. It feels very buggy. And as you retry over and over, if you touch for too long, then it becomes a select.

Chrome on Desktops (and Android) does not have that issue (although I switched to onMouseDown for them to match the iOS model. onMouseDown does not do it as well on iOS). Annoying to have these differences on various Browsers.

davidjerleke commented 4 months ago

@Gitol yes but I think iOS won't fire the click event when a touch event includes an accidental slight swipe gesture. One benefit with this approach is that you filter out accidental clicks that were actually intended to be swipe gestures.

davidjerleke commented 4 months ago

You can shorten the iOS click threshold by adding this to your buttons:

button {
  touch-action: manipulation;
}

With the above CSS, iOS won’t wait for a double tap to zoom before firing the click event, making the click event snappier.

Gitol commented 4 months ago

I understand. The additional problem with a caroussel implementation is that touching the screen (even is imprecise) may trigger a swipe motion while the user wanted a click.

Thanks! I will try the { touch-action: manipulation; }

davidjerleke commented 4 months ago

@Gitol please read about the dragThreshold option in the docs.

davidjerleke commented 4 months ago

@Gitol the carousel scrollNext and scrollPrev are not designed to work with custom touch handlers when placed inside slides or the container. This is because it’s the area where the carousel is listening for touch/mouse interactions as explained here. Use click handlers on buttons if you want them inside the container or slides.

Note that you can still add touch listeners to any button placed outside the slides and the carousel container.

Gitol commented 4 months ago

Good to know. I will try your suggestions. Getting back to you very soon. Thank you!

Unfortunately, for my UX, the slides are fullscreen, so some buttons are on top of the slide.

davidjerleke commented 4 months ago

Unfortunately, for my UX, the slides are fullscreen, so some buttons are on top of the slide.

@Gitol in that case you should use click listeners.

Gitol commented 4 months ago

I confirm that using { touch-action: manipulation; } allowed me to use onClick() again, with an acceptable click rate for iOS.

Thank you for the guidance!

davidjerleke commented 4 months ago

Glad to hear that @Gitol 👍🏻.