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

[Android] Gesture incorrectly activated in scroll zone #2605

Closed
raineorshine opened this issue Nov 21, 2024 · 5 comments · Fixed by #2617
Closed

[Android] Gesture incorrectly activated in scroll zone #2605

raineorshine opened this issue Nov 21, 2024 · 5 comments · Fixed by #2617
Assignees
Labels
bug Something isn't working

Comments

@raineorshine
Copy link
Contributor

Android Chrome only.

Steps to Reproduce

Scroll by dragging in the scroll zone.

Current Behavior

Sometimes the TraceGesture is visible and a gesture is activated.

2024_11_21_19_18_01.mp4

Expected Behavior

Gestures should not be able to be activated when a touch starts in the scroll zone.

@raineorshine raineorshine added the bug Something isn't working label Nov 21, 2024
@snqb

This comment was marked as off-topic.

@snqb
Copy link
Collaborator

snqb commented Nov 22, 2024

And concerning the gesture: I think I'll just tweak the PanResponder and it should be gone.
It's just wrongly capturing scroll motions, but I just haven't gotten to that yet.

@raineorshine
Copy link
Contributor Author

Not the gesture issue itself, but scroll being choppy and slow is not an android exclusive, it looks like. From a very preliminary research I can conclude that it's the fact that we virtualize thoughts, that leads to choppiness.

Attaching a chromium browser test on Windows. I guess, in the end we want to have some better kind of logic behind virtualization. If this thing is choppy on a powerful PC, then it's gonna be(and is) atrocious on less powerful Android devices.

Thanks, that's good to know. We have an existing issue to track this problem: #2399

And concerning the gesture: I think I'll just tweak the PanResponder and it should be gone.
It's just wrongly capturing scroll motions, but I just haven't gotten to that yet.

Okay, sounds good!

@snqb
Copy link
Collaborator

snqb commented Nov 22, 2024

A bit more of updates.
After more digging, I realized, that it's mostly the TraceGesture that has no notion of scroll vs gesture. So a lot of times it could paint a gesture, while we're scrolling, whilst not triggering testures.

I think it's wrong, and we should only show the trace on definite gestures.
Preliminarily turning on this.scrolling on responder events and rendering TraceGesture conditionally helps, but is kinda ugly.

There's a lot to grasp for rn in the MultiGesture, but I'm at 80% of certainty I get everything right.
The plan is to tweak the PanResponder to differentiate nicely scrolls and gestures, basically.

But while on that, I've seen this thing. It splits the windows in scroll/gesture parts, and does through "touchstart". The comment says about moving it. It could decrease the usage of imperative class-level flags.
1)Is it fine if I touch this thing here too? I feel like mixing dom-level touchstart & panresponder doesn't help a lot. And I'm doing quite a bit in PanResponder already.
2) So if we're on the left side of the screen, shall we never trigger scroll for righties? Sounds alright to me, as it's a simple way to separate those, but still wanna be sure, as it could be called hijacking in one way or another.

 // enable/disable scrolling based on where the user clicks
    // TODO: Could this be moved to onMoveShouldSetResponder?
    document.body.addEventListener('touchstart', e => {
      if (e?.touches.length > 0) {
        const x = e.touches[0].clientX
        const y = e.touches[0].clientY
        this.clientStart = { x, y }

        // disable gestures in the scroll zone on the right side of the screen
        // disable scroll in the gesture zone on the left side of the screen
        // (reverse in left-handed mode)
        const viewport = viewportStore.getState()
        const scrollZoneWidth = viewport.scrollZoneWidth
        console.log("ScrollZoneWidth", scrollZoneWidth)
        const isInGestureZone =
          (this.leftHanded ? x > scrollZoneWidth : x < viewport.innerWidth - scrollZoneWidth) && y > TOOLBAR_HEIGHT
        if (isInGestureZone && !props.shouldCancelGesture?.()) {
          this.disableScroll = true
        } else {
          this.scrolling = true;
          this.abandon = true
        }
      }
    })

@raineorshine
Copy link
Contributor Author

Hi, thanks for the update.

After more digging, I realized, that it's mostly the TraceGesture that has no notion of scroll vs gesture. So a lot of times it could paint a gesture, while we're scrolling, whilst not triggering testures.

That's not true, TraceGesture is specifically only shown when a gesture is in progress:

const gestureStarted = gestureStore.useSelector(gesturePath => gesturePath.length > 0)

The plan is to tweak the PanResponder to differentiate nicely scrolls and gestures, basically.

I'm not sure what you have in mind, but note that the logic already works correctly in Chrome and Safari. The question is, where does it behave differently on Android and why? I'm not comfortable changing the existing logic that works correctly on all platforms except Android.

1)Is it fine if I touch this thing here too? I feel like mixing dom-level touchstart & panresponder doesn't help a lot. And I'm doing quite a bit in PanResponder already.

I agree with you that mixing dom-level touchstart and PanResponder is not ideal. However, by altering something that is already working on most platforms you would be creating a significant risk of introducing a regression. I don't recommend you go down this path.

  1. So if we're on the left side of the screen, shall we never trigger scroll for righties? Sounds alright to me, as it's a simple way to separate those, but still wanna be sure, as it could be called hijacking in one way or another.

I don't quite understand what you're asking. If touchstart occurs on the left side of the screen, then scroll is disabled. If touchstart occurs on the right side of the screen, then the gesture is abandoned. In left-handed mode this is reversed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants