Call setCursor onTouchEnd when moving between thoughts

[ethan-james]

Fixes #4173

Rapid taps between thoughts trigger the same type of focus-retargeting on iOS Safari that was implicated in #4291 and #4394. The mouse events arrive on the wrong element, so all of the cursor-management logic sets focus on the wrong element. The only events that arrive on the correct editable are touch events, so it is necessary to manage the cursor from there.

It is also necessary to detect and block mouse events that land on the wrong editable. This is accomplished by storing the last element to receive a touch event, and then calling preventDefault in onMouseDown in order to stop focus on the re-targeted editable.

Complication

It was necessary to avoid calling selection.set inside onTouchEnd, as that would swallow subsequent rapid taps entirely. This moves us a bit closer to the future imagined in #4470.

Lack of test coverage

Copilot was unable to reproduce the bug in BrowserStack. Efforts are documented in #4538. The taps were confirmed to be landing in the correct places, but latency between taps was pretty high, above 750ms. When I instrumented the iPhone and reproduced the bug manually, taps were closer to 250ms apart.

Copilot was able to get the taps closer together by combining them in a single performActions call, but it didn't help to reproduce the bug.

Further thoughts

1. I gated the onTouchEnd event so that it would only fire when a tap fit the profile for being re-targeted. I'm not sure that changed the behavior, but it felt safer and it eliminates duplicate calls to setCursor in some cases.

2. This logic is only invoked when state.isKeyboardOpen is true, plus it benefits from being able to call getCaretOffset to correctly set the caret offset. For those reasons, I wanted it inside of useEditMode even though there are some duplicate checks in handleTapBehavior in Editable.

This does mean that there are touchend handlers in useEditMode as well as in Editable, and both of them will run and both can call preventDefault. This is perhaps not ideal.

3. The module-level variables lastTouchEndTime and lastTouchEndTarget are necessary to allow onMouseDown to short-circuit when the re-targeted mouse event hits the wrong editable. It may be a better idea to migrate in the direction of handling all caret-setting behavior in onTouchEnd on iOS, although it would still be necessary to gate mousedown in order to prevent re-targeted focus pulling the caret back to the wrong thought.

[github-actions]

Vercel preview: https://em-2n09x61xv-cybersemics.vercel.app

[raineorshine]

Great work! Good explanation of the issue, and it seems like the retargeted tap detection and move condition are narrow enough that the chance of this impacting other behavior is low.

---

Copilot was unable to reproduce the bug in BrowserStack. Efforts are documented in #4538. The taps were confirmed to be landing in the correct places, but latency between taps was pretty high, above 750ms. When I instrumented the iPhone and reproduced the bug manually, taps were closer to 250ms apart.

Copilot was able to get the taps closer together by combining them in a single performActions call, but it didn't help to reproduce the bug.

That's fine, thanks for trying.

FYI I would be wary of adding any automated test that depends on timing. The cloud test runner is executed on shared compute that can easily be paused and resumed without our control. If we had a subset of tests that we ran only on a dedicated machine this might be possible, but at the moment it is out of reach. It's the same reason we don't have automated performance tests or animation tests.

1. I gated the onTouchEnd event so that it would only fire when a tap fit the profile for being re-targeted. I'm not sure that changed the behavior, but it felt safer and it eliminates duplicate calls to setCursor in some cases.

I like it.

This does mean that there are touchend handlers in useEditMode as well as in Editable, and both of them will run and both can call preventDefault. This is perhaps not ideal.

3. The module-level variables lastTouchEndTime and lastTouchEndTarget are necessary to allow onMouseDown to short-circuit when the re-targeted mouse event hits the wrong editable. It may be a better idea to migrate in the direction of handling all caret-setting behavior in onTouchEnd on iOS, although it would still be necessary to gate mousedown in order to prevent re-targeted focus pulling the caret back to the wrong thought.

Thanks for calling out these architecture concerns. Let's keep these and #4470 in mind for a possible refactor in the future. It's not easy work, but if we can pull it off it would reduce some code complexity.

[raineorshine]

store is typically not accessed directly. The conventional approach is to dispatch a thunk to get fresh state without having to import the app store or add it to the hook dependency list:

dispatch((dispatch, getState) => {
  const state = store.getState()
  ...
})

[ethan-james]

Oops, that's right. Should I make a note of that somewhere so that Copilot picks up on it next time?

[raineorshine]

Thanks! I will add this to .github/instructions/code-standards.instructions.md

#4621

[raineorshine]

@BayuAri I've tested this and confirmed that it works on mobile Safari.

This should be a Mobile Safari only bug, but could you test the behavior on Android just in case? Thanks!

[BayuAri]

@raineorshine
Issue is not reproducible on Android and Mobile Chrome as well as iOS Capacitor