fix(webui): let Tab focus the Playbook selection menu - #1300
Merged
Conversation
Selecting text in the web Playbook editor pops up the selection action
menu, but the editor's keydown handler gave Tab exactly one job: insert an
indent. With a selection live, `document.execCommand("insertText")`
*replaces* it — so Tab both failed to reach the menu and destroyed the very
text the menu was offering to act on.
The TUI has settled this contract over #1240/#1285/#1286/#1289 and spec 0196
records it. This mirrors that end state rather than inventing a web one:
- Bare Tab moves focus into a visible menu; the document is untouched.
- Once focused, Tab/S-Tab and the arrows walk the rows, Enter runs the
focused one (Shift: in a fork), C-g cancels selection and menu together.
- Escape peels the menu off but keeps the selection, so the next Tab reaches
the editor and nests every list line the selection spans (S-Tab un-nests).
A second Escape then cancels the region — two stages, as in the TUI.
- C-o still focuses the menu, and can reopen one Escape dismissed.
Menu focus is real focus: the rows are `role="menuitem"` buttons that the
browser reports as `document.activeElement`, not a painted highlight. Two
details the round trip needs — the editor's `blur` handler disarms the mark,
and a blurred contenteditable does not reliably keep a readable Range — so
both are parked on focus and restored when focus comes back, leaving the
region extendable and Escape's second stage with something to cancel.
Tab with no selection still inserts a tab stop, and nothing outside the
Playbook editor changes.
Spec 0196 previously scoped itself to the TUI and listed web menu focus as a
non-goal; it now states the contract for any client that shows the menu.
Verified against a real Chrome over CDP with trusted key events, for both a
pointer drag and a C-Space region. The new web_smoke coverage asserts
`activeElement` moves into the menu and `playbookSerialize()` is unchanged;
reverting the Tab branch fails it with `text: "- item"` — the original bug.
Merged
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.
Root cause
Selecting text in the web Playbook editor pops up the selection action menu, but
playbookOnKeyDowngave Tab exactly one job:With a selection live,
insertTextreplaces it. So Tab both failed to reach the menu and destroyed the very text the menu was offering to act on. The menu was pointer-only: nothing moved focus into it, and its rows were not reachable from the keyboard at all.The TUI contract I matched
The TUI settled this over #1240 → #1285 → #1286 → #1289, and spec 0196 records it. I derived the end state from
crates/cli/src/app/editor.rsas it stands onmain(handle_playbook_selection_menu_key), not from any single commit message:C-oC-gThe web now mirrors all of it. Note the ordering subtlety #1286/#1289 encode and that this reproduces: Escape's menu-dismissal stage runs before the region cancel, and an empty region retires the dismissal so the menu returns after a
C-Spacekeyboard selection (whose range is momentarily empty between the mark and the first motion).Composing with #1298
playbookOnKeyDownhad just gained the emacs mark. Two details that path forced:blurhandler callsplaybookDeactivateMark(), so focusing a menu button disarmed the region — costing Escape its second stage on a keyboard selection.Range, andplaybookRun/playbookRunVerbread the live selection.Both are snapshotted when focus enters the menu and restored when it comes back, so a mouse-drag selection and a
C-Spaceregion behave identically.Accessibility
Focus is real focus, not a highlight: the menu is
role="menu", rows arerole="menuitem"<button>s that the browser reports asdocument.activeElementand renders with:focus-visible. Dismissal returns focus deliberately to the editor.Evidence
Driven against real Chrome over CDP with trusted key events (a synthetic
dispatchEventnever runs the default action apreventDefaultassertion needs to be meaningful), for both a real mouse drag and aC-Space+ motion region.Verified end-to-end: Tab focuses
playbookSelectionRunBtnwithplaybookSerialize()unchanged; a second Tab moves to▸ Challenge assumptions; Enter genuinely ran the selection (run sent (selection, v7)); dismissed-menu Tab turned- alpha iteminto- alpha itemacross a 3-line selection while leaving the prose line alone; plain Tab with no selection still inserts a tab stop.Tests
New
web_smokecoverage assertsactiveElementmoves into the menu andplaybookSerialize()is unchanged, then that Escape→Tab indents instead of clobbering. Tab is dispatched with itstextso the browser's insertion default action is genuinely in play — otherwise "document unchanged" would pass with the handler removed.Proved non-vacuous by reverting the Tab branch and re-running:
That failure output is the reported bug — "alpha" replaced by two spaces.
cargo buildthencargo test --workspaceunfiltered: 2836 passed, 0 failed.Spec
Spec 0196 scoped itself to the TUI and listed "Web UI menu focus" as an explicit non-goal, which this change contradicts. Rather than add an 0208 that conflicts, I updated 0196 to state the contract for any client that shows the menu, and recorded the two consequences this PR discovered (focus must be real focus; borrowing focus must not cost the user their region).
Scope
crates/daemon/assets/index.htmlonly — the change lives in theconstructbinary (assets areinclude_str!'d into the daemon). Expect a trivial merge with the concurrent playbook font-size branch.