Another option might be to fork TouchBackendImpl and have it subscribe to some sort of state so that it's actually tied to useLongPress rather than using setTimeout to approximate the coupling.

Maybe I should have started a new PR for #3074, but my new changes rely on the existing changes in here, so I combined them.

The effect in useDragHold was clearing dragHold as soon as it detected the beginning of the drag, which means that when the drag was canceled, there was nothing left to indicate to shouldCancelGesture that the long press was still in progress.

I went through the uses of state.dragHold in the app, and I'll break them down here:

AppComponent:

• shouldCancelGesture wasn't checking dragHold, now it is to fix this issue and [Mobile] Bullet incorrectly highlighted on gesture #3072
• sets data-drag-hold attribute using dragHold

Bullet:

• short-circuits clickHandler if dragHold, doesn't use dragInProgress

Editable:

• refactored to put dragHold and dragInProgress together in Move dragHold into dispatch #3109

QuickDropPanel:

• checks them both together to form isDragging flag

useEditMode:

• short-circuits setSelectionToCursorOffset if dragHold is true, ignores dragInProgress

useDragHold:

• this contains the effect that I modified in 3ea84c3 just now, which still sets isPressed to false and dispatches clearMulticursors if dragHold is true

So the logic is a little too complex to say definitively that it's OK to let dragHold be true until touchend, but I think that it is OK. Bullet and useEditMode are using dragHold to block click/tap events, and those handlers are ignoring dragInProgress, presumably because drags can't trigger those interactions. I don't think it should make any difference if dragHold remains true until the long press ends.

I did test my changes against #2847 and I don't see any difference if I remove

      // If we were dragging but now we're not, make sure to reset the lock
      if (!isDragging && state.dragHold) {
        // Reset the lock to allow immediate long press after drag ends
        longPressStore.unlock()
      }

Both endDrag (in useDragAndDropThought) and stop (in useLongPress) perform both of the functions that I removed from useDragHold:

1. set dragHold to false
2. set globals.longpressing to false

so I believe that we should be covered whether or not this comment from useDragHold comes into play:

// react-dnd stops propagation so onLongPressEnd sometimes doesn't get called.

I think I'm about ready to start experimenting with forking TouchBackendImpl.ts in order to solve the original issue, #3072. Tying long press behavior more directly to drag-and-drop behavior would prevent timing issues such as this.

Hi! Thanks for all the information.

I can prevent the gesture from showing after the dragHold action fires by adding that condition to shouldCancelGesture.

That makes sense. This seems relatively independent from other changes. Could we make this change in a separate PR? Cleaving off functionality on a complex issue like this will simplify the review process and provide a more granular commit history to track regressions.

However, drag-and-drop does not activate, most likely because

setTimeout(
  this.handleTopMoveStart.bind(this, e as any),
  delay,
)

in TouchBackendImpl.ts does not fire at the exact same time as

      timerIdRef.current = setTimeout(() => {
        globals.longpressing = true
        haptics.light()
        onLongPressStart?.()
        longPressStore.lock()
        timerIdRef.current = 0
        if (!unmounted.current) {
          setPressed(true)
        }
      }, delay)

in useLongPress.

For these kinds of references, it would be useful if you could link to the specific line(s) of code you are referring to in the codebase. That would make it quicker for me to get up to speed, as I wouldn't have to look it up by filename or Search in Files.

If you use "Copy Permalink", then GitHub will insert the snippet:

https://github.com/cybersemics/em/blob/1fee57f22a55cc376151468b2c1a8a43fadb5c61/src/hooks/useLongPress.ts#L47-L57

There may be an extension for VS Code that makes this easier to do from your editor, but I'm not sure. So far I have been browsing the codebase on GitHub to get this.

Hope that's helpful :)

I'm still looking at TouchBackendImpl to try to verify that this is related to setTimeout. Would you be open to subtracting a few milliseconds from TIMEOUT_LONG_PRESS_THOUGHT in:

