cybersemics / em

A beautiful, minimalistic note-taking app for personal sensemaking.
Other
288 stars 121 forks source link

[Android] Gesture incorrectly activated in scroll zone #2605

Closed raineorshine closed 1 week ago

raineorshine commented 1 week ago

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.

https://github.com/user-attachments/assets/1e5e6e7d-3319-4782-9697-a404625e835e

Expected Behavior

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

snqb commented 1 week ago

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.

https://github.com/user-attachments/assets/bacac01d-8206-4f8a-86d0-203a7a7245ff

snqb commented 1 week ago

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 commented 1 week ago

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: https://github.com/cybersemics/em/issues/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 commented 1 week ago

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 commented 1 week ago

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:

https://github.com/cybersemics/em/blob/8760cae0efa8ecd9dd4760dfc4fd7b6f75da43e3/src/components/TraceGesture.tsx#L56

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.

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.

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.