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

clickScroll with pointerType: "pen": scroll is aborted if pen leaves scrollbar track #630

Closed
tryggvigy opened this issue Apr 26, 2024 · 11 comments · Fixed by #631
Closed

Comments

@tryggvigy
Copy link
Contributor

tryggvigy commented Apr 26, 2024

Describe the bug
Hey there! We got this bug reported to us, where users using the wacom pen as a mouse input device are having a hard time using the overlay scrollbars. Here is a video demonstrating the issue:

Screen.Recording.2024-04-22.at.15.11.36.mp4

To Reproduce
Steps to reproduce the behavior:

  1. Use a pen input device like wacom
  2. Enable clickScroll: true
  3. grab the scrollbar handle
  4. move the mouse out of the scrollbar track
  5. scrolling stops even though "pointerup" has not fired yet

Expected behavior

Scrolling to continue until the user releases the pen

Examples

TODO: I'll add a minimum repro example here asap!

Please create a small example of the bug.
To do this you can use online platforms like JSFiddle, CodeSandbox or StackBlitz. You can also create a separate Github repository which I can clone.

Environment

  • Used Operating System(s):
  • MacOS
  • Used Browser(s) (with version):
  • Chrome 124

Additional context
I dug into this a little bit with breakpoints. I can see that releasePointerCapture is called differently for pen and mouse.

With a mouse it triggers when the mouse button is released as expected:

pointerType: "mouse"
target: div.os-scrollbar-handle
type: "pointerup"

With a pen, it triggers on pointerleave of the .os-scrollbar-track element, consistent with the bug behaviour.

pointerType: "pen"
target: div.os-scrollbar-track
type: "pointerleave"

I guess the question is why the second one is being triggered for pen input devices there.

This might be hard to reproduce for maintainers so I'm happy to help. I got my hands on a wacom pen here.

@KingSora
Copy link
Owner

KingSora commented Apr 26, 2024

Good day @tryggvigy :)

This seems a bit weird indeed.. The same logic works for mouse and touch devices, so it should also work for pen (at least thats the assumption). I can also confirm the correct behavior with an iPad & Pen.

Unfortunately I dont have a wacom device or anything related to that...

Do you have any suggestions going forward? - I'm open for anything.

@KingSora
Copy link
Owner

@tryggvigy I'm not able to test it, but you could play with the releasePointerCaptureEvents variable. Its possible that some events shouldn't be part of this variable to invoke the releasePointerCapture function.

@KingSora
Copy link
Owner

KingSora commented Apr 27, 2024

@tryggvigy I've also found this issue which looks similar to ours: https://stackoverflow.com/questions/59010779/pointer-event-issue-pointercancel-with-pressure-input-pen

They suggest to use the css style touch-action: none. In OverlayScrollbars case this style should be applied to the selector .os-scrollbar, .os-scrollbar *. If that alone is not helping you could also try to call pointerDownEvent.preventDefault().

@tryggvigy
Copy link
Contributor Author

tryggvigy commented Apr 27, 2024

Thank for the pointers @KingSora! I will investigate this more this week. Hopefully i can find a cause/solution. I found someone in the office that borrowed me the pen and i can reproduce so that's a good start :)

I'll keep this issue updated if I find something!

@tryggvigy
Copy link
Contributor Author

tryggvigy commented Apr 29, 2024

Some new findings:

  • I can reproduce this in https://kingsora.github.io/OverlayScrollbars/example/overlayscrollbars/
  • This bug is only present in Chrome and Edge, not Safari and Firefox. So I assume it's caused by Chromium handling pointer events for pen slightly differently.
  • Removing 'pointerleave' from releasePointerCaptureEvents makes this bug go away but I assume it could cause issues elsewhere doing this.
  • Calling pointerDownEvent.preventDefault() also fixes the issue I thought this since sometimes it works as expected but this does in fact not solve the issue
  • right clicking with the pen causes the expected click scrolling behaviour afterwards.

@KingSora
Copy link
Owner

KingSora commented Apr 29, 2024

@tryggvigy Thanks for this update! According to MDN pointerleave should not even fire when setPointerCapture is called: https://developer.mozilla.org/en-US/docs/Web/API/Element/setPointerCapture I just added it there back then to be "on the safe side"

I would test the library again without this callback and fix in this way if we don't find a better solution.

@tryggvigy
Copy link
Contributor Author

tryggvigy commented Apr 29, 2024

@KingSora removing pointerCaptureElement.setPointerCapture(pointerDownEvent.pointerId); seems to have no effect.

The only way I've found around the issue is to remove 'pointerleave' from releasePointerCaptureEvents.

Some things I'm wondering:

  1. I'm fairly sure this issue is only present in v2 of overlay-scrollbars not v1. Do you know what high level differences there are with the event chains on the track/handle/etc between v1 and v2?
  2. It's intuitive to me that we would want to handle const releasePointerCaptureEvents = 'pointerup pointercancel lostpointercapture'; while 'pointerdown' is happening but I don't understand quite yet the pointerleave of the track element should cause releasePointerCapture() like the current logic is. If you have any background on why this is the way it is today I'd love to learn that!

Do you think it would be a safe "fix" to stop handling pointerleave on the track?

All tests seem to pass with this change

- const releasePointerCaptureEvents = 'pointerup pointerleave pointercancel lostpointercapture';
+ const releasePointerCaptureEvents = 'pointerup pointercancel lostpointercapture';

Thank you for engaging with me on this issue!

@KingSora
Copy link
Owner

KingSora commented Apr 29, 2024

@tryggvigy:

  1. The way I'm handling interactions completely changed between v1 and v2. Since I wanted v1 to be compatible with IE 11 and the concept of pointerevents or pointer capture wasn available there I had to handle interactivity like this: First detect an interaction on the element itself (e.g. mousedown / touchdown) then register global document listeners like mousemove or touchmove and track the movement on the document, since otherwise the same behavior would appear as you are observing right now with this bug just with mouse or touch. After the user releases the pointer (mouseup, touchup), or cancels it, the interaction would end and all added document listeners are removed again. This approach has its own drawbacks (e.g. when the pointer would leave the document / window the interaction would be canceled, other elements could still receive pointer events and trigger conflciting behaviors etc.) and its much more code to maintain.
  2. In theory I would not need any of the events: pointerup pointerleave pointercancel lostpointercapture because according to MDN pointerleave should not be fired and pointerup or pointercancel implicitly release the pointer capture. lostpointercapture should fire after a pointer capture is released so it should have no effect at all. So the purpose of all these events is basically just "I want to be 'smart' and on the safe side" assuming that if any of the events fires while a pointer capture is in progress something went wrong and the pointer capture is potentially never released if I dont do it manually. (at least that was my intention back then)

I personally think it should be fine to remove the pointerleave lostpointercapture from the list and still assume everything to work properly since manually releasing pointer events is anyway not needed. (at least according to MDN).

Obviosuly before releasing the fix I would properly test the changes with all input devices I have (mouse, touch, pen (iOS))..

@tryggvigy
Copy link
Contributor Author

tryggvigy commented Apr 29, 2024

great! I'll make a PR to remove perhaps only the pointerleave on the track to be safe.

Until then I can work around this issue with

  useEffect(() => {
    const track = instance()?.elements().scrollbarVertical.track;
    if (!track) return (): void => {};

    const handlePointerLeave = (e: PointerEvent): void => {
      // On Chromium browsers, pen causes a pointerleave event to be fired
      // when the pen leaves the track, this stops click scrolling in
      // overlay-scrollbars currently. This works around this chromium bug.
      if (pointerType === 'pen') {
        e.stopImmediatePropagation();
      }
    };
    track.addEventListener('pointerleave', handlePointerLeave);
    return (): void => {
      track.removeEventListener('pointerleave', handlePointerLeave);
    };
  }, [instance, pointerType]);

@KingSora
Copy link
Owner

KingSora commented May 6, 2024

@tryggvigy I've decided to include a few other things into the next release, thats why there is no new version yet. I'm confident to have a release this week :)

@tryggvigy
Copy link
Contributor Author

Nice!

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 a pull request may close this issue.

2 participants