input: Fix accessibility focus and SetValue - #3246
Conversation
Bind semantic input focus to the editor and send accessibility value changes through the existing user-edit path. Preserve addon focus rings, read-only and disabled guards, masked value privacy, change events and undo.
huacnlee
left a comment
huacnlee
left a comment
There was a problem hiding this comment.
Reviewed the focused accessibility change and regression tests. SetValue preserves replace_all and rejects writes when read-only or disabled. Focus targets the editor. Security: PASS for the affected scope. Linux, macOS, and Windows CI passed.
|
@steipete, we found a keyboard-focus regression after merging this PR. The outer semantic frame and inner editor both register the same FocusHandle in GPUI's TabStopMap. Shift-Tab can resolve another entry for the current editor and stay on that input. This was missed in our review. We have opened a full revert in #3253, with four UI regression tests covering adjacent inputs, forward/reverse traversal and wraparound, prefix/suffix buttons, and Textarea/Editor focus departure. All four reproduce traversal failures before the revert and pass afterward. We also opened a narrower draft follow-up in #3254, based on your action handlers and tests: explicit guarded Focus plus editable-only SetValue, retaining replace_all and the original frame/editor focus ownership. Its 10 UI tests and 12 component tests pass. Native accessibility dispatch still needs acceptance testing, and reported accessibility focus remains unresolved with the independent frame handle. Please take a look at #3254, particularly the native accessibility behavior. Any further focus-binding change needs to preserve existing Tab/Shift-Tab order and addon focus. The revert and follow-up are currently open, not merged. |
## Description Reverts #3246 (8a7e0ec) in full and adds focused UI regression coverage. The outer semantic frame and inner editor both registered the same FocusHandle in the TabStopMap. Reverse traversal could resolve another entry for the currently focused editor, leaving Shift-Tab stuck. The regression affects Input and the reverse-focus traversal primitive used with Textarea and Editor. The production change is the exact inverse of #3246. The tests cover adjacent inputs, repeated forward/reverse cycles and wraparound, prefix/suffix button order and keyboard activation, multiline body clicks, editing, selection, and focus departure. Button activation sends native key-down and key-up events. This full revert also removes #3246's accessibility Focus action and read-only/disabled SetValue protections. Draft follow-up #3254 restores narrowly scoped action handling while preserving existing focus ownership and the replace_all path. No public API or dependency changes. ## How to Test - Before revert: all four new regression scenarios failed on focus traversal. - After revert: `cargo test -p gpui-kit --features test-support --test input_focus --test input --locked` — 10 passed. - `cargo test -p gpui-component --features test-support --lib input::input::tests --locked` — 11 passed. - `rustfmt --check --edition 2024 crates/kit/tests/input_focus.rs` and `git diff --check` passed. Native screen-reader automation and Story GUI checks were not run. This PR restores prior behavior; it does not claim to resolve the original accessibility defects. AI-assisted: Codex prepared the revert, regression tests, and description; the diff was checked and the tests above were executed by the agent. Maintainer review remains pending.
…er (#3254) ## Description Follow-up to #3246 after the full revert in #3253. This PR targets main and contains only the independent accessibility action fix. Restore the narrowly scoped accessibility action fixes without sharing the editor's FocusHandle between the semantic frame and editor: - Explicit Focus action focuses the editor unless disabled. - SetValue is advertised only when editable and rechecks editability when executed. - Replacement continues through the existing replace_all path. The original frame handle, addon hierarchy, editor rendering, tab registration, and editing logic remain as before #3246. Production changes are limited to the action handlers and their registration; the remaining diff restores regression tests from #3246. No public API or dependency changes. ## How to Test - `cargo test -p gpui-kit --features test-support --test input_focus --test input --locked` — 10 passed, including the four focus regressions retained by #3253. - `cargo test -p gpui-component --features test-support --lib input::input::tests --locked` — 12 passed. - `cargo clippy -p gpui-kit --features test-support --test input_focus --locked -- --deny warnings` passed. - Targeted rustfmt checks and `git diff --check` passed. ## Remaining acceptance work Headless tests invoke accessibility helpers and inspect advertised SetValue properties; they do not drive native accessibility requests through GPUI's private dispatcher. - Verify native Focus followed by typing, including read-only and disabled controls. - Verify native SetValue, including editability changing after an action was queued. - Accessibility focused-node reporting remains unresolved: the independent semantic frame is not the actual editor focus handle, so the native accessibility tree can report window-root focus. This PR does not claim to preserve all focused-node reporting improvements from #3246. Native screen-reader automation and Story GUI checks have not run. AI-assisted: Codex prepared this focused follow-up and description using the implementation and tests from @steipete's #3246, and executed the listed automated checks. Maintainer/native acceptance remains pending.
2 participants
Accessibility Focus targets the input frame instead of the keyboard editor, and SetValue can overwrite read-only inputs. On the original main,
crates/component/src/input/input.rs:716tracksframe_focus_handle, while:488callsreplace_all; its programmatic path explicitly lifts edit restrictions (crates/base/src/input/base/state.rs:969). The read-only regression test fails before this fix.Bind the semantic node to the editor's focus handle and handle Focus with the current disabled guard. Keep separate addon focus scopes so their focus rings still work. Offer SetValue only for editable inputs and recheck editability when the action runs, while keeping the existing
replace_allpath and its normalization, change events, masking, and undo behavior. Add an Editor regression test to ensure SetValue preserves an exact value that would otherwise trigger typing auto-close. No public API or dependency changes.Validation on the current revision (Linux):
cargo test -p gpui-component --features test-support --locked --quiet: 613 unit and 48 integration tests passed.cargo clippy -p gpui-component --all-targets --locked -- --deny warnings,cargo fmt --all -- --check,cargo check -p gpui-component --no-default-features --locked, andgit diff --checkpassed.The author also validated the original focus change in an isolated signed downstream macOS app: Peekaboo 4.5.0 background click focused the actual editor, and background SetValue changed the URL, confirmed by a fresh accessibility-tree read. The frontmost PID was unchanged. This used the same original focus diff on v0.6.6 because main declares v0.6.5 crates that the app's v0.6.6 dependency does not consume; no AccessKit or GPUI patches were used.
Follow-ups: text ranges/selection, ReplaceSelectedText and broader accessibility documentation remain out of scope. AccessKit macOS still reports
AXFocused=falsefor inactive windows; Peekaboo reports indeterminate results despite confirmed internal focus and subsequent value readback. Manual Linux/Windows testing has not run.AI-assisted: this change was developed with AI coding assistance (OpenAI Codex) and reviewed and tested by the author.