Repository navigation
Put the caret where a tap landed when iOS reverts it after the touchend #6002
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
8435ba0
eba2990
f53f344
3c08c7d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| import { useEffect } from 'react' | ||
| import { useDispatch } from 'react-redux' | ||
| import Path from '../../@types/Path' | ||
| import { setCursorActionCreator as setCursor } from '../../actions/setCursor' | ||
| import { isSafari, isTouch } from '../../browser' | ||
| import getCaretOffset from '../../device/getCaretOffset' | ||
| import * as selection from '../../device/selection' | ||
| import isMultiEditing from '../../selectors/isMultiEditing' | ||
| import isMulticursorPath from '../../selectors/isMulticursorPath' | ||
| import multitouchStore from '../../stores/multitouchStore' | ||
|
|
||
| /** | ||
| * Puts the caret where a tap landed when iOS moves it back. A tap a few characters from the caret gets no mouse | ||
| * events, so onMouseDown never places the caret, and after the touchend iOS moves the caret to the tapped word and | ||
| * then, within about 70ms, back to where it was before the tap (#4220). | ||
| */ | ||
| const useRevertCorrection = ({ | ||
| editableRef, | ||
| enabled, | ||
| path, | ||
| }: { | ||
| editableRef: React.RefObject<HTMLElement | null> | ||
| /** Correct only while this editable is the one being edited, i.e. the cursor thought with the keyboard open. */ | ||
| enabled: boolean | ||
| /** The thought whose cursor offset follows the corrected caret. */ | ||
| path: Path | ||
| }) => { | ||
| const dispatch = useDispatch() | ||
|
|
||
| useEffect(() => { | ||
| const editable = editableRef.current | ||
| if (!editable || !enabled || !isTouch || !isSafari()) return | ||
|
|
||
| // The last tap on the editable: where it landed, the caret offset at its touchend, and whether the caret has left | ||
| // that offset since. It lasts until the next input of any kind, since the revert arrives after the touchend. | ||
| let watch: { clientX: number; clientY: number; before: number; moved: boolean } | null = null | ||
|
|
||
| /** Starts watching a tap. */ | ||
| const onTouchEnd = (e: TouchEvent) => { | ||
| const touch = e.changedTouches[0] | ||
| // iOS moves the caret for a tap only after its touchend, so this is still the offset from before the tap. A drag | ||
| // or long press has already moved the caret by now, and nothing moves it back. | ||
| const before = selection.isCollapsed() ? selection.offsetFromNode(editable) : null | ||
| watch = | ||
| touch && before !== null && e.touches.length === 0 && !multitouchStore.getState() | ||
| ? { clientX: touch.clientX, clientY: touch.clientY, before, moved: false } | ||
| : null | ||
| } | ||
|
|
||
| /** Ends the watch on any other input. A mousedown means the tap is one that onMouseDown places itself. */ | ||
| const endWatch = () => (watch = null) | ||
|
|
||
| /** Notes a move away from the offset before the tap, and corrects a return to it after one. */ | ||
| const onSelectionChange = () => { | ||
| if (!watch || document.activeElement !== editable || !selection.isCollapsed()) return | ||
| const offset = selection.offsetFromNode(editable) | ||
| if (offset === null) return | ||
| if (offset !== watch.before) { | ||
| watch = { ...watch, moved: true } | ||
| return | ||
| } | ||
| if (!watch.moved) return | ||
|
|
||
| const { clientX, clientY, before } = watch | ||
| watch = null | ||
| const tapped = getCaretOffset(editable, { clientX, clientY }).offset | ||
| // a tap on the caret | ||
| if (tapped === null || tapped === before) return | ||
|
|
||
| // iOS is still handling the tap when it fires this selectionchange. | ||
| requestAnimationFrame(() => { | ||
| selection.set(editable, { offset: tapped }) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This deviates from the declarative design of Is there something fundamentally different about It may be cleaner the way you have it, but since it does seem to be a departure from our earlier architecture, along with
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The detection can't fold into the declarative effect, because it reacts to iOS changing the selection rather than to app state, so a The blocker is that the selection effect deliberately doesn't re-run when only So I'm looking at keeping the detection in the hook and routing the write through Generated by Claude Code
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like a new Anything decided here should be extended to cover For now, I think I'll drop the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for working on this! Sorry for the long + granular replies. There are a lot of different aspects to this.
Okay, understood. The However, we often use ministores to subscribe to realtime changes outside the Redux path. Not sure if that would be helpful in this case.
Yes, some shared logic would be an improvement.
I like this idea. But why use a nonce rather than a proper representation of the browser selection? A nonce is a course tool that says something needs updating without saying why. A selection representation (effectively a reactive representation of
Yes, exactly. Look for opportunities for a more unified architecture, as the selection handling has become more fragmented over time.
It was unfortunate when these two started to deviate.
That's a legitimate concern. The situation is very different when overriding touch events. Now we're not just setting the selection from an idle state, we are actively competing with native browser events. Redux updates themselves are synchronous and can be guaranteed to make changes before the next paint using |
||
| dispatch((dispatch, getState) => { | ||
| const state = getState() | ||
| dispatch( | ||
| setCursor({ | ||
| path, | ||
| offset: tapped, | ||
| preserveMulticursor: isMultiEditing(state) && isMulticursorPath(state, path), | ||
| }), | ||
| ) | ||
| }) | ||
| }) | ||
| } | ||
|
|
||
| editable.addEventListener('touchend', onTouchEnd) | ||
| // Capture, so that a handler that stops propagation cannot keep the watch open. | ||
| document.addEventListener('touchstart', endWatch, { capture: true, passive: true }) | ||
| document.addEventListener('mousedown', endWatch, { capture: true }) | ||
| document.addEventListener('keydown', endWatch, { capture: true }) | ||
| document.addEventListener('beforeinput', endWatch, { capture: true }) | ||
| document.addEventListener('selectionchange', onSelectionChange) | ||
|
|
||
| return () => { | ||
| editable.removeEventListener('touchend', onTouchEnd) | ||
| document.removeEventListener('touchstart', endWatch, { capture: true }) | ||
| document.removeEventListener('mousedown', endWatch, { capture: true }) | ||
| document.removeEventListener('keydown', endWatch, { capture: true }) | ||
| document.removeEventListener('beforeinput', endWatch, { capture: true }) | ||
| document.removeEventListener('selectionchange', onSelectionChange) | ||
| } | ||
| }, [dispatch, editableRef, enabled, path]) | ||
| } | ||
|
|
||
| export default useRevertCorrection | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| import getCaretOffset from './getCaretOffset' | ||
|
|
||
| /** | ||
| * Waits for the caret to come to rest at the given offset in the thought being edited. A caret that iOS places at a | ||
| * tap can be moved back within about 70ms, so the offset must match on two samples taken further apart than that. | ||
| * Uses the global browser object from WDIO. | ||
| */ | ||
| const waitForCaretOffset = async (offset: number): Promise<void> => { | ||
| let matchedPreviousSample = false | ||
| try { | ||
| await browser.waitUntil( | ||
| async () => { | ||
| const matched = (await getCaretOffset()) === offset | ||
| const settled = matched && matchedPreviousSample | ||
| matchedPreviousSample = matched | ||
| return settled | ||
| }, | ||
| { timeout: 5000, interval: 250 }, | ||
| ) | ||
| } catch { | ||
| throw new Error(`Expected the caret to come to rest at offset ${offset}, but it was at ${await getCaretOffset()}.`) | ||
| } | ||
| } | ||
|
|
||
| export default waitForCaretOffset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why
requestAnimationFrame? I’m not sure there is any guarantee that the browser selection lines up with the frame rate.Is there something more salient than the next frame that you could wait for? Or is it a magic number that might be sensitive to cpu load?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that I managed to get rid of it now that this solution is specific to #4220 and doesn't also try to fix #5981.