More robust autoscroll and autoscroll prevention#4435
Conversation
…oggle (#3765) Introduces a second technique for suppressing iOS WebKit's native autoscroll on focus, which is the cause of `position: fixed` elements (toolbar) jumping when the user taps a thought far from the current cursor. v1 (existing preventAutoscroll) tries to neutralize autoscroll by transforming the target. v2 (focusWithoutAutoscroll) takes ownership of focus instead: preventDefault on mousedown to block the native focus + caret-from-tap, then el.focus({ preventScroll: true }) to focus programmatically without scroll. In useEditMode mousedown the v2 ordering is: dispatch setCursor → focus → selection.set. Dispatching the cursor before focus() makes Editable.onFocus's setCursorOnThought no-op via its equalPath early return, which avoids a race that previously clobbered cursorOffset to 0. The dispatch carries isKeyboardOpen: true so Redux stays in sync with the visible keyboard. A small floating overlay (DebugAutoscrollToggle) lets the technique be flipped at runtime via localStorage and exposes an in-app log panel for on-device debugging, since console output isn't reachable in iOS Capacitor. The overlay and instrumentation are temporary; remove once the A/B is decided.
Checkpoint commit before merging issue-3596 (virtualKeyboardStore) so the in-flight work isn't sitting in the working tree during the merge. - focusWithoutAutoscroll is now the single chokepoint for v2 cursor changes (tap, Return, arrow keys, gestures, sidebar restore). Suppresses both the focus-driven and selection-driven autoscroll on iOS. - useEditMode mousedown dispatches setCursor before focus so Editable.onFocus early-returns instead of clobbering cursorOffset to 0. - Empty-thought tap fix: getCaretOffset returns null on empty thoughts; the v2 mousedown branch now falls back to offset 0 so the second tap still focuses and opens the keyboard. - Debug log scroll instrumentation patches focus/scrollIntoView/scrollTo/By and clusters scroll bursts to spot native autoscroll sources.
…ug tooling The A/B is decided in favor of v2 (focusWithoutAutoscroll): - Remove v1 preventAutoscroll and its call sites (Editable, VirtualThought). - Remove the A/B debug tooling (DebugAutoscrollToggle, autoscrollTechnique, debugAutoscrollLog). - Extract the trigger-edge computation into autoscrollEdges, keyboard-aware via virtualKeyboardStore. - Rewrite scrollCursorIntoView around the trigger edges, reading the cursor's target position from its offsetParent so it is not stale during layout animations. - asyncFocus focuses its hidden input with preventScroll: true so priming cannot itself scroll the page.
The #3765 chokepoint made onMouseDown preventDefault unconditional, which suppressed native drag-select, double-click-word, and the right-click context menu on desktop. Restore main's inVoidArea escape hatch: preventDefault only fires in a void area on non-iOS platforms, while iOS Safari still always blocks so the focusWithoutAutoscroll chokepoint can suppress the position:fixed jolt.
Pressing Enter at the bottom of the screen should trigger a small scroll on every keystroke, as iOS native autoscroll (and main) does. The rhythm holds iff the landing margin is smaller than one line step; the fixed fontSize*2 margin plus the fontSize*2 trigger buffer could equal or exceed it, absorbing Enters and then scrolling too far. - scrollMargin: fontSize*2 -> height/2, so the cursor always lands half a line inside the edge and the next Enter re-triggers. Also scales the landing position with multi-line thoughts, matching the dynamic feel of iOS native. - bottomEdge buffer: 0 while the keyboard is open. The keyboard inset already keeps the cursor above the keyboard; an additional inset trigger edge on the small visible strip made scrolls fire early and overshoot. Top edge buffer unchanged.
… and how asyncFocus primes it
…e thought The fontSize*2 inset on the top edge fired while the cursor was still fully visible and, stacked with the landing margin, scrolled it ~2.5 lines below the toolbar — noticeably more than the bottom edge's one-line-per-Enter rhythm. Trigger at the toolbar itself and let the height/2 landing margin provide the headroom, matching main's reveal-one-thought behavior at the top.
…Store visualViewport.height never shrinks in the native app (Capacitor Keyboard resize: 'none'), so the smooth-vs-instant threshold was a full screenful there instead of the visible area above the keyboard. Read the keyboard height from virtualKeyboardStore instead — the same source autoscrollEdges uses — so the whole autoscroll flow shares one source of keyboard truth. Also clarify comments for fresh readers (name the TreeNodePositioner transition and autocrop explicitly) and rename locals to match the trigger-edge model: scrollMargin -> landingMargin, isAboveViewport/isBelowViewport -> isAboveTopEdge/isBelowBottomEdge.
…margin terminology
Trim insider phrasing ("chokepoint") and over-detailed rationale, and name
the two distances consistently across files: the trigger buffer decides
when a scroll starts (autoscrollEdges); the landing margin decides where
the cursor settles (scrollCursorIntoView).
…iHandler owns keyboard-height estimate
Move per-orientation keyboard-height estimation out of viewport.ts and into
iOSSafariHandler, its only consumer. viewport.ts no longer tracks keyboard
height at all. Also removes two dead code paths:
- storageModel virtualKeyboardHeight{Landscape,Portrait} fields (never read/written)
- iOSCapacitorHandler virtualKeyboardHeight write (orphaned after preventAutoscroll
was removed earlier on this branch)
Behavior is unchanged: the same measurement (innerHeight - visualViewport.height),
per-orientation cache, and geometric fallback now live in the handler.
…y owns autoscroll
topEdge was flush with the toolbar, so the cursor had to pass fully under the toolbar before an upward scroll fired — which made cursorUp/arrow-up navigation feel sticky. Add a fontSize*2 top buffer mirroring bottomBuffer so the scroll fires ~one thought early, the same rhythm as the bottom edge. Experimental (#3765): topBuffer is a single named constant; set it to 0 to restore the previous flush-to-toolbar behavior if the upward scroll over-reveals. Exposed on the AutoscrollEdges interface alongside bottomBuffer.
There was a problem hiding this comment.
Pull request overview
This PR addresses iOS (Capacitor + Safari) native autoscroll side effects (fixed toolbars/navbars “jumping” and caret being covered by the keyboard) by preventing native autoscroll during focus/selection and moving all cursor reveal logic into JavaScript-driven autoscroll with a defined “comfort zone”.
Changes:
- Introduces
focusWithoutAutoscrollto focus + set selection without triggering iOS native autoscroll. - Refactors cursor autoscroll into a comfort-zone model (
autoscrollEdges+ updatedscrollCursorIntoView) and reacts to virtual keyboard target height changes. - Updates virtual keyboard handling to publish
targetHeight(one-shot per keyboard movement) and removes the oldviewportStore.virtualKeyboardHeightapproach (and deletespreventAutoscroll).
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/stores/virtualKeyboardStore.ts | Adds targetHeight to virtual keyboard ministore state. |
| src/stores/viewport.ts | Removes virtual-keyboard height estimation/caching from viewport store. |
| src/hooks/useScrollCursorIntoView.ts | Subscribes to keyboard targetHeight changes to re-run cursor reveal when the keyboard moves. |
| src/device/virtual-keyboard/handlers/iOSSafariHandler.ts | Adds per-open targetHeight publishing and prediction logic for Safari. |
| src/device/virtual-keyboard/handlers/iOSCapacitorHandler.ts | Publishes targetHeight from native events immediately; adjusts close semantics. |
| src/device/scrollCursorIntoView.ts | Implements comfort-zone-based autoscroll and smooth/instant scrolling heuristics. |
| src/device/preventAutoscroll.ts | Deletes prior CSS-based native autoscroll mitigation implementation. |
| src/device/focusWithoutAutoscroll.ts | New module: focus + caret placement without native autoscroll; restores scrollY after selection changes. |
| src/device/autoscrollEdges.ts | New module: computes autoscroll trigger edges (“comfort zone”) from DOM + stores. |
| src/device/asyncFocus.ts | Adds preventScroll: true when focusing hidden input to avoid a scroll-to-top flicker on iOS. |
| src/components/VirtualThought.tsx | Removes preventAutoscroll-specific height update skip. |
| src/components/Note.tsx | Switches note focus/selection to focusWithoutAutoscroll and manual caret offset on mouse down. |
| src/components/Editable/useEditMode.ts | Routes cursor placement and tap-to-caret through focusWithoutAutoscroll; removes preventAutoscroll wiring. |
| src/components/Editable.tsx | Minor formatting-only brace expansion around suppressFocusStore guard. |
| src/@types/VirtualKeyboardState.ts | Documents and adds targetHeight alongside spring-animated height. |
| predictedFinalHeight = Math.max(targetHeight, predictedFinalHeight ?? targetHeight) | ||
| virtualKeyboardStore.update({ open: true, targetHeight: predictedFinalHeight }) |
There was a problem hiding this comment.
@copilot Agreed, but we should probably use the ScreenOrientation API here instead of inferring the screen orientation using window.innerHeight > window.innerWidth.
There was a problem hiding this comment.
Updated in aeb9dd5: orientation is now read from the ScreenOrientation API (screen.orientation.type) instead of inferring from viewport dimensions, with a media-query fallback when unavailable.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: fbmcipher <16063438+fbmcipher@users.noreply.github.com>
Co-authored-by: fbmcipher <16063438+fbmcipher@users.noreply.github.com>
# Conflicts: # src/components/Editable/useEditMode.ts
Note.onMouseDown called e.preventDefault() unconditionally, which blocked native double-click word selection on desktop (the copy-from-note paste flow broke). Gate preventDefault to iOS Safari / void-area taps, mirroring Editable.onMouseDown, so desktop native selection is preserved while the iOS autoscroll suppression still routes through focusWithoutAutoscroll.
# Conflicts: # src/components/VirtualThought.tsx # src/device/preventAutoscroll.ts
…veal The top trigger buffer (fontSize * 2) fires as soon as the cursor enters the buffer band below the toolbar, even when the document is already scrolled to the top and there's nothing above to reveal. scrollYNew then computes an unreachable negative target, and Math.max(1, scrollYNew) manufactured a 1px scroll out of nothing — shifting the entire page and breaking every puppeteer snapshot with a near-top cursor at font sizes where the buffer reaches the cursor (22, 28). Confirmed via bisect: window.scrollY flipped from 0 to 1 at exactly this trigger, isolated by diffing raw screenshots between the commit that introduced the buffer and its parent.
…fer/margin roles Correctness (behavior-neutral): replace the scrollY===0 special case with a general reachability clamp. The scroll target is clamped to the range the page can actually reach (0..maxScroll); if that lands on the current position there is nowhere to go, so we bail before the Safari floor-to-1 turns a no-op into a phantom 1px scroll. This covers the bottom edge's latent short-document case for the same price, and expresses reachability in the logic rather than as a corner-case guard bolted on afterward. Clarity: rename isAboveTopEdge/isBelowBottomEdge -> crossedTopEdge/ crossedBottomEdge (they mean "crossed the trigger line", not "is occluded"), and document the deliberate coupling: because the landing is measured from the edge, the trigger buffer sets both when a scroll fires and how deep the cursor parks (buffer + landingMargin inside the visible area). No behavior change — all five scroll-sensitive snapshot tests still pass.
…e scroll position The color-theme "colored and highlighted text" baseline was baked on main at window.scrollY=1. Instrumenting main's scrollCursorIntoView shows that 1 is not an intended scroll target: with the colored test's color-picker-expanded toolbar (toolbarBottom≈138) the cursor is technically occluded, so main computes a corrective scrollYNew of -65 — an unreachable target (you can't scroll above the document top) — and Math.max(1, -65) floors it to 1. On that tall page (763px) the 1px artifact sticks; on the shorter superscript/subthought pages the same Math.max floor evaluates to 1 but never takes, so those baselines are at 0. The branch's scrollCursorIntoView correctly resolves the unreachable scroll to "best reachable target is 0, already there, no-op" and stays at 0 in every context (verified deterministic, alone and combined). Regenerating this one baseline aligns it with the correct phantom-free behavior; all other snapshots already matched.
|
|
setCursorOnThought (Editable.onFocus) unconditionally nulled the cursor offset. While editing, setCursor coerces a null offset to 0, so any programmatic refocus of the thought that already holds the cursor reset the caret to the start. This surfaced after merging siblings: the merge places the caret mid-thought, then focusWithoutAutoscroll's focus() fires onFocus -> setCursorOnThought, which nulled the offset -> coerced to 0 -> the caret effect yanked the caret back to 0. The deleteThought "after merging siblings, caret should be in between" unit test has been red on this branch since the focusWithoutAutoscroll adoption (real browsers happened to avoid it via keyboard-open timing, but jsdom does not). Fix: only default the offset to null (let the browser choose the position) when focusing a DIFFERENT thought — the genuine click/tap case this handler targets. When refocusing the thought that already holds the cursor, preserve the existing offset so a deliberately-placed caret survives. Verified: deleteThought passes, and caret/caret-multiline (incl. within-thought line navigation, which needs setCursorOnThought to keep running) stay green.
# Conflicts: # src/e2e/puppeteer/__tests__/drag-and-drop-multiselect.ts
|
@raineorshine Ready for review! Note: I regenerated one snapshot test ( This PR fixes the bug, so the snapshot test needed to be regenerated. |
raineorshine
left a comment
raineorshine
left a comment
There was a problem hiding this comment.
Hi, thanks so much for the PR. This will be a huge improvement!
focus({ preventScroll: true }) did not exist when em began, and it's about time we got a native way to prevent autoscroll.
I do have some concern about how much is done in one PR. This not only makes reviewing more challenging, but increases the likelihood of regressions. Can you explain why all of these changes must to be done together? For example, preventing native autoscroll is orthogonal to scrollCursorIntoView as far as I know. I would have assumed a good starting point would be to just sub out the outdated preventAutoscroll with native focus({ preventScroll: true }). If that was safe, then we could move on to other improvements.
The concept of a comfort zone and autoscrollEdges sound promising, but I would be more inclined to consider changes to scrollCursorIntoView in a dedicated PR with specific test cases that we are targeting.
Given that this PR makes deep changes to useEditMode, it means that all of the manual test cases from #3410 and #4371 are at risk. There are delicate timing issues related to pointer events, touch events, focus events, and offset calculations. All selection behavior will need to be re-tested.
@BayuAri We can cautiously proceed with manual testing, but if you encounter more than one selection-related regression, you should bail so that we can stop and see if there is a way to break this PR into smaller pieces. There is no point documenting a large number of test cases when there are potentially too many changes to safely make at once.
@fbmcipher Please hold off on the architectural feedback and internal changes until we demonstrate the behavioral viability.
Thanks!
There was a problem hiding this comment.
This should probably go in the selection module.
There was a problem hiding this comment.
With a PR as large as this, I would have considered preserving preventAutoscroll. Flip a flag so the next selection.set uses focusWithoutAutoscroll, and then flip it back on preventAutoscrollEnd. This would have preserved the existing architecture during the migration and reduced the size of the diff. Then removing preventAutoscroll completely would have been a pure refactoring PR.
| /* | ||
| When a new thought is created, the Shift key should be on when Auto-Capitalization is enabled. | ||
| On Mobile Safari, Auto-Capitalization is broken if the selection is set synchronously (#999). | ||
| Only breaks on Enter or Backspace, not gesture. | ||
|
|
||
| setTimeout fixes it, however it introduces an infinite loop when a nested empty thought is created. | ||
| Not calling asyncFocus when the selection is already on a thought prevents the infinite loop. | ||
| Also, setTimeout is frequently pushed into the next frame and the keyboard will intermittently close on iOS Safari. | ||
| Replacing setTimeout with requestAnimationFrame guarantees (hopefully?) that it will be processed before the next repaint, | ||
| keeping the keyboard open while rapidly deleting thoughts. (#3129) | ||
| if (!shouldSetSelection) return | ||
|
|
||
| If the last action is swapParent, set the selection synchronously to keep the focus stable after the swap. | ||
| */ | ||
| if (isTouch && isSafari() && lastUndoableActionType !== 'swapParent' && !selection.isThought()) { | ||
| asyncFocus() |
There was a problem hiding this comment.
Can you confirm that Auto-Capitalization and swapParent behavior are preserved?
| // Provide an escape hatch to allow the next default selection rather than setting it. | ||
| // This allows the user to set the selection in the middle of a non-cursor thought when keyboard is open. | ||
| // Otherwise the caret is moved to the beginning of the thought. | ||
| const allowDefaultSelection = useCallback(() => { | ||
| disabledRef.current = true | ||
| // enable on next tick, which is long enough to skip the next setSelectionToCursorOffset | ||
| setTimeout(() => { | ||
| disabledRef.current = false | ||
| }) | ||
| }, []) |
There was a problem hiding this comment.
What changed that this is no longer required?
There was a problem hiding this comment.
It looks like this is only used in scrollCursorIntoView. If so, it should probably not be separated out into a separate module. This separation is obscuring some of the complex DOM and component dependencies, including toolbar, navigation, keyboard and Layout. I would suggest putting this back into scrollCursorView and then thinking more deeply about encapsulation boundaries, dependencies, and the desired API.
device utilities are meant to be low level functions that abstract away some of the browser API details and can easily be stubbed in tests or other non-browser environments. scrollCursorIntoView already violates this encapsulation, so we'd like to try to find way to not make it worse.
| // There is a bug in iOS Safari where visualViewport.height is incorrect if the phone is rotated with the keyboard up, rotated back, and the keyboard is closed. | ||
| // It can be detected by ensuring the visualViewport portrait mode matches window portrait mode. | ||
| // If it is invalid, go back to the default | ||
| const isPortrait = window.innerHeight > window.innerWidth | ||
| const currentKeyboardHeight = window.visualViewport ? window.innerHeight - window.visualViewport.height : 0 |
There was a problem hiding this comment.
Is this special case no longer needed because visualViewport is not used anymore? Just want to make sure the iOS Safari bug described here doesn't return.
There was a problem hiding this comment.
Test Results:
- iOS Capacitor: tap a thought near the bottom of a long list — toolbar/navbar no longer jump; cursor lands above the keyboard.
- iOS Safari: autoscroll behaves the same, though some toolbar jumping is unavoidable in Safari.
- Top edge: navigating up reveals ~one thought under the toolbar.
- Desktop (no virtual keyboard): no behavior change; text selection still works on non-iOS.
- Hold Enter to add many thoughts rapidly — scroll keeps a steady one-line-per-Enter rhythm.
- Tap a thought already covered by an open keyboard — it scrolls into view.
For discussion about the last 2 items:
1. Scroll is not steady on Mobile Safari when rapidly creating a series of thought.
After 7-8 thoughts, the newly created is started at the middle of the screen.
The.newly.created.thougt.is.at.the.middle.of.the.screen.after.7-8.new.thought.with.rapid.creation.MP4
2. The view is scrolled onto the last position of the cursor when user selects all and favorite
Step to Reproduce
- Create 20 thoughts
- Place the cursor on Thought 20
- Scroll up to let the Thought 20 hidden from view
- Select all
- Tap Favorite on Command Center
Current behavior
The view is scrolled based on the position of the cursor where it left off. However, the thought is hidden under the Command Center.
I think the app is assuming the viewport without Virtual Keyboard onscreen.
Note: this does not happen on main
The.view.is.scrolled.onto.the.last.position.of.the.cursor.when.user.selects.all.and.favorite.MP4
Expected behavior
Nothing should happen
Auto scroll should not be triggered by favoriting on Select All thoughts.
Vibrating thoughts
In addition, I see an oddly vibrating thoughts on Mobile Safari 1x on this branch but I can not reproduce it anymore.
Vibrating.thoughts.mp4
@raineorshine
Please advice
|
@raineorshine Okay, sounds good – thank you for intervening. The reason for the conflation was that I was focussed on a single PR to fix the single issue #3765 – but, yes, you're right that this is too big and complex as-is. It might be a good idea to split this PR into two separate chunks so that we can discuss "native scroll prevention" and "autoscroll" as separate tickets. From the initial tests @BayuAri just did above, it looks like the "native scroll prevention" portion of this submission is working...
...but that the new JS-based autoscroll mechanism needs more work...
I'm thinking the best course forward would be to submit a new PR that just focuses test on testing the new "native scroll prevention" as its own ticket and with its own test cases before we move on to tackling JS autoscroll. What do you think? |
Fixes #3765
CleanShot.2026-06-22.at.23.30.00.mp4
Background
On iOS Capacitor, the existing autoscroll made
position: fixedelements (toolbar, navbar) visibly jump, and the cursor would often end up hidden behind the virtual keyboard.In #3765, investigations concluded that this is an issue with iOS' native autoscroll that wasn't treatable from within JavaScript. Given that we are already manipulating iOS' autoscroll with the
preventAutoscroll.ts, we came to the decision to figure out a way to fully prevent native autoscroll, and take over ourselves with JavaScript.Sandbox testing showed this could work, but with the tradeoff that we'd need to fully take over caret placement logic in JavaScript, too. Luckily, this was already being implemented in #3410, and recently shipped – unblocking this issue for further development.
Approach
Using the
preventScrolloption onHTMLElement.focus()(which is supported in iOS 15.5 or later), we now stop iOS' native autoscroll from ever firing at all when focussing a new editable in a new module calledfocusWithAutoscroll.ts. This is contrast to the centering logic approach used bypreventAutoscroll.ts, which worked in Mobile Safari but broke down in Capacitor.scrollCursorIntoViewnow solely handles autoscroll. It has been updated with smooth scrolling and a new "comfort zone" concept which makes autoscroll work with a simple rule: if the cursor is about to cross a comfort zone threshold, scroll so the cursor falls within the "comfort zone" region.Details
focusWithoutAutoscroll.tsThis new module is used across the application to focus an editable element without autoscroll. It's used to prevent autoscroll based on touch focus changes, cursor changes, keyboard events, programmatic focus changes, etc.
autoscrollEdges.tsThis module defines, in viewport co-ordinates, the editor's "comfort zone". These measurements create a kind of "safe area". A cursor that falls outside the comfort zone will trigger autoscroll. This visualization by Opus 4.8 may help to visualize this:
(Before, these values were computed inline in
scrollCursorIntoView. Now, they're defined in their own module.)A new measurement, called the "landing margin", determines how far into the comfort zone the cursor should settle once a scroll has fired – rather than landing right at the edge of the comfort zone. Here, that's
height / 2When the keyboard is open, a special
bottomBufferchanges to move the autoscroll edge to the tip of the keyboard. We subscribe to the newvirtualKeyboardStoreto get accurate and up-to-date keyboard measurements.These values might need tweaking to align with the desired feel, but the general functionality
scrollCursorIntoView.tsThis is the module that actually handles autoscroll. It mostly gets a cleanup:
preventAutoscroll's timeout – much cleaner.virtualKeyboardStoreinstead ofviewport.ts. This gets autoscroll working on Android/iOS Capacitor.autoscrollEdgesrather than computing the thresholds inline.virtualKeyboardStoreAdded a
targetHeightvalue (where the keyboard will be) alongside the existing spring-animatedheight(which is used for elements that want to move/animate/stick with the keyboard).We can do this elegantly with Capacitor (
iOSCapacitorHandler), which reports that value to us. On iOS (iOSSafariHandler), we need to predict it, and correct our prediction once we know the keyboard's height. This seemed to work okay in testing.While we're at it, moved the virtual keyboard prediction logic into
iOSSafariHandlerrather thanviewport.ts.Notes
scrollCursorIntoViewuses direct DOM reads to read information about the cursor overlay. This isn't new –maindid this too – but it does feel like an unusual approach. I've left it here for now, LMK if you feel a need to refactor this. Probably we'd just track information about the cursor in a ministore.Test checklist