Repository navigation
Let focus travel with the tab it moved - #287
Merged
Merged
Conversation
Ctrl+Shift+Arrow moved the tab but left focus on the index it came from, so a second press moved whatever had slid into that slot. Two tabs ping-ponged instead of one walking down the strip: start [Discover, Scan, Scan] focus index 0 (Discover) Ctrl+Shift+Right #1 [Scan, Discover, Scan] focus index 0 (Scan) Ctrl+Shift+Right #2 [Discover, Scan, Scan] focus index 0 (Discover) The focus-follow code was what broke focus-follow: onMoveTab(tab.id, to); const sibling = event.currentTarget.parentElement?.children[to]; if (sibling instanceof HTMLElement) sibling.focus(); It ran synchronously, before React re-rendered, so `children[to]` read the pre-move DOM and grabbed the neighbour -- the tab about to slide into the slot being vacated. React then reordered and carried that neighbour back to the old index, with focus on it. The fix is to delete it. Focus is already on this tab's own element, and the list is keyed by tab id, so React reorders by moving that node rather than recreating it; focus travels with the tab without help. The existing test asserted only that onMoveTab was called, so it passed throughout -- its onMoveTab is a mock and the tabs never actually move, which makes everything that depends on the re-render invisible to it. The new test renders a strip that really reorders, and checks the moved element still holds focus and that a second press carries the same tab onward rather than swapping the pair back. It fails against the old code on exactly that assertion.
Owner
Author
|
Confirmed in the running app with native keystrokes (not synthetic), which is how the bug was originally established — the walkthrough warns the two diverge in this harness. Before, the pair ping-ponged and focus never left index 0. After, the Discover tab walks 0 → 1 → 2 and keeps focus throughout — a repeated press carries the same tab, which is what the code always claimed. Method: Vite served this branch from an isolated worktree on 1420 so the app loaded the fixed source, verified by fetching the module through Vite and confirming the The outstanding app-level check from the PR description is now done. |
fstubner
added a commit
that referenced
this pull request
Aug 29, 2026
#287 fixed the focus bug this branch recorded under Known gaps, so the entry describes behaviour that no longer exists. Step 3 states the working behaviour instead, with the native-key observation that confirmed it.
fstubner
added a commit
that referenced
this pull request
Aug 29, 2026
* Walk the tab strip, and record the keyboard bug it turned up The desktop walkthrough covered launching, running, reading and exporting a result, but said nothing about reordering or closing tabs -- so the acceptance gate structurally could not catch a regression in any of it. Written the way the rest of the file claims to be: by driving the running app and recording what it did, not by reading the components. Every figure quoted is from a run at 1100x800 with three tabs open. That turned up a defect. Ctrl+Shift+Arrow moves the tab but leaves focus on the index it left, so a second press moves whatever slid into that slot and two tabs ping-pong instead of one walking down the strip. Reproduced with both synthetic and native key events, so it is not the selenium artifact the file already warns about. `tabStripKeyboard.ts` and #281 both state that focus follows the tab; that is the intent, not the behaviour. Recorded under Known gaps rather than fixed here, so the fix is reviewed on its own. Also observed and now written down: the strip reorders live rather than on release, a cancelled drag reverts to the pre-press order, plain arrows move the selection and wrap while Ctrl+Shift moves the tab and clamps, the close menu disables rather than hides items that would do nothing, and closing every tab lands on "No Tabs Open" rather than a blank window. * Correct the tab fixture, and record that reordering renames tabs The fixture in "Managing tabs" wrote the three tabs as `Discover 192.168.1.0/24` and two `Scan 127.0.0.1`. The strip does not label them that way: duplicates carry `#` and their position in the strip, so they read `Scan: 127.0.0.1 #2` and `#3`, and there is no `#1`. That is not just a wrong fixture. The number is a position, not a name, so it moves when anything around it moves -- and drag-to-reorder is the headline of the section that got this wrong. Observed: moving the Discover tab one place right turns `#2` into `#1` and leaves the two duplicates reading `#1` and `#3` with no `#2` between them. Found by an independent acceptance pass, which is the argument for running one: the section was written from the app, and this still went unnoticed because I had read the labels as the values I typed into the form rather than as what the strip rendered. * Record Ctrl+Shift+Arrow as working, now that it is #287 fixed the focus bug this branch recorded under Known gaps, so the entry describes behaviour that no longer exists. Step 3 states the working behaviour instead, with the native-key observation that confirmed it.
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
1 participant
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.
Ctrl+Shift+Arrowmoved the tab but left focus on the index it came from, so a second press moved whatever had slid into that slot. Two tabs ping-ponged instead of one walking down the strip:The focus-follow code was what broke focus-follow
That runs synchronously, before React re-renders, so
children[to]reads the pre-move DOM and grabs the neighbour — the tab about to slide into the slot being vacated. React then reorders and carries that neighbour back to the old index, with focus sitting on it.The fix is to delete it. Focus is already on this tab's own element, and the list is keyed by
tab.id, so React reorders by moving that node rather than recreating it. Focus travels with the tab without any help.Why the existing test did not catch it
renderStrip'sonMoveTabis a mock, so the tabs never actually move. Everything that depends on the re-render — focus above all — is invisible to it, and "moves the focused tab with Ctrl+Shift+Arrow" passed happily throughout while asserting only that the callback fired.The new test renders a strip that really reorders, then checks the moved element still holds focus and that a second press carries the same tab onward rather than swapping the pair back.
Verification
document.activeElementis the ARP tab, not the moved Scan tab) and passes with the fix. That is the regression pinned, not just a green suite.tscexit 0,eslintclean.Sequencing note
#286 records this under Known gaps in
ux-walkthrough.md. Whichever of the two lands second needs to move that entry out of Known gaps and into the Managing tabs steps as working behaviour — this branch is cut frommain, so it does not contain that text yet.