<DndProvider
    backend={isTouch ? TouchBackend : HTML5Backend}
    options={{ delayTouchStart: TIMEOUT_LONG_PRESS_THOUGHT, preview: true }}
  >

in order to guarantee that drag-and-drop activates before long press, so that quick movements don't get discarded?

I would be open to it, if it's reliable. As you've pointed out, the root of the problem seems to be that DndProvider and useLongPress are not synchronized.

If we reduced delayTouchStart, how confident are you that react-dnd's beginDrag would always fire before useLongPress?

Are there any potential downsides to beginDrag firing noticeably before useLongPress? The bullet is highlighted when holding or dragging, so that shouldn't be a problem. I'm less clear about the impact on other uses of state.dragHold.

I'll make a high level comment as I'm trying to wrap my head around all this: Conceptually, a clean transition from state.dragHold to state.dragInProgress makes the most sense, i.e. they are mutually exclusive and state.dragHold is set to false immediately when state.dragInProgress is set to true. However, if there are practical reasons to allow some overlap, that could be acceptable. It's just hard to evaluate.

So the logic is a little too complex to say definitively that it's OK to let dragHold be true until touchend, but I think that it is OK. Bullet and useEditMode are using dragHold to block click/tap events, and those handlers are ignoring dragInProgress, presumably because drags can't trigger those interactions. I don't think it should make any difference if dragHold remains true until the long press ends.

This is the only part that doesn't seem quite right to me. However please correct me if I'm missing your point.

