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