input: Do not start the blink cursor on an unfocused input - #3140
Merged
Merged
Conversation
`BlinkCursor::stop`, called from `on_blur`, resets the epoch but leaves `paused` and `visible` holding whatever the last `pause` set. A blur inside the 300ms pause window — tabbing away right after a keystroke — therefore leaves `paused == true` on a stopped cursor. The next `start` then hits `blink`'s `if self.paused` early return and never moves the epoch off zero, so the refocused input has a cursor that is not blinking. The defect is masked today, and masked by accident: `pause` acts on any cursor, blinking or not, so the next keystroke starts the loop again. That accident is itself a bug — it is what makes a programmatic `set_value` blink an unfocused input forever — and the next commit removes it. This one has to land first so that removing the mask does not turn a hidden defect into a visible one. Clear `paused` and `visible` alongside the epoch, so a stopped cursor carries nothing into the next focus. As a side effect the caret now appears the moment an input is focused, rather than starting from whatever `visible` the previous focus left behind. novakduc@arch
`InputState::set_value` pauses the blink cursor whether or not the input is focused, and a pause resumes blinking 300ms later. On an input that was never focused, or that was blurred, the pause is what *starts* the blink loop, and nothing ever stops it: the blur that would call `stop` has already happened. Every blink calls `cx.notify()`, and `InputState` observes its own cursor, so each blink repaints the whole view the input sits in. One programmatic `set_value` on an off-screen input therefore costs two full view repaints per second for the life of the window, and a pane that seeds twenty inputs pays it twenty times over. Nothing appears on screen: the input is not focused, so it draws no cursor. ### The fix `BlinkCursor` already uses `epoch == 0` as its "not blinking" sentinel — `new` starts there, `stop` resets to it, and `start` moves off it. `pause` now honours that sentinel and returns early, so only a cursor that is already blinking can be paused. A focused input is unaffected: `on_focus` calls `start`, which leaves the epoch at 1 or more, and there is no window in which a focused input has a zero epoch. The guard is in `pause` rather than at the call site because `pause_blink_cursor` has ten production callers across `state.rs` and `movement.rs`, plus two direct `BlinkCursor::pause` calls; guarding one entry point would leave the rest. It also matches the keystroke path, which already wraps its `pause` in `focus_handle.is_focused(window)`. This is also what stops masking the stale-pause defect fixed in the previous commit, which is why that one comes first. ### Verification - `cargo test -p gpui-base --lib` (991) and `cargo test -p gpui-component --lib` (547) pass; clippy and `cargo fmt --check` are clean at both commits. - The two new tests were confirmed red without the guard and green with it. - `repeated_pauses_keep_cursor_visible_until_idle` now starts the cursor before pausing it: it asserts the focused typing behaviour, and a never-started cursor no longer stands in for a focused one. - `test_multi_cursor_actions_reveal_hidden_carets` faked the hidden blink phase with a fresh `BlinkCursor`; it now starts one and advances the clock into the hidden phase instead. - `test_cursor_layout_consumer_updates_after_selection` was relying on the stray blink loop for the frame in which its consumer read the caret geometry. A notify sent from inside a draw marks the view dirty without asking for another frame, so the test now invalidates the input explicitly. novakduc@arch
huacnlee
enabled auto-merge (squash)
September 20, 2026 12:47
Member
|
Thank you. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3138. Depends on #3139 — please merge that one first.
This branch is stacked on #3139, so its commit still shows here until that PR
lands; afterwards this diff is just the
pauseguard and its tests. #3139 is aone-line-each reset in
stopand is not a regression on its own. This PR is,if it merges alone: see "Why the order is forced" below.
Summary
InputState::set_valuepauses the blink cursor whether or not the input isfocused, and a pause resumes blinking 300ms later. On an input that was never
focused, or that was blurred, the pause is what starts the blink loop — and
nothing ever stops it, because the blur that would call
stophas alreadyhappened.
Every blink calls
cx.notify(), andInputStateobserves its own cursor, soeach blink repaints the whole view the input sits in. One programmatic
set_valueon an off-screen input costs two full view repaints per second forthe life of the window, and a pane that seeds twenty inputs pays it twenty
times over. Nothing appears on screen while it happens: the input is not
focused, so it draws no cursor.
The chain
Starting from a cursor with
epoch == 0:pausepaused,visible, notify,epoch 0 → 1, timer 300msblink(1)1 == self.epoch, so it proceeds: notify, timer 500msblink(2)The fix
BlinkCursoralready usesepoch == 0as its "not blinking" sentinel:newstarts there,
stop(fromon_blur) resets to it, andstart(fromon_focus) moves off it.pausenow honours that sentinel and returns early,so only a cursor that is already blinking can be paused. A focused input is
unaffected —
on_focuscallsstart, which leaves the epoch at 1 or more, andthere is no window in which a focused input has a zero epoch.
The guard is in
pauserather than at the call site becausepause_blink_cursorhas ten production callers acrossstate.rsandmovement.rs, plus two directBlinkCursor::pausecalls; guarding one entrypoint would leave the rest. It is also what the keystroke path already does —
intercept_keystrokeswraps itspauseinfocus_handle.is_focused(window),so an unfocused pause is already treated as wrong there. The programmatic path
just was not covered.
Why the order is forced
stopcurrently leavespausedset (#3139). That defect is invisible todayonly because
pauseacts on any cursor, so the next keystroke restarts theloop — the very thing this PR removes. Merged before #3139, this guard would
turn that hidden defect into a caret that never blinks after a refocus:
blurring_a_paused_cursor_leaves_the_next_focus_blinking, added in #3139,fails against this commit without it.
Public API
None.
BlinkCursorand itspausearepub(crate). (#3139 carries the oneobservable change, in
OtpState::cursor_visible.)Verification
Automated
pausing_a_cursor_that_is_not_blinking_does_not_start_it, the reportedbug: a never-started cursor is paused, and must send no notification over the
next three seconds. Red without the guard (six notifies), green with it.
test_set_value_on_unfocused_input_stays_quiet, which drives a realInputStatethrough a window: afterset_valuesettles, advancing the clockthree seconds must not notify again. Red without the guard.
stopreset alone, bothtests above still fail.
stopruns only fromon_blur, and the input inBlinkCursor::pausestarts a permanent blink loop on an unfocused input #3138 was never focused, so nothing on that path reaches it. This guard iswhat fixes the filed bug.
cargo test --workspace --exclude gpui-shell --features gpui-component-story/test-support --locked: 106 test binaries, 0 failures.cargo clippy --workspace --exclude gpui-shell --locked -- --deny warnings,cargo fmt --check,typos,cargo machete,python3 script/check-ai rustand
cargo test -p gpui-base -p gpui-component --doc: all clean.webview,gpui-wry) and the two wasm crateswere excluded locally for want of
webkit2gtk-4.1; CI covers them.Three existing tests changed
repeated_pauses_keep_cursor_visible_until_idlepaused a cursor that wasnever started. It asserts the focused typing behaviour, so it now calls
startfirst and makes that explicit. (Flagged inBlinkCursor::pausestarts a permanent blink loop on an unfocused input #3138 as the one expectedcasualty; there turned out to be two more.)
test_multi_cursor_actions_reveal_hidden_caretsfaked the hidden blink phasewith a fresh
BlinkCursor. It now starts one and advances the clock into thehidden phase, which is the state it meant to describe.
test_cursor_layout_consumer_updates_after_selectionwas passing because ofthis bug: the stray blink loop supplied the frame in which its consumer read
the freshly painted caret geometry. A notify sent from inside a draw marks the
view dirty without asking for another frame —
invalidate_viewsetsdirtyand wakes the platform only when
draw_phase == DrawPhase::None— so the testnow invalidates the input explicitly before reading the baseline. The
assertion it exists for, that a caret move reaches render consumers, is
unchanged.
Manual
Not done.
docs/ACCESSIBILITY-UI-TESTING.mdis the required method for focusand input behavior and it is macOS-only (
./script/run-story-macos, the macOSaccessibility tree); this branch was written on Linux. The pass worth running on
the Input story is the one #3139 covers: type a character, Tab away and
Shift-Tab straight back within 300ms, and confirm the caret appears immediately
and keeps blinking.
AI assistance
Written with Claude Code: the guard, the new tests and the changes to the three
existing tests are all AI-generated, reviewed and run by me. The bug itself was
diagnosed downstream from CPU sampling, not by the model.
novakduc@arch