• dragHold is true between touchstart and touchend when the user has not moved their finger at all.
• dragHold is also true between touchstart and touchend if the user only moves their finger a tiny bit, specifically beneath SCROLL_THRESHOLD of 10px. (Despite the name, this has nothing to do with scrolling... that's a vestige from when the gesture zone and scroll zone were one and the same.)
• However, dragHold is not true until touchend when the user's finger moves more than the threshold. In that case, the user is no longer holding the thought, and is actively dragging.

The idea is that dragHold and dragInProgress do not overlap. dragHold should just be used for when the user is holding a thought, and has not initiated a drag-and-drop.

I think I'm about ready to start experimenting with forking TouchBackendImpl.ts in order to solve the original issue, #3072. Tying long press behavior more directly to drag-and-drop behavior would prevent timing issues such as this.

We can hook into beginDrag if need to. I think that would correspond to the start of the drag, after delayTouchStart. Do you need deeper integration with TouchBackendImpl?

FYI We're using a patched react-dnd because react-dnd/react-dnd#3664 hasn't been merged.

This is the only part that doesn't seem quite right to me. However please correct me if I'm missing your point.

dragHold is true between touchstart and touchend when the user has not moved their finger at all.
dragHold is also true between touchstart and touchend if the user only moves their finger a tiny bit, specifically beneath SCROLL_THRESHOLD of 10px. (Despite the name, this has nothing to do with scrolling... that's a vestige from when the gesture zone and scroll zone were one and the same.)
However, dragHold is not true until touchend when the user's finger moves more than the threshold. In that case, the user is no > longer holding the thought, and is actively dragging.
The idea is that dragHold and dragInProgress do not overlap. dragHold should just be used for when the user is holding a thought, and has not initiated a drag-and-drop.

The issue with #3074 is that dragHold becomes false as soon as the drag starts, then dragInProgress becomes false once they scratch to cancel. At that point, we need something for shouldCancelGesture to check in order to know that the long press is still ongoing.

I realize now that globals.longpressing would probably work fine for that. Prior to this PR, it would also get set to false on touchmove (significant move exceeding the SCROLL_THRESHOLD, that is) but in order to fix #3072 I changed it so that stop doesn't get called in useLongPress as long as they have been pressing for long enough for the timeout to fire. This is also a significant change, since anything relying on globals.longpressing will now see it being true for much longer.

So I see the logic in putting dragHold back the way it was and using globals.longpressing instead. Does that sound like it would work?

Hope that's helpful :)

It is, thanks. I was trying to get everything written down before I had to stop working for the day, but a lot of those big snippets could have been linked instead.

The issue with #3074 is that dragHold becomes false as soon as the drag starts, then dragInProgress becomes false once they scratch to cancel. At that point, we need something for shouldCancelGesture to check in order to know that the long press is still ongoing.

I see, thank you. This makes total sense.

I realize now that globals.longpressing would probably work fine for that. Prior to this PR, it would also get set to false on touchmove (significant move exceeding the SCROLL_THRESHOLD, that is) but in order to fix #3072 I changed it so that stop doesn't get called in useLongPress as long as they have been pressing for long enough for the timeout to fire. This is also a significant change, since anything relying on globals.longpressing will now see it being true for much longer.

I wouldn't be comfortable allowing globals.longpressing to stay true after a significant touchmove. It's too big a change to the semantics. If the user moves their finger more than the threshold, then they are no longer long-pressing.

So I see the logic in putting dragHold back the way it was and using globals.longpressing instead. Does that sound like it would work?

Yes, I think that would definitely work.

Another approach would be to add state.dragCancelled to represent the time after a drag is cancelled but before touchend. This would keep the state in Redux instead of adding another dependency on the dubious global variables. This could give us a gradual off ramp from globals.longpressing.

In the short term, it adds complexity by adding another touch/drag-related state variable. In the long term, it helps us transition from globals to Redux state. globals.longpressing doesn't change all that often that it needs to be kept out of the Redux state, like the editingValue ministore.

What do you think?

I would be open to it, if it's reliable. As you've pointed out, the root of the problem seems to be that DndProvider and useLongPress are not synchronized.

If we reduced delayTouchStart, how confident are you that react-dnd's beginDrag would always fire before useLongPress?

It depends on how many milliseconds we reduced it by. I would assume that 25ms would be good enough, but would need to test it out.

Are there any potential downsides to beginDrag firing noticeably before useLongPress? The bullet is highlighted when holding or dragging, so that shouldn't be a problem. I'm less clear about the impact on other uses of state.dragHold.

I think the only downside is that it might not work. I don't think there's anything wrong with drag-and-drop beginning before the long press, but the events might interfere or get blocked. We would need to test it out to know for sure.

We can hook into beginDrag if need to. I think that would correspond to the start of the drag, after delayTouchStart. Do you need deeper integration with TouchBackendImpl?

beginDrag doesn't fire until TouchBackendImpl tells it to (after the timeout) so the deeper integration would mean being able to subscribe to something rather than relying on setTimeout to try to line up the different types of long press interactions.

Another approach would be to add state.dragCancelled to represent the time after a drag is cancelled but before touchend. This would keep the state in Redux instead of adding another dependency on the dubious global variables. This could give us a gradual off ramp from globals.longpressing.

In the short term, it adds complexity by adding another touch/drag-related state variable. In the long term, it helps us transition from globals to Redux state. globals.longpressing doesn't change all that often that it needs to be kept out of the Redux state, like the editingValue ministore.

What do you think?

I think that sounds good, and it sounds like it will work. I could also see moving to a single variable that would be a sort of enum for the different state that it's in, like LongPressState.DRAG_HOLD, LongPressState.DRAG_IN_PROGRESS, LongPressState.DRAG_CANCELLED, etc. Functionally, it wouldn't matter one way or the other.

It looks like globals.longpressing is only checked in useLongPress, and it doesn't necessarily need to be a global variable. It could pull a variable straight out of the Redux state, with the only caveat being that it might change in the time it takes for the 10ms setTimeout to run so maybe it would need to be stored in a ref instead.

That makes sense. This seems relatively independent from other changes. Could we make this change in a separate PR? Cleaving off functionality on a complex issue like this will simplify the review process and provide a more granular commit history to track regressions.

I can start a new one. It sounds like we are also going to revert the other changes in here and add some sort of dragCancelled variable that would also be checked in shouldCancelGesture. I'll get a new PR started.

I would be open to it, if it's reliable. As you've pointed out, the root of the problem seems to be that DndProvider and useLongPress are not synchronized.
If we reduced delayTouchStart, how confident are you that react-dnd's beginDrag would always fire before useLongPress?

It depends on how many milliseconds we reduced it by. I would assume that 25ms would be good enough, but would need to test it out.

Are there any potential downsides to beginDrag firing noticeably before useLongPress? The bullet is highlighted when holding or dragging, so that shouldn't be a problem. I'm less clear about the impact on other uses of state.dragHold.

I think the only downside is that it might not work. I don't think there's anything wrong with drag-and-drop beginning before the long press, but the events might interfere or get blocked. We would need to test it out to know for sure.

Okay, I'm open to an empirical approach.

We can hook into beginDrag if need to. I think that would correspond to the start of the drag, after delayTouchStart. Do you need deeper integration with TouchBackendImpl?

beginDrag doesn't fire until TouchBackendImpl tells it to (after the timeout) so the deeper integration would mean being able to subscribe to something rather than relying on setTimeout to try to line up the different types of long press interactions.

That's what I mean, you can use beginDrag to hook into TouchBackendImpl's setTimeout. Is there a different event from react-dnd that we need to hook into?

I think that sounds good, and it sounds like it will work. I could also see moving to a single variable that would be a sort of enum for the different state that it's in, like LongPressState.DRAG_HOLD, LongPressState.DRAG_IN_PROGRESS, LongPressState.DRAG_CANCELLED, etc. Functionally, it wouldn't matter one way or the other.

Yes! I like that. It would enforce mutual exclusivity. (We use PascalCase for enum values.)

It looks like globals.longpressing is only checked in useLongPress, and it doesn't necessarily need to be a global variable. It could pull a variable straight out of the Redux state...

This might just be semantics, but the Redux state is global. It just forces updates to occur in the order they are dispatched, helping with race conditions.

...with the only caveat being that it might change in the time it takes for the 10ms setTimeout to run...

Yeah, this is a concern.

...so maybe it would need to be stored in a ref instead.

I'm not sure about a ref. Which component would it live in? The long press state needs to be coordinated across thoughts.

To add to the confusion, we also have https://github.com/cybersemics/em/blob/c7d9cde5095e1866525ae9fd372a694242568db2/src/stores/longPressStore.ts. Though according to your comment, it may no longer be needed (?). (Separate PR to remove longPressStore...?)

That's what I mean, you can use beginDrag to hook into TouchBackendImpl's setTimeout. Is there a different event from react-dnd that we need to hook into?

I was just saying that one of our options is to replace the timeout entirely so that we have more control over the internal timing of when react-dnd begins a drag, rather than trying to match up the internal timeout in TouchBackendImpl with our own.

I haven't mapped out exactly how that would work yet, which is why I haven't been more specific. Something like

longPressStore.subscribe(this.handleTopMoveStart.bind(this, e as any))

in a forked copy of TouchBackendImpl. Whether that's exactly right or not is less important than the decision:

1. tweak the timeout on TIMEOUT_LONG_PRESS_THOUGHT, which is easy (assuming it works) but remains a timing-based solution, or
2. move away from a timing-based solution and allow useDragHold to initiate a drag when it chooses

I think that sounds good, and it sounds like it will work. I could also see moving to a single variable that would be a sort of enum for the different state that it's in, like LongPressState.DRAG_HOLD, LongPressState.DRAG_IN_PROGRESS, LongPressState.DRAG_CANCELLED, etc. Functionally, it wouldn't matter one way or the other.

Yes! I like that. It would enforce mutual exclusivity. (We use PascalCase for enum values.)

OK! That seems like a good change to make in a new PR.

It looks like globals.longpressing is only checked in useLongPress, and it doesn't necessarily need to be a global variable. It could pull a variable straight out of the Redux state...

This might just be semantics, but the Redux state is global. It just forces updates to occur in the order they are dispatched, helping with race conditions.

Sure, maybe by "global" I really meant "unrestricted", since as we go into below, its value updates immediately and is not affected by React or Redux lifecycles.

...with the only caveat being that it might change in the time it takes for the 10ms setTimeout to run...

Yeah, this is a concern.

...so maybe it would need to be stored in a ref instead.

I'm not sure about a ref. Which component would it live in? The long press state needs to be coordinated across thoughts.

Basically it would look something like:

const longPressing = useSelector(state => state.longPressing) // or whatever
const longPressingRef = useRef(longPressing)

useEffect(() => longPressingRef.current = longPressing, [longPressing])

which is not my favorite pattern but it happens in plenty of other places.

To add to the confusion, we also have https://github.com/cybersemics/em/blob/c7d9cde5095e1866525ae9fd372a694242568db2/src/stores/longPressStore.ts. Though according to your comment, it may no longer be needed (?). (Separate PR to remove longPressStore...?)

longPressStore seems like it might be a little redundant, but it would in fact be useful for explicitly triggering drag-and-drop as I described above. My comment was pointing out that longPressStore.unlock() is already called in enough other places that I'm not sure it really needs to be called in useDragHold as well. Which, yeah, could be a separate PR.

...with the only caveat being that it might change in the time it takes for the 10ms setTimeout to run...

Yeah, this is a concern.

...so maybe it would need to be stored in a ref instead.

I'm not sure about a ref. Which component would it live in? The long press state needs to be coordinated across thoughts.

Basically it would look something like:

const longPressing = useSelector(state => state.longPressing) // or whatever
const longPressingRef = useRef(longPressing)

useEffect(() => longPressingRef.current = longPressing, [longPressing])

which is not my favorite pattern but it happens in plenty of other places.

Can you clarify. Is this a single ref for all thoughts, or a separate ref for each thought?

Can you clarify. Is this a single ref for all thoughts, or a separate ref for each thought?

I think a separate ref for each thought, since I would just put it into useLongPress. If it's preferable to have a single one, then that could be arranged.

Can you clarify. Is this a single ref for all thoughts, or a separate ref for each thought?

I think a separate ref for each thought, since I would just put it into useLongPress. If it's preferable to have a single one, then that could be arranged.

Got it, thanks.

If all of the longpressing behavior were isolated to a single thought, I would be okay with a ref. For coordinating across thoughts, I would prefer to use the Redux store.

state.dragHold, longPressStore, and globals.longpressing are all global currently. Moving from global state to per-thought state seems like a very big change.

Basically it would look something like:

const longPressing = useSelector(state => state.longPressing) // or whatever
const longPressingRef = useRef(longPressing)

useEffect(() => longPressingRef.current = longPressing, [longPressing])

which is not my favorite pattern but it happens in plenty of other places.

I'm not sure I understand the difference between the proposed state.longPressing and the current state.dragHold.

I don't see the point of storing something from the Redux state in a ref when the Redux state can be accessed directly, but maybe I'm missing something.

That makes sense. This seems relatively independent from other changes. Could we make this change in a separate PR? Cleaving off functionality on a complex issue like this will simplify the review process and provide a more granular commit history to track regressions.

I can start a new one. It sounds like we are also going to revert the other changes in here and add some sort of dragCancelled variable that would also be checked in shouldCancelGesture. I'll get a new PR started.

Thanks, that will help reduce the scope and make it easier for me to evaluate the risk compared to solving #3072 and #3073 and #3074 all at once.

That's what I mean, you can use beginDrag to hook into TouchBackendImpl's setTimeout. Is there a different event from react-dnd that we need to hook into?

I was just saying that one of our options is to replace the timeout entirely so that we have more control over the internal timing of when react-dnd begins a drag, rather than trying to match up the internal timeout in TouchBackendImpl with our own.

I generally agree with the need to synchronize the long press with react-dnd instead of maintaining a separate timeout in useLongPress.

I haven't mapped out exactly how that would work yet, which is why I haven't been more specific. Something like

longPressStore.subscribe(this.handleTopMoveStart.bind(this, e as any))

in a forked copy of TouchBackendImpl.

I'm hesitant to fork due to the maintenance costs that will potentially be incurred over time.

I know we're trying to move away from the timeout approach, but what if we could guarantee that our setTimeout began in the exact same frame and use the exact same duration as in TouchBackendImpl?

This looks promising from my testing, and does not require a fork:

  const dragDropManager = useDragDropManager()

  useEffect(() => {
    const backend = dragDropManager.getBackend() as any
    const onStart = () => {
      setTimeout(() => {
        console.log('handleTopMoveStartDelay should be called now')
      }, TIMEOUT_LONG_PRESS_THOUGHT)
    }
    backend.addEventListener(backend.options.rootElement, 'start', onStart)
    return () => {
      backend.removeEventListener(backend.options.rootElement, 'start', onStart)
    }
  }, [dragDropManager])

Reference: https://github.com/react-dnd/react-dnd/blob/1dd1bcdd39d9545fdbb319e34ddbeecf0fb9d89c/packages/backend-touch/src/TouchBackendImpl.ts#L131

I don't see the point of storing something from the Redux state in a ref when the Redux state can be accessed directly, but maybe I'm missing something.

The only thing is that variables pulled out of the state with useSelector won't update during the 10ms setTimeout, so what I was proposing is simply to copy the Redux state variable into a ref (per-thought) so that it can potentially be updated while the timeout is running. I wasn't proposing using the ref for anything more than that.

Sorry, which setTimeout?

Sorry, which setTimeout?

This one, a secret third setTimeout that has nothing to do with the other ones we've been discussing 😄

That's the only place that checks globals.longpressing, and so the idea of using a ref was geared toward making sure that behavior didn't change at all.

Sorry, which setTimeout?

This one, a secret third setTimeout that has nothing to do with the other ones we've been discussing 😄

Omg 😱

That's the only place that checks globals.longpressing, and so the idea of using a ref was geared toward making sure that behavior didn't change at all.

In that case, maybe we could access store.getState() directly since it is event-based and does not need to be reactive? I'm not loving the idea of the ref.

Sorry, which setTimeout?

This one, a secret third setTimeout that has nothing to do with the other ones we've been discussing 😄

Omg 😱

That's the only place that checks globals.longpressing, and so the idea of using a ref was geared toward making sure that behavior didn't change at all.

In that case, maybe we could access store.getState() directly since it is event-based and does not need to be reactive? I'm not loving the idea of the ref.

Yeah, that's a good idea.

I know we're trying to move away from the timeout approach, but what if we could guarantee that our setTimeout began in the exact same frame and use the exact same duration as in TouchBackendImpl?

This looks promising from my testing, and does not require a fork:

I added that snippet to useLongPress along with a timestamp, and I added a similar log statement with timestamp in here and I saw the following:

timerIdRef should be called now (1751911062334)
handleTopMoveStartDelay should be called now (1751911062373)

This looks promising because that ~40ms discrepancy would explain why there's a narrow window where dragHold can be active before drag-and-drop takes over. It should be possible to identify and eliminate the discrepancy by figuring out where that extra 40ms comes from.

If we want to, it should also be possible to move the start method from useLongPress inside the event handler (backend.addEventListener(backend.options.rootElement, 'start', onStart)) and essentially let react-dnd decide when a long press should begin. That's kind of the inverse of my idea, where our long press code would trigger react-dnd. If we're moving in that direction anyway, it may not be necessary to identify the source of the discrepancy.

If we want to, it should also be possible to move the start method from useLongPress inside the event handler (backend.addEventListener(backend.options.rootElement, 'start', onStart)) and essentially let react-dnd decide when a long press should begin. That's kind of the inverse of my idea, where our long press code would trigger react-dnd. If we're moving in that direction anyway, it may not be necessary to identify the source of the discrepancy.

Yes, this is what I'm suggesting. I think it would be less maintenance than a fork.

If you are able to identify the (deterministic) cause of the 40ms discrepancy, then we could avoid tightly coupling to TouchBackendImpl. I wouldn't spend too much time on this unless you have a good lead though.