Skip to content

input: Clear the blink state when the cursor stops - #3139

Merged
huacnlee merged 2 commits into
longbridge:mainfrom
novakduc:input-blink-cursor-stop-reset
Sep 20, 2026
Merged

huacnlee merged 2 commits into
longbridge:mainfrom
novakduc:input-blink-cursor-stop-reset

Conversation

@novakduc

Copy link
Copy Markdown
Contributor

Summary

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, never moves the epoch off zero, and the
refocused input has a cursor that is not blinking.

// blink_cursor.rs
fn blink(&mut self, epoch: usize, cx: &mut Context<Self>) {
    if self.paused || epoch != self.epoch {   // <- a stale `paused` swallows `start`
        self.visible = true;
        return;
    }
    ...
}

The defect is in the tree today and is masked by accident: pause acts on any
cursor, blinking or not, so the next keystroke starts the loop again. That
accident is itself the bug in #3138 — it is what makes a programmatic
set_value blink an unfocused input forever. Fixing #3138 removes the mask, so
this wants to land first, or fixing one bug turns the other from hidden into
visible.

The fix

Clear paused and visible alongside the epoch, so a stopped cursor carries
nothing into the next focus. stop is the end of a blink loop; leaving two
flags behind from the middle of the previous one has no meaning for the next.

As a side effect the caret now appears the moment an input is focused, rather
than starting from whatever visible the previous focus happened to leave
behind.

Public API

No signature changes. One public function changes what it observes:
OtpState::cursor_visible now returns false after a blur, where the stale
flags could previously keep it true. Both call sites (gpui-component's
OtpInput and gpui-shell's otp_input) already gate the caret on focus, so
nothing changes on screen.

Verification

Automated

  • Added blurring_a_paused_cursor_leaves_the_next_focus_blinking: start, pause,
    stop, start — the caret must show immediately and hide one interval later.
    Confirmed red on current main, green with this change.
  • cargo test -p gpui-base --lib (989) and cargo clippy -p gpui-base --lib --tests -- --deny warnings pass at this commit; cargo fmt --check, typos
    and cargo machete are clean.

Manual

Not done. docs/ACCESSIBILITY-UI-TESTING.md is the required method for focus
and input behavior and it is macOS-only (./script/run-story-macos, the macOS
accessibility tree); this branch was written on Linux. The pass worth running on
the Input story: 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 change and its test are AI-generated, reviewed and
run by me.

novakduc@arch

`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
@novakduc

Copy link
Copy Markdown
Contributor Author

Prerequisite for #3140 (which fixes #3138). That PR removes the accident currently masking this defect, so this wants to land first.

@huacnlee
huacnlee enabled auto-merge (squash) September 20, 2026 12:31
@huacnlee
huacnlee merged commit 9a8c196 into longbridge:main Sep 20, 2026
12 checks passed
huacnlee added a commit that referenced this pull request Sep 20, 2026
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 `pause` guard and its tests.
#3139 is a
one-line-each reset in `stop` and is not a regression on its own. This
PR is,
if it merges alone: see "Why the order is forced" below.

## Summary

`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, because 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 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 while it happens: the input is not
focused, so it draws no cursor.

### The chain

```
InputState::set_value        → replace_text → replace_text_in_range_silent
  → replace_text_in_range    → self.pause_blink_cursor(cx)   ← no focus check
    → BlinkCursor::pause                                     ← no focus check
```

Starting from a cursor with `epoch == 0`:

| t | what runs | effect |
| :----- | :------------------ |
:----------------------------------------------------------- |
| 0ms | `pause` | `paused`, `visible`, **notify**, `epoch 0 → 1`, timer
300ms |
| 300ms | resume → `blink(1)` | `1 == self.epoch`, so it proceeds:
**notify**, timer 500ms |
| 800ms | `blink(2)` | **notify**, timer 500ms |
| … | | two notifies a second, indefinitely |

## The fix

`BlinkCursor` already uses `epoch == 0` as its "not blinking" sentinel:
`new`
starts there, `stop` (from `on_blur`) resets to it, and `start` (from
`on_focus`) 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 is also what the keystroke path already
does —
`intercept_keystrokes` wraps its `pause` in
`focus_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

`stop` currently leaves `paused` set (#3139). That defect is invisible
today
only because `pause` acts on any cursor, so the next keystroke restarts
the
loop — 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. `BlinkCursor` and its `pause` are `pub(crate)`. (#3139 carries the
one
observable change, in `OtpState::cursor_visible`.)

## Verification

### Automated

- Added `pausing_a_cursor_that_is_not_blinking_does_not_start_it`, the
reported
bug: 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.
- Added `test_set_value_on_unfocused_input_stays_quiet`, which drives a
real
`InputState` through a window: after `set_value` settles, advancing the
clock
  three seconds must not notify again. Red without the guard.
- Confirmed the two halves separately: with #3139's `stop` reset alone,
both
tests above still fail. `stop` runs only from `on_blur`, and the input
in
#3138 was never focused, so nothing on that path reaches it. This guard
is
  what 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
rust`
  and `cargo test -p gpui-base -p gpui-component --doc`: all clean.
- The webview-dependent crates (`webview`, `gpui-wry`) and the two wasm
crates
  were excluded locally for want of `webkit2gtk-4.1`; CI covers them.

### Three existing tests changed

- `repeated_pauses_keep_cursor_visible_until_idle` paused a cursor that
was
never started. It asserts the focused typing behaviour, so it now calls
`start` first and makes that explicit. (Flagged in #3138 as the one
expected
  casualty; there turned out to be two more.)
- `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, which is the state it meant to describe.
- `test_cursor_layout_consumer_updates_after_selection` was passing
*because of*
this 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_view` sets
`dirty`
and wakes the platform only when `draw_phase == DrawPhase::None` — so
the test
  now 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.md` is the required method for
focus
and input behavior and it is macOS-only (`./script/run-story-macos`, the
macOS
accessibility 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

---------

Co-authored-by: Jason Lee <huacnlee@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants