From 5d2e14aba2b25bdcb77cfd07331b5f35ec0b6b3a Mon Sep 17 00:00:00 2001 From: efiten Date: Thu, 3 Sep 2026 09:42:10 +0200 Subject: [PATCH] fix(#1943): cancel the deferred swatch focus so arrow keys are not undone (#1945) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 `589fa987` and `859173f1`) 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. 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.focus` with a stack trace, on one open: ``` focus(#f97316) @10205ms <- the keydown handler focus(#ef4444) @10208ms <- channel-color-picker.js:146:58 ``` 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: ``` ✗ Enter should assign focused color (#f97316), got #ef4444 ``` ## 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 - The **regression test added here fails on unmodified master** with `a late focus timer must not move focus after the user did` and passes with the fix. - It is **deterministic, not load-dependent**: it reproduces the exact sequence the stack trace identified (open, Escape, reopen, ArrowRight before the timer lands) rather than waiting for contention. It also asserts the Enter path, so the user-visible half is covered and not just focus position. - Full suite: 10 of 10, three consecutive runs. ## 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. Co-authored-by: Claude Opus 5 (1M context) --- public/channel-color-picker.js | 21 +++++++++++- test-channel-color-picker-e2e.js | 58 +++++++++++++++++++++++++++----- 2 files changed, 70 insertions(+), 9 deletions(-) diff --git a/public/channel-color-picker.js b/public/channel-color-picker.js index cd7ddfff..997e778b 100644 --- a/public/channel-color-picker.js +++ b/public/channel-color-picker.js @@ -28,6 +28,9 @@ // overwriting body.style.overflow directly. Without this, two cooperating // surfaces (this picker + SlideOver) corrupt overflow last-writer-wins. var scrollLockToken = null; + // #1943: handle for the deferred focus of the first swatch, so it can be + // cancelled. It used to be fire-and-forget. + var focusTimer = null; function createPopover() { if (popoverEl) return popoverEl; @@ -143,7 +146,20 @@ // Focus first swatch for keyboard accessibility var firstSwatch = el.querySelector('.cc-swatch'); - if (firstSwatch) setTimeout(function() { firstSwatch.focus(); }, 0); + // #1943: this used to be an uncancellable setTimeout(0). It could land + // AFTER the user had already moved focus with an arrow key, snapping the + // selection back to the first swatch, and Enter then assigned the wrong + // colour. Cancel any pending one, and do not steal focus that already sits + // inside the popover or that belongs to a popover since hidden. + if (focusTimer) { clearTimeout(focusTimer); focusTimer = null; } + if (firstSwatch) { + focusTimer = setTimeout(function () { + focusTimer = null; + if (el.style.display === 'none') return; + if (el.contains(document.activeElement)) return; + firstSwatch.focus(); + }, 0); + } // Listen for outside click / Escape setTimeout(function() { @@ -153,6 +169,9 @@ } function hidePopover() { + // #1943: a pending focus timer from this show() must not fire into the + // next one. + if (focusTimer) { clearTimeout(focusTimer); focusTimer = null; } if (popoverEl) popoverEl.style.display = 'none'; currentChannel = null; if (window.__scrollLock && scrollLockToken != null) { diff --git a/test-channel-color-picker-e2e.js b/test-channel-color-picker-e2e.js index 5743610f..5395aa5c 100644 --- a/test-channel-color-picker-e2e.js +++ b/test-channel-color-picker-e2e.js @@ -146,15 +146,15 @@ function assert(c, m) { if (!c) throw new Error(m || 'assertion failed'); } document.activeElement.getAttribute('data-color')); await page.keyboard.press('ArrowRight'); // Wait for focus to actually move rather than reading activeElement on the - // next tick. The keydown handler moves focus, but under CI load that can - // land after Playwright's evaluate has already run, and the assertion then - // compares the swatch against itself: "was #ef4444, now #ef4444". + // next tick. // - // This is the same macrotask race the "outside click" step below documents - // at length for #1317; the fix there was to wait on the real condition - // instead of a proxy, and this step needs it too. Observed failing on the - // master pushes for 589fa987 (2026-08-31) and 859173f1 (2026-09-02), and - // on PR #1884, which passed unchanged on a re-run. + // #1939 added this wait believing the intermittent "was #ef4444, now + // #ef4444" failure was a macrotask race in which the handler had not yet + // run. That was wrong, and #1943 has the measurement: the failing step took + // 16 ms while this wait has a 3 s budget, so it was resolving, not timing + // out. Focus DID move and was then taken back by showPopover's deferred + // focus of the first swatch. The product side is fixed in this PR; the wait + // stays as ordinary defensiveness against a slow handler, not as a cure. try { await page.waitForFunction((prev) => { const el = document.activeElement; @@ -179,6 +179,48 @@ function assert(c, m) { if (!c) throw new Error(m || 'assertion failed'); } 'Enter should assign focused color (' + nextColor + '), got ' + stored); }); + await step('a late focus timer does not snap the selection back (#1943)', async () => { + // Deterministic reproduction of #1943. showPopover() defers focusing the + // first swatch with setTimeout(0). Reopening while a swatch still has focus + // means the test's usual "wait for a focused swatch" is satisfied by the + // OLD focus, so ArrowRight runs before the new timer lands. The timer then + // fired into the popover and pulled focus back to the first swatch, and + // Enter assigned that colour instead of the navigated-to one. Proven with a + // focus() stack trace: the second focus came from channel-color-picker.js:146. + await page.evaluate(() => window.ChannelColorPicker.show('#lateA', 100, 100)); + await page.waitForFunction(() => { + const el = document.activeElement; + return el && el.classList && el.classList.contains('cc-swatch'); + }, { timeout: 2000 }); + await page.keyboard.press('Escape'); + // Reopen and move immediately, without waiting for the new focus timer. + await page.evaluate(() => window.ChannelColorPicker.show('#lateB', 100, 100)); + const before = await page.evaluate(() => + document.activeElement && document.activeElement.getAttribute + ? document.activeElement.getAttribute('data-color') : null); + await page.keyboard.press('ArrowRight'); + const moved = await page.evaluate(() => + document.activeElement.getAttribute('data-color')); + // Give any pending setTimeout(0) more than enough time to land. + await page.waitForTimeout(150); + const settled = await page.evaluate(() => + document.activeElement.getAttribute('data-color')); + assert(settled === moved, + 'a late focus timer must not move focus after the user did (was ' + before + + ', moved to ' + moved + ', settled on ' + settled + ')'); + await page.keyboard.press('Enter'); + await page.waitForFunction(() => { + const el = document.querySelector('.cc-picker-popover'); + return el && el.style.display === 'none'; + }, { timeout: 3000 }); + const stored = await page.evaluate(() => + window.ChannelColors && window.ChannelColors.get('#lateB')); + assert(stored === moved, + 'Enter must assign the swatch the user navigated to (expected ' + moved + + ', got ' + stored + ')'); + await page.evaluate(() => window.ChannelColors.remove('#lateB')); + }); + await step('outside click closes popover', async () => { // De-flake history: #1317 (62a81776) tried `mouse.click(700,500)` + a // `rect.width > 0` "listener installed" proxy. That proxy is FALSE — it