Skip to content

input: Anchor completion/code-action popups to the current frame's caret - #3412

Merged
huacnlee merged 3 commits into
longbridge:mainfrom
sola-ryu:mv/input-redraw-after-caret-moves
Oct 11, 2026
Merged

huacnlee merged 3 commits into
longbridge:mainfrom
sola-ryu:mv/input-redraw-after-caret-moves

Conversation

@sola-ryu

@sola-ryu sola-ryu commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

A completion popup opened by a keystroke stays where the caret was: the menu resolves its position in render() from the input's previous frame paint layout, but the caret's new position is only known after the current frame's prepaint.

This change resolves the popup position during the current frame instead of requesting a follow-up frame:

  • TextElement::prepaint stashes fresh caret geometry (PrepaintCaretGeometry: cursor bounds, line height, input bounds, clamped scroll offset) in the state. Only the positioning geometry is stashed; the existing paint-time state updates are untouched.
  • Popups live inside deferred(), so their prepaint runs after the input's prepaint. A new CaretAnchoredPopup element resolves the anchor there via the shared caret_popup_anchor() and builds the popup content from it — position, hitboxes, accessibility bounds, and width/layout decisions (max_width, vertical vs horizontal docs) are all consistent with the resolved position.
  • Both the completion and code-action menus use the shared anchor; their duplicated origin() methods are removed.

No extra frame is requested for any input, ordinary or otherwise.

Two regression tests in crates/kit/tests/input/completions.rs:

  • completion_popup_follows_caret_on_first_frame — types a character, renders exactly one frame, and asserts the popup moved right with the caret and sits at the freshly laid-out caret position (not just moved).
  • ordinary_input_schedules_no_followup_frame — types in a plain input, settles, and asserts simulate_next_frame schedules nothing.

Public API

gpui-base

  • PrepaintCaretGeometry (doc-hidden) — caret positioning geometry from the input's latest prepaint: cursor bounds, line height, input bounds, and the clamped scroll offset.
  • InputBaseState::prepaint_caret_geometry(&self) -> Option<PrepaintCaretGeometry> (doc-hidden) — reads the stashed geometry for caret-anchored popups.

No public API changes; behavior-only fix otherwise.


This PR is 100% AI-generated: originally drafted with Claude on 2026-10-04, reworked per review feedback, rebased onto current main, reviewed and validated by the contributor: cargo fmt --check, cargo clippy -p gpui-base -p gpui-component --all-targets -- --deny warnings, and the input test suite (168 passed, including the two new regression tests) are all green.

@huacnlee huacnlee left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR addresses a valid problem: a completion popup can retain the previous caret position because it reads persisted geometry before the input publishes the current frame's layout. I confirmed that GPUI's drawing-time notification does not deliver the observer notification or request the follow-up frame in this path.

However, please revise the approach before merging:

  1. The next-frame notification is installed in the shared Input implementation for every geometry change, including input bounds, scroll size, caret bounds, and line height. This requests an additional frame even for ordinary inputs with no caret-anchored popup. It also leaves popup positioning dependent on a second frame rather than using the current frame's geometry.
  2. The new test checks that an observer receives a notification, not that the completion popup actually follows the caret. It could pass while the popup remains incorrectly positioned.

Please resolve caret-anchored popup positioning during the current frame: make the necessary caret/input/scroll geometry available after input prepaint, and read it when the popup is positioned in deferred prepaint rather than capturing the previous position during render. Keep the change narrowly focused on positioning geometry rather than moving all of the existing paint-time state updates. Share the positioning logic between completion and code-action menus, and keep popup content, hitboxes, accessibility bounds, and width/layout decisions consistent with the resolved position.

Please add a regression test that verifies the popup follows the caret on the first rendered frame after editing, with coverage for wrapping/scrolling as appropriate, and that ordinary inputs do not acquire unnecessary follow-up frame requests.

Validation for this review: inspected the PR diff, Input and popup rendering paths, and the pinned GPUI notification/frame scheduling implementation. I did not run the tests or perform UI validation.

@sola-ryu
sola-ryu force-pushed the mv/input-redraw-after-caret-moves branch from 6b5b3f2 to cb95759 Compare October 8, 2026 14:28
@sola-ryu sola-ryu changed the title input: Redraw once more after a frame moves the caret input: Anchor completion/code-action popups to the current frame's caret Oct 8, 2026
@sola-ryu

