Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion public/channel-color-picker.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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() {
Expand All @@ -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) {
Expand Down
41 changes: 41 additions & 0 deletions test-channel-color-picker-e2e.js
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,47 @@ 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.
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
Expand Down
Loading