fix(#1943): cancel the deferred swatch focus so arrow keys are not undone - #1945
Merged
Merged
Conversation
… are not undone The colour picker's keyboard navigation does not work, and the E2E flake that has been failing unrelated PRs (Kpa-clawbot#1940, Kpa-clawbot#1941, and master pushes 589fa98 and 859173f) was reporting it correctly. Cause. showPopover deferred focusing the first swatch with an uncancellable setTimeout(..., 0) at channel-color-picker.js:146, and nothing cleared it on hide: the file contained zero clearTimeout calls. When the popover is reopened while a swatch still holds focus, that timer lands after the user has already pressed an arrow key, and pulls focus back to the first swatch. Proven rather than argued. Instrumenting HTMLElement.prototype.focus with a stack trace gives, on one open: focus(#f97316) @10205ms from the keydown handler focus(#ef4444) @10208ms from channel-color-picker.js:146:58 Three milliseconds apart. The user-visible consequence is worse than a flaky test: arrow to a colour, press Enter, and the FIRST colour is assigned. The existing suite showed it as "Enter should assign focused color (#f97316), got #ef4444" once the timing was held still. This is why the test failed intermittently: the revert happens on every open, and only whether the assertion reads before or after it varies. Kpa-clawbot#1939 assumed a race in which focus had not yet moved and added a wait for it; that wait resolved successfully and then the value was reverted underneath it, so it never helped. Its comment is corrected here. Fix. Keep a handle for the timer, cancel a pending one on both show and hide, and inside it do nothing when the popover has since been hidden or when focus already sits inside it. A fresh open still focuses the first swatch; an open that inherits focus, or a user who has already navigated, is left alone. Verified: the regression test added here fails on unmodified master ("a late focus timer must not move focus after the user did") and passes with the fix, and it is deterministic rather than load-dependent, reproducing the sequence (open, Escape, reopen, ArrowRight before the timer lands) that the stack trace identified. Full suite 10 of 10 across 3 consecutive runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PQS3XLoPD98yu9pxdRujqg
This was referenced Sep 3, 2026
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.
Closes #1943. The colour picker's keyboard navigation is broken, and the E2E flake that has been failing unrelated PRs (#1940, #1941, and master pushes
589fa987and859173f1) was reporting it correctly.Cause
showPopoverdeferred focusing the first swatch with an uncancellablesetTimeout(..., 0)atchannel-color-picker.js:146, and nothing cleared it on hide. The file contained zeroclearTimeoutcalls. Reopen the popover while a swatch still holds focus and that timer lands after the user has already pressed an arrow key, pulling focus back to the first swatch.Proven, not argued. Instrumenting
HTMLElement.prototype.focuswith a stack trace, on one open:Three milliseconds apart.
The user-visible bug
Worse than a flaky test. Open the picker, arrow to a colour, press Enter, and the first colour is assigned instead of the one you chose. Holding the timing still made the existing suite say so directly:
Why the test looked flaky
The revert happens on every open. Only whether the assertion reads before or after it varies, which is why an idle machine passes and a loaded runner does not.
#1939 (mine) assumed the opposite: a race in which the handler had not yet moved focus, cured by waiting for it. #1943 has the measurement that disproves it. The failing step took 16 ms while that wait has a 3 second budget, so the wait was resolving successfully and then the value was reverted underneath it. It never helped. Its comment is corrected in this PR rather than left to mislead the next reader.
Fix
Keep a handle for the timer, cancel a pending one on both show and hide, and inside it do nothing when the popover has since been hidden or when focus already sits inside it.
A fresh open still focuses the first swatch, which is what the accessibility behaviour is for. An open that inherits focus, or a user who has already navigated, is left alone.
Verification
a late focus timer must not move focus after the user didand passes with the fix.Note on the other flake
This is one of two E2E failures blocking the queue. The other, #1925, is a different mechanism in a different file and is fixed separately in #1944. Together they should leave the E2E suite deterministic again.
Same shape as @TeTeHacko's finding in #1940: something is operable before its setup has finished. That is now three instances in this codebase, so it may be worth a look as a pattern rather than three separate fixes.