sola-ryu commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Reworked per your feedback:

  1. No extra frame: the on_next_frame notify is gone. The input now stashes fresh caret geometry (PrepaintCaretGeometry) during prepaint; popups live inside deferred(), so their prepaint runs after the input's prepaint and they anchor to the current frame's caret. Ordinary inputs get no follow-up frame requests.

  2. Shared positioning: both menus' duplicated origin() methods are replaced by a shared caret_popup_anchor(), and both renders go through a new CaretAnchoredPopup element that resolves the anchor in its deferred prepaint and builds the popup content from it — position, hitboxes, accessibility bounds, max_width, and the vertical/horizontal docs layout are all derived from the resolved position.

  3. Tests: completion_popup_follows_caret_on_first_frame types a character, renders exactly one frame, and asserts the popup sits at the freshly laid-out caret (via test-supported observation of the menu bounds, compared against the stashed geometry). ordinary_input_schedules_no_followup_frame asserts simulate_next_frame schedules nothing after typing in a plain input.

Only the positioning geometry is stashed in prepaint; the existing paint-time state updates are untouched.

The popup position was resolved in render() from the previous frame's
paint layout, so a completion menu opened on the keystroke's own frame
stayed where the caret was until the next frame.

The input now stashes fresh caret geometry (PrepaintCaretGeometry) during
prepaint. Popups live inside deferred(), so their prepaint runs after the
input's prepaint; a new CaretAnchoredPopup element resolves the anchor
there via the shared caret_popup_anchor() and builds the popup content
from it, keeping position, hitboxes, accessibility, and width decisions
consistent with the resolved position. No follow-up frame is requested;
ordinary inputs are unaffected.
@sola-ryu
sola-ryu force-pushed the mv/input-redraw-after-caret-moves branch from cb95759 to c10d183 Compare October 8, 2026 14:35
@sola-ryu

