Move setCaretOffset into onMouseDown and preventAutoscrollEnd back into onFocus#4371
Conversation
… back into onFocus" This reverts commit 2a8082b.
|
@ethan-james is attempting to deploy a commit to the Cybersemics Institute Team on Vercel. A member of the Team first needs to authorize it. |
This reverts commit 47e9eab.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Thank you! The code looks fine.
Follow-Up (speculative)
If we want, we can pursue the possibility of moving all
selection.setbehavior into theshouldSetSelectioneffect rather than maintaining separateselection.setbehavior insetCaretOffset. The flow would look like:
setCaretOffsetupdatescaretOffsetin the Redux store- the
shouldSetSelectioneffect evaluates its conditions and determines whether to actpreventAutoscrollandselection.setare called in one declarative place
That makes sense to me, and more closely matches the earlier declarative design. We can try this in a separate PR unless you think otherwise.
Test Coverage (unsuccessful)
I tried to add a Browserstack iOS test, but I was never able to get it to fail on main (#4403)
If manual testing doesn't turn up anything, I'm okay merging this PR and then working on an automated test afterwards. Since browser selection behavior is critical, fragile, and high risk, we will want to add test coverage.
@BayuAri This is a high risk change that requires extensive testing of the browser selection across all platforms. Any of the test cases from #3374 (comment) are fair game, but use your best judgment as you decide what to test. I'll let you manually test first before me. Thank you!
🎉
Yes, I think exploring it in a separate PR makes sense.
Sounds good. It would be good to explore whether |
|
Original issue is fixed. Issue A: Toolbar is jumping down and up when user moves the caret between ThoughtsStep to Reproduce
Current behaviorThe toolbar is jumping down and up. As reference, the behavior on
PR/4371 Issue.A.Toolbar.is.jumping.down.and.up.when.user.moves.the.caret.between.Thoughts.MP4main Behavior.of.the.toolbar.on.main.MP4Expected behaviorToolbar should not jump when user moves caret between thoughts Side note: a blank area under the Thought World on the
|
There was a problem hiding this comment.
I found no more issue on this PR aside from Issue A: Toolbar is jumping down and up when user moves the caret between Thoughts
I logged 2 related issue uncovered with this PR tested as follow
#4408
#4426
@ethan-james What's your assessment of this issue? My impression from Bayu is that it was preexisting but exacerbated in this PR. |
I hadn't really noticed it on the iPhone 12 Pro, although it's not too hard to reproduce. It is much more dramatic on iPhone 16 Pro Max. I zeroed in on a test case where I toggle between Main: Screen.Recording.2026-06-23.at.10.04.56.AM.movScreen.Recording.2026-06-23.at.10.03.44.AM.movI broadened my tests to see if there were more cases of the toolbar jumping in the branch than there were on main, but my assessment is that they're pretty similar, at least on the iPhone 16 Pro Max simulator and the iPhone 12 Pro device. |
|
Thanks. Looks like a preventAutoscroll issue. I'm satisfied merging this and then addressing the scroll issue separately. It might even be solved by #4435. |
Yes, the test case looks perfect on the |
Bring the caretFocus.ts spec in line with the reproduction branch: the finger-sized right-edge tap on a non-cursor thought, with the detailed root-cause explanation (Safari touch-adjustment retargeting + the stale offsetRef onMouseUp focus path that cybersemics#4371 removed). On this fixed branch focus is prevented, so the spec asserts the keyboard stays down. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>


Fixes #4298
Overview
Triggering
preventAutoscrollEndinonMouseUpwas problematic becausemouseupisn't always dispatched, and when it is dispatched, it doesn't necessarily mean that the editable was focused. MovingpreventAutoscrollEndback intoonFocusmeans thatsetCaretOffsetneeds to fire earlier, such as inonMouseDownwhere it spent most of its time during development of #3410.Complications (solved)
setCaretOffsetis calledonMouseDown.Solution: In order to preserve native behavior, it is necessary to keep track of whether a press has ended (
touchendhas been dispatched) or whether it is ongoing in order to short-circuitonMouseDown.setCaretOffsetupdates the Redux state in a way that triggers theshouldSetSelectioneffect, which creates duplicate invocations ofpreventAutoscrollandselection.set().Solution: Call
allowDefaultSelectionat the same time assetCaretOffsetto preventshouldSetSelectionfrom being true.Follow-Up (speculative)
If we want, we can pursue the possibility of moving all
selection.setbehavior into theshouldSetSelectioneffect rather than maintaining separateselection.setbehavior insetCaretOffset. The flow would look like:setCaretOffsetupdatescaretOffsetin the Redux storeshouldSetSelectioneffect evaluates its conditions and determines whether to actpreventAutoscrollandselection.setare called in one declarative placeConfidence Level
low-to-medium pending further testing
I verified that deferring
selection.setuntil the effect runs seemed to work well enough when tapping between thoughts, although triggering theselection.setinsetCaretOffsetis still necessary when tapping within the active thought because theshouldSetSelectioneffect isn't currently triggered when only thestate.cursorOffsetvalue updates.I didn't test any of the commands that rely on
shouldSetSelection, and I am somewhat confident that the flicker/lag can be avoided by either usinguseEffectas it is currently structured, or by changing touseLayoutEffect, which introduces additional uncertainty regarding all of those commands that need to be tested.Pros
selection.setbehavior would makeuseEditModeeasier to reason aboutallowDefaultSelectionentirely (RemoveallowDefaultSelection#4179)Cons
selection.setinsetCaretOffsetguarantees that the caret position with minimal lag/flicker, which may be more difficult to guarantee if it is offloaded onto the React render cycleTest Coverage (unsuccessful)
I tried to add a Browserstack iOS test, but I was never able to get it to fail on main (#4403)