diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 684fd64c..c8cf21a3 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -426,6 +426,26 @@ jobs: CHROMIUM_REQUIRE=1 BASE_URL=http://localhost:13581 node test-channels-ws-race-1498-e2e.js 2>&1 | tee -a e2e-output.txt CHROMIUM_REQUIRE=1 BASE_URL=http://localhost:13581 node test-issue-1487-byop-modal-layout-e2e.js 2>&1 | tee -a e2e-output.txt + # #1616: slide-over focus-restore flake-gate. Runs the slide-over + # E2E 20 consecutive times against the SAME backend instance so + # the Chromium-headless focus race documented in #1172/#1616 has + # a 20× shot at firing. Any single non-zero exit aborts. This is + # the architectural-fix gate — if it ever turns red post-merge, + # the focused-but-hidden state has crept back in. + # + # PERMANENT step. Adds ~3-4 min to the e2e-test job in exchange + # for closing out a flake family that was blocking ~8 unrelated + # PRs at a time. If profiling pressures the budget later, drop + # repeat count first; do not delete. + - name: Slide-over E2E flake-gate (#1616, --repeat-each=20) + run: | + set -e + for i in $(seq 1 20); do + echo "--- slide-over E2E run $i/20 ---" + BASE_URL=http://localhost:13581 node test-slideover-1056-e2e.js 2>&1 | tee -a slideover-repeat-output.txt + done + echo "20 passed" + - name: Collect frontend coverage (parallel) if: success() && github.event_name == 'push' run: | diff --git a/public/live.css b/public/live.css index 99fab451..6e033e5b 100644 --- a/public/live.css +++ b/public/live.css @@ -322,6 +322,18 @@ visibility: hidden; } +/* #1616 — the legend is informational (color swatches + labels). Its only + * interactive child is the corner-reposition button in .panel-header. The + * legend's bounding box overlaps the Leaflet topright Settings cog + * (#liveControlsToggle) on viewports where both pin to the right edge, + * which made Playwright (and real users on touch) miss the cog. Disable + * pointer events on the legend surface and re-enable only on the move + * button so the cog stays clickable through the legend's empty area. + */ +.live-legend { pointer-events: none; } +.live-legend .panel-header, +.live-legend .panel-corner-btn { pointer-events: auto; } + .legend-title { font-size: 9px; font-weight: 700; diff --git a/public/nodes.js b/public/nodes.js index b443e181..548184f1 100644 --- a/public/nodes.js +++ b/public/nodes.js @@ -1393,6 +1393,16 @@ }); const dupMap = buildDupNameMap(_allNodes); + // #1616 followup: capture the focused row's data-key BEFORE we replace + // the tbody — async events (`theme-refresh`, ws node updates, etc.) + // can fire renderRows() while the user is keyboard-navigating. Without + // restoration, focus drops to on every re-render and the + // slide-over close-path focus-restore loses the race. + const tbodyHadFocus = tbody.contains(document.activeElement); + const focusedKey = tbodyHadFocus + ? (document.activeElement.closest('tr[data-key]') || {}).getAttribute && + document.activeElement.closest('tr[data-key]').getAttribute('data-key') + : null; tbody.innerHTML = sorted.map(n => { const roleColor = ROLE_COLORS[n.role] || '#6b7280'; const isClaimed = myKeys.has(n.public_key); @@ -1418,6 +1428,17 @@ var _ndTbl = document.getElementById('nodesTable'); if (_ndTbl) window.TableResponsive.register(_ndTbl); } + // #1616 followup: re-land focus on the replacement with the + // same data-key, if the prior tbody had focus. Without this, an + // async re-render (theme-refresh, ws node update) blurs the row + // the user just keyboard-navigated to. + if (focusedKey) { + try { + const sel = (window.CSS && CSS.escape) ? CSS.escape(focusedKey) : focusedKey; + const fresh = tbody.querySelector('tr[data-key="' + sel + '"]'); + if (fresh) fresh.focus({ preventScroll: true }); + } catch (_) {} + } } /** diff --git a/public/packets.js b/public/packets.js index 2b2f366b..54c63d0a 100644 --- a/public/packets.js +++ b/public/packets.js @@ -257,12 +257,34 @@ // (multiple SlideOver opens reuse the same token; only paired with a // matching release on close). let scrollLockToken = null; + // #1616: open state is now tracked by a flag instead of panel.hidden. + // The architectural close path DETACHES panel + backdrop from the DOM + // (removeChild) — there is no "focused-but-hidden" intermediate state + // for Chromium-headless to race against. See close() below. + let panelOpen = false; + // #1616: list of pending one-shot MutationObservers (per-close-invocation) + // waiting for a row to re-attach so we can land focus on the new instance. + // Tracked so a follow-up close()/open() can disconnect any stragglers. + let pendingFocusObservers = []; + + function disconnectFocusObservers() { + for (var i = 0; i < pendingFocusObservers.length; i++) { + try { pendingFocusObservers[i].disconnect(); } catch (_) {} + } + pendingFocusObservers = []; + } function ensureNodes() { if (panel && backdrop) return; backdrop = document.createElement('div'); backdrop.className = 'slide-over-backdrop'; + // #1616: state is data-state="open|closed"; closed nodes are DETACHED + // from the DOM in close() so panel.hidden never has to be true while + // anything inside (or the panel itself) holds focus. We keep + // backdrop.hidden as a defensive fallback for the brief window after + // ensureNodes() runs before the first open() attaches them. backdrop.hidden = true; + backdrop.setAttribute('data-state', 'closed'); // Backdrop is decorative — assistive tech should not announce it. backdrop.setAttribute('aria-hidden', 'true'); backdrop.addEventListener('click', function () { close(); }); @@ -277,6 +299,7 @@ // is the actual title rendered into the panel. panel.setAttribute('aria-labelledby', 'slideOverTitle'); panel.hidden = true; + panel.setAttribute('data-state', 'closed'); panel.tabIndex = -1; panel.innerHTML = '
' + @@ -315,8 +338,10 @@ try { first.focus(); } catch {} } }); - document.body.appendChild(backdrop); - document.body.appendChild(panel); + // #1616: do NOT attach panel + backdrop to on ensureNodes(). + // They are appended in open() and removed in close() so the closed + // state is "not in the DOM" — Chromium-headless cannot land focus on + // something that isn't there. // Single Escape handler shared across all uses. document.addEventListener('keydown', function (e) { @@ -352,7 +377,10 @@ } function isOpen() { - return !!(panel && !panel.hidden); + // #1616: open state is the panelOpen flag, NOT (!panel.hidden). + // When closed the panel is DETACHED from , so a `.hidden` + // check on a node nothing roots to is meaningless. + return panelOpen; } function open(opts) { @@ -365,6 +393,10 @@ // prior close() can detect that a newer open has happened and skip // its stale focus-restore. openSeq++; + // #1616: a fresh open() invalidates any pending focus-observers + // from a recent close() — disconnect them so they can't land a + // stale focus on row A after row B has opened. + disconnectFocusObservers(); closeCb = typeof opts.onClose === 'function' ? opts.onClose : null; // If the caller passes restoreFocus(), it owns lookup at close-time — // useful when the caller re-renders the row table (which would detach @@ -382,8 +414,16 @@ title.textContent = opts.title || 'Detail'; content = panel.querySelector('.slide-over-content'); content.innerHTML = ''; + // #1616: ATTACH panel + backdrop now. They were removed (or never + // appended) by the prior close(). Order matters — backdrop first so + // it sits beneath the panel in source/paint order. + if (backdrop.parentNode !== document.body) document.body.appendChild(backdrop); + if (panel.parentNode !== document.body) document.body.appendChild(panel); backdrop.hidden = false; panel.hidden = false; + backdrop.setAttribute('data-state', 'open'); + panel.setAttribute('data-state', 'open'); + panelOpen = true; // Focus the close button so Esc/Enter works without an extra tab. const x = panel.querySelector('.slide-over-close'); if (x) try { x.focus(); } catch {} @@ -391,9 +431,7 @@ } function close() { - if (!panel || panel.hidden) return; - panel.hidden = true; - if (backdrop) backdrop.hidden = true; + if (!panel || !panelOpen) return; // #1168 Munger #3: release the ref-counted scroll-lock token. if (scrollLockToken != null) { window.__scrollLock.release(scrollLockToken); @@ -401,63 +439,140 @@ } const cb = closeCb; closeCb = null; - if (content) content.innerHTML = ''; - // Restore focus to whatever opened us (typically the table row), so - // keyboard users don't get dumped at the top of the document. - let toFocus = prevFocus; const resolver = prevFocusResolver; + let toFocus = prevFocus; prevFocus = null; prevFocusResolver = null; - // #1168 Munger #1: capture the open-sequence at close-time. If a NEW - // open() happens before our deferred rAF fires, openSeq will have - // advanced past this value and the stale rAF must no-op (otherwise - // it would steal focus back to row A's originating row AFTER row B - // is open — clobbering B's focus). const seqAtClose = openSeq; + + // Fire the user's onClose BEFORE we touch focus. Callers typically + // re-render the originating table inside cb() (e.g. clearing the + // selected row state), which detaches the originating ; we + // intentionally re-look-up via the resolver below. if (cb) try { cb(); } catch {} - // Resolver runs AFTER cb (cb may re-render the table and reattach the row). + + // Resolve the focus target by DATA-VALUE LOOKUP AT RESTORE TIME. + // #1616: no captured-element-ref path — if cb() re-rendered the + // tbody, the captured prevFocus is now an orphaned node detached + // from the document, and Chromium-headless will silently swallow + // a .focus() call on it (activeElement falls back to ). if (resolver) { try { const resolved = resolver(); if (resolved) toFocus = resolved; - } catch {} + } catch (_) {} } - if (toFocus && typeof toFocus.focus === 'function' && document.body.contains(toFocus)) { - // Defer to next microtask + rAF so the focus call lands AFTER any - // event-handler bookkeeping (e.g. an Escape keydown chain that would - // otherwise see focus snap back to as the key event unwinds). - const tryFocus = function () { - // Munger #1: bail if a newer open() has happened since close-time. - if (openSeq !== seqAtClose) return; - let t = toFocus; - if (resolver) { - try { - const fresh = resolver(); - if (fresh) t = fresh; - } catch (_) {} - } - // MINOR fix: don't steal focus if the user already focused another input - if (document.activeElement && - document.activeElement !== document.body && - document.activeElement !== t && - !panel.contains(document.activeElement)) { + // #1616 (architectural): SYNCHRONOUSLY focus the target row BEFORE + // the panel/backdrop are detached. This eliminates the + // focused-but-hidden intermediate state Chromium-headless reasons + // about non-deterministically. If activeElement is inside the panel + // when the panel detaches, focus drops to ; landing focus on + // the row first means the detach is a no-op for the active element. + // The deferred microtask + rAF + observer chain below handles the + // case where cb() queued an async re-render that detaches this row. + if (toFocus && typeof toFocus.focus === 'function' && document.body.contains(toFocus)) { + try { toFocus.focus({ preventScroll: true }); } catch (_) {} + } + + // DETACH panel + backdrop. After this point, there is no "panel + // node sitting hidden in the document tree" for Chromium to race + // a stale blur against. + panelOpen = false; + panel.setAttribute('data-state', 'closed'); + backdrop.setAttribute('data-state', 'closed'); + if (content) content.innerHTML = ''; + panel.hidden = true; + backdrop.hidden = true; + if (panel.parentNode) panel.parentNode.removeChild(panel); + if (backdrop.parentNode) backdrop.parentNode.removeChild(backdrop); + + // If sync focus landed: schedule a microtask re-check. The + // caller's cb() may have queued an ASYNC re-render (microtask + // or rAF) that detaches the row AFTER our sync focus() — in that + // case activeElement falls to AFTER this function returns, + // and we need a second-pass focus restore. queueMicrotask() runs + // AFTER any Promise.resolve().then(...) the caller queued from + // inside cb() (FIFO microtask queue), so re-resolving here picks + // up the NEW row instance. + if (!resolver) { + // No resolver = no second-pass capability; best-effort done. + return; + } + + // Always run the deferred re-check pass, even if sync focus + // appeared to land — it may be on a soon-to-be-detached orphan. + function deferredRecheck() { + // Stale guard — newer open() bumped openSeq; bail. + if (openSeq !== seqAtClose) return; + // If focus already landed on something non-body that matches + // the resolver, we're done. + let fresh = null; + try { fresh = resolver(); } catch (_) {} + if (fresh && document.body.contains(fresh)) { + if (document.activeElement === fresh) return; + try { fresh.focus({ preventScroll: true }); } catch (_) {} + if (document.activeElement === fresh) return; + } + // Still not landed. Attach a one-shot MutationObserver rooted + // at document.body — broader than the prior tbody-rooted + // observer (which fails when the tbody itself is innerHTML- + // swapped), still bounded by a short timeout below. + attachBodyObserver(); + } + + function attachBodyObserver() { + const obs = new MutationObserver(function () { + if (openSeq !== seqAtClose) { + obs.disconnect(); + const idx = pendingFocusObservers.indexOf(obs); + if (idx >= 0) pendingFocusObservers.splice(idx, 1); return; } - - if (t && document.body.contains(t)) { - try { t.focus({ preventScroll: true }); } catch (_) {} + let target = null; + try { target = resolver(); } catch (_) {} + if (!target || !document.body.contains(target)) return; + try { target.focus({ preventScroll: true }); } catch (_) {} + if (document.activeElement === target) { + obs.disconnect(); + const idx = pendingFocusObservers.indexOf(obs); + if (idx >= 0) pendingFocusObservers.splice(idx, 1); } - }; - - requestAnimationFrame(function () { - if (document.activeElement && panel.contains(document.activeElement)) { - try { document.activeElement.blur(); } catch (_) {} - } - tryFocus(); - setTimeout(tryFocus, 10); }); + // #1616 followup: root = document.body. The prior 'smart' tbody + // root logic broke when cb() replaced the entire tbody via + // innerHTML — the observer was watching a node that no longer + // received the mutation we cared about. document.body is broader + // but correctly catches the re-attachment in all known callsites + // (nodes/packets/observers all swap rows under tbody under body). + obs.observe(document.body, { childList: true, subtree: true }); + pendingFocusObservers.push(obs); + // Tight timeout — 200ms is enough for any sync or microtask/rAF + // re-render to commit. Longer (e.g. the prior 2s) creates new + // races where the observer fires AFTER a subsequent open() and + // steals focus from row B back to row A. + setTimeout(function () { + obs.disconnect(); + const idx = pendingFocusObservers.indexOf(obs); + if (idx >= 0) pendingFocusObservers.splice(idx, 1); + }, 200); } + + // Step 1: microtask — runs AFTER cb's Promise.resolve().then(...) + // re-renders, BEFORE next animation frame. Catches the common + // async-render path. + queueMicrotask(function () { + if (openSeq !== seqAtClose) return; + let fresh = null; + try { fresh = resolver(); } catch (_) {} + if (fresh && document.body.contains(fresh) + && document.activeElement !== fresh) { + try { fresh.focus({ preventScroll: true }); } catch (_) {} + } + // Step 2: rAF — runs after layout commits. Catches re-renders + // that wait on rAF or that resolve only after style/layout work. + requestAnimationFrame(deferredRecheck); + }); } // If the viewport grows past the breakpoint while open, close the slide-over diff --git a/test-slideover-1056-e2e.js b/test-slideover-1056-e2e.js index 3581c4b5..7362cca1 100644 --- a/test-slideover-1056-e2e.js +++ b/test-slideover-1056-e2e.js @@ -459,15 +459,11 @@ const PAGES = [ }); // ------------------------------------------------------------------ - // SKIP: tracked in #1172 — flaky in CI Chromium, see issue for repro. - // X-click focus-restore is real and works locally; head-to-head with - // headless CI flake. Soft-warn pattern was removed (#1168 Munger #4): - // skipped tests are VISIBLE in CI output (↷ SKIP), not silently - // swallowed by `if (!cond) console.warn(...)`. Hard assertions - // preserved below — flip step.skip → step once #1172 ships a fix. + // #1616: hard-asserted (architectural fix in SlideOver.close — detach + // panel on close, MutationObserver fallback for late-attaching rows). + // Supersedes the #1172 soften-and-track approach. // ------------------------------------------------------------------ - step.skip('focus-restore@800: X-button click returns focus to originating row', - 'tracked in #1172 — flaky in CI Chromium', async () => { + await step('focus-restore@800: X-button click returns focus to originating row', async () => { const rowKey = await openPanelFromRow(); await page.evaluate(() => { const x = document.querySelector('.slide-over-panel .slide-over-close'); @@ -486,6 +482,82 @@ const PAGES = [ assert(r.isActive, 'focus did NOT restore to originating row after X click: ' + JSON.stringify(r)); }); + // ------------------------------------------------------------------ + // #1616 followup: SYNTHETIC deterministic repro for the Escape-path + // focus-restore race. The natural test above flakes ~50% of the time + // in Chromium-headless because the nodes module's onClose calls + // `renderRows()` synchronously and the originating instance is + // replaced — but the exact micro-timing between focus(), the panel + // detach, and the synchronous innerHTML swap is racy: under some + // schedules Chromium drops activeElement to before the + // replacement is laid out. + // + // This test forces the race deterministically by driving SlideOver + // directly with an onClose that defers the row swap to a microtask + // (Promise.resolve().then(...)). Resolver runs synchronously at + // close-time and returns the OLD (still-attached) tr — but by the + // time the microtask fires, the OLD tr is detached and Chromium + // re-derives activeElement = . The MutationObserver fallback + // SHOULD pick up the new row attachment and re-land focus. + // ------------------------------------------------------------------ + await step('focus-restore@800: Escape with async-cb re-render (synthetic)', async () => { + // Build an isolated test surface: a tbody with a single row and a + // resolver/closer pair driven through SlideOver.open() directly. + // We rebuild the row from scratch in a microtask after close() to + // exactly mirror the production async-render race. + await page.evaluate(() => { + // Clean any prior fixture so re-runs are deterministic. + const old = document.getElementById('synth-1616'); + if (old) old.remove(); + const wrap = document.createElement('div'); + wrap.id = 'synth-1616'; + wrap.innerHTML = '' + + '' + + '
A
'; + document.body.appendChild(wrap); + }); + // Focus the row, open SlideOver with a microtask-delayed onClose. + await page.evaluate(() => { + const tbody = document.getElementById('synth-tbody'); + const row = tbody.querySelector('tr[data-value="row-A"]'); + row.focus(); + window.SlideOver.open({ + title: 'Synthetic', + restoreFocus: function () { + return document.querySelector('#synth-tbody tr[data-value="row-A"]'); + }, + onClose: function () { + // Defer the row swap to a microtask so it lands AFTER + // close()'s synchronous focus + detach. This is the precise + // race: resolver() returned the OLD tr (still attached at + // close() time); the microtask then replaces it. + Promise.resolve().then(function () { + tbody.innerHTML = 'A-new'; + }); + } + }); + }); + // Slide-over is open. Press Escape. + await page.keyboard.press('Escape'); + // Wait for the new row to mount AND focus to land on it. + await page.waitForFunction(() => { + const row = document.querySelector('#synth-tbody tr[data-value="row-A"]'); + return !!row && document.activeElement === row; + }, null, { timeout: 1500 }).catch(() => {}); + const r = await page.evaluate(() => { + const row = document.querySelector('#synth-tbody tr[data-value="row-A"]'); + return { + rowExists: !!row, + isActive: !!row && document.activeElement === row, + rowText: row && row.textContent, + activeTag: document.activeElement && document.activeElement.tagName, + }; + }); + assert(r.rowExists, 'synthetic row vanished after async cb'); + assert(r.rowText === 'A-new', 'synthetic row was not re-rendered by microtask (got: ' + r.rowText + ')'); + assert(r.isActive, 'focus did NOT restore to NEW synthetic row after async cb: ' + JSON.stringify(r)); + }); + await ctx.close(); } @@ -608,42 +680,17 @@ const PAGES = [ assert(after.backdropGone, 'backdrop still shown after viewport crossed BP'); assert(!after.bodyHasLockClass, 'scroll-locked class still present after viewport crossed BP'); assert(after.bodyOverflow !== 'hidden', 'body scroll-lock not released after viewport crossed BP (overflow=' + after.bodyOverflow + ')'); - // Focus-restore portion of this scenario is exercised in the - // skipped step below (tracked in #1172). Soft-warn pattern removed - // per #1168 Munger #4 — skipped is visible, soft-warn was not. + // #1616: focus-restore portion is now hard-asserted (architectural + // fix in SlideOver.close — panel detach + MutationObserver fallback). + assert(after.rowExists, 'originating row vanished after viewport-crossing close'); + assert(after.focusRestored, 'focus did NOT restore to originating row after viewport-crossing close: ' + JSON.stringify(after)); }); - // ------------------------------------------------------------------ - // SKIP: tracked in #1172 — flaky in CI Chromium, see issue for repro. - // Same root cause as the X-click case above. Cleanup checks - // (panel/backdrop/scroll-lock) are HARD in the step above; only the - // focus identity check is skipped. Restore by flipping step.skip → - // step once #1172 ships a fix. - // ------------------------------------------------------------------ - step.skip('resize@800→1440 nodes: focus restored after viewport-crossing close', - 'tracked in #1172 — flaky in CI Chromium', async () => { - const rowKey = await page.evaluate(() => { - const r = document.querySelector('#nodesTable tbody tr[data-value]'); - if (!r) return null; - r.focus(); - r.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, view: window })); - return r.getAttribute('data-value'); - }); - assert(rowKey, 'no nodes row for resize focus test'); - await page.waitForFunction(() => { - const p = document.querySelector('.slide-over-panel'); - return p && !p.hidden; - }, null, { timeout: 8000 }); - await page.setViewportSize({ width: 1440, height: 900 }); - await page.waitForTimeout(500); - const after = await page.evaluate((key) => { - const esc = (window.CSS && CSS.escape) ? CSS.escape(key) : key; - const row = document.querySelector('#nodesTable tbody tr[data-value="' + esc + '"]'); - return { rowExists: !!row, focusRestored: !!row && document.activeElement === row }; - }, rowKey); - assert(after.rowExists, 'originating row vanished'); - assert(after.focusRestored, 'focus not restored after viewport-crossing close'); - }); + // #1616: the formerly-skipped duplicate focus-only test has been merged + // into the cleanup assertion above (single open/resize cycle). The + // separate test was unreachable anyway — by the time it ran the viewport + // was already 1440, so SlideOver.shouldUse() returned false and the + // panel never opened. await ctx.close(); }