sola-ryu commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto latest main (now includes #3410, #3411, #3413). No code changes — one import-list conflict resolved in crates/component/src/input/mod.rs (ShowCompletions from #3411 + PrepaintCaretGeometry from this PR). Verified: 181 input tests pass, clippy clean with --deny warnings, fmt clean.

Keep the code-action popup hitbox sized to its contents, encapsulate prepaint caret geometry, and verify completion positioning on the first frame including wrapping and scrolling.

AI-assisted change using Codex.
@huacnlee
huacnlee enabled auto-merge (squash) October 9, 2026 11:20
@endless-bot
endless-bot disabled auto-merge October 11, 2026 03:57
@huacnlee
huacnlee merged commit 75fc4ed into longbridge:main Oct 11, 2026
16 checks passed
@huacnlee huacnlee mentioned this pull request Oct 12, 2026
huacnlee added a commit that referenced this pull request Oct 12, 2026
## Description

Prepare v0.7.2 on `stable` from the PRs merged into `main` since v0.7.1,
leaving out every change that breaks existing consumers. `stable`
already carries #3371 and #3373; this adds the remaining compatible PRs
with `git cherry-pick -x` and bumps the workspace to 0.7.2.

### Included

- kit: Add optional gpui-fast backend (#3375)
- tooltip: Make show delay and grace period configurable with
`TooltipDefaults` (#3376)
- docs: Explain recovering focus when the focused element stops
rendering (#3386)
- diff: Add readonly patch viewer (#3378)
- kit: Use one Windows manifest with GPUI Fast (#3380)
- test: Make Progress and TitleBar findable in UI tests (#3383)
- input: Preserve selection, focus ring and context menu actions (#3382)
- diff: Add a Git history example and complete review APIs (#3388)
- text_view: Ignore gestures that stay inside one block plugin (#3390)
- text_view: Round the first and last table rows to the frame radius
(#3309)
- menu: Open a context menu with a long press on touch (#3393)
- text_view: Keep narrow table columns on one line (#3399)
- settings: Keep focus rings inside the list viewport (#3394)
- scrollable_mask: Keep rounded scroll-table corners clean at any scroll
offset (#3400)
- dialog: Add margin_top to AlertDialog (#3404)
- input: Add InputState::add_selection for programmatic multi-cursor
(#3403)
- text_selection: Stop writing selection state on every frame (#3407)
- table: Make the sort icon a labelled button (#3396)
- table: Report visible row ranges of one row or none (#3397)
- dialog: Support configurable entrance animations (#3405)
- docs: Explain the open_dialog builder and footer contracts (#3406)
- dock: Hand the focus to the tab that replaces a closed one (#3409)
- dropdown_button: Add A11y names for the menu trigger (#3410)
- text, resizable: Stop notifying state that did not change (#3418)
- editor: Add on-demand completion commands (#3411)
- table: Clear a right-clicked cell's outline on the next click (#3413)
- tab: Scroll a clicked tab into view with the next one peeking (#3419)
- table: Allow a header row height separate from the body (#3420)
- menu: Focus a context menu before its first frame (#3422)
- text: Preserve logical selection across presentation changes (#3421)
- text: Keep inline HTML mark highlights in Markdown (#3425)
- input: Anchor completion/code-action popups to the current frame's
caret (#3412)
- input: Run custom context menu builders after releasing the input
(#3433)
- input: Keep auto-grow rows fitted when the limits change (#3430)
- text: Paint TextView selection under the glyphs (#3439)

Already on `stable`: #3371, #3373.

### Excluded (Breaking Changes)

- #3432 motion: `Easing::sample` now returns values outside `[0, 1]` for
overshooting curves.
- #3415 tab: new `TabVariant::Folder` breaks exhaustive matches.
- #3402 kit: pointer helpers now keep held modifiers; nested
context-menu behavior changes in tests.
- #3384 test: `TestWindowExt` gains required methods.
- #3302 resizable: `ResizeHandle::on_drag` takes `&T` instead of
`Rc<T>`.
- #3389, #3395, #3398 webview: wrapper API changes, `gpui-wry` →
`gpui-webview` rename, and the GPUI Fast requirement.

#3411, #3405 and #3396 have a Breaking Changes section that reads "None"
and are included. #3376 only deprecates `TooltipRequest::placement`, and
#3397 / #3382 change behavior compatibly, so they are included too.

### Adaptations

- `crates/kit/tests/collections.rs` and `crates/kit/tests/rendering.rs`:
conflict resolutions drop the tests that belong to #3402 and keep each
backported PR's own test.
- `test: Keep backported tests on the v0.7.1 test API`: the menu and
completion tests from #3422 and #3412 used `find_all` and
`pointer_down`/`pointer_up` from #3384/#3402. They now query
`gpui_kit::base::test_support::snapshots` and dispatch mouse events
directly.
- `kit: Prepare v0.7.2`: workspace version bump; `Cargo.lock` only
updates workspace members.

## Public API

No new public API beyond what the included PRs add; each lists its items
in its own `## Public API` section (notably #3375, #3376, #3378, #3388,
#3403, #3404, #3405, #3411, #3420).

## How to Test

- `cargo clippy --workspace --all-targets -- --deny warnings` passes on
Linux.
- `cargo test -p gpui-kit --features "test-support component assets"
--test collections --test menu --test input` passes on Linux.
- The macOS-only `rendering` test (#3439 adds one case, the count is
updated to 11) was not run.

AI-assisted: the backport, conflict resolution and test adaptations were
done with Claude Code.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Floyd Wang <codelife721@gmail.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Gustave <gustavel@gmail.com>
Co-authored-by: luren <84951845+lurenjia534@users.noreply.github.com>
Co-authored-by: Ylin <ylinwind@gmail.com>
Co-authored-by: ihavecoke <mixboip@gmail.com>
Co-authored-by: Lorenzo <150276837+Bombatomica64@users.noreply.github.com>
Co-authored-by: MR. XIE <113512030+xxww0098@users.noreply.github.com>
Co-authored-by: xxww0098 <shefkie1957@gmail.com>
Co-authored-by: Sola 🌙🤖 <266175376+sola-ryu@users.noreply.github.com>
Co-authored-by: Sola <sola-ryu@users.noreply.github.com>
Co-authored-by: Andy Hsu <i@nn.ci>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Floyd Wang <gassnake999@gmail.com>
Co-authored-by: Daiki Tomioka <20753105+TomiXRM@users.noreply.github.com>
Co-authored-by: Henrik Øgård <henrik@ogard.no>
Co-authored-by: Henrik Øgård <henrikogaard@users.noreply.github.com>
Co-authored-by: Endless <endless@longbridge.com>
Co-authored-by: John Costa <costajohnt@gmail.com>
Co-authored-by: Alexander <87220898+sneffets@users.noreply.github.com>
Co-authored-by: Cesar Talén <154442565+cesartalen@users.noreply.github.com>
@sola-ryu
sola-ryu deleted the mv/input-redraw-after-caret-moves branch October 14, 2026 03:14
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.

3 participants