diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index e7b0168a4..a98fb23ce 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -451,6 +451,7 @@ jobs: BASE_URL=http://localhost:13581 node test-issue-1128-packets-layout-e2e.js 2>&1 | tee -a e2e-output.txt BASE_URL=http://localhost:13581 node test-issue-189-group-caret-e2e.js 2>&1 | tee -a e2e-output.txt BASE_URL=http://localhost:13581 node test-issue-254-affinity-toggle-mobile-aria-e2e.js 2>&1 | tee -a e2e-output.txt + BASE_URL=http://localhost:13581 node test-issue-259-mobile-expanded-group-e2e.js 2>&1 | tee -a e2e-output.txt BASE_URL=http://localhost:13581 node test-issue-1128-multi-viewport-e2e.js 2>&1 | tee -a e2e-output.txt BASE_URL=http://localhost:13581 node test-issue-1136-live-region-e2e.js 2>&1 | tee -a e2e-output.txt BASE_URL=http://localhost:13581 node test-live-multibyte-only-e2e.js 2>&1 | tee -a e2e-output.txt diff --git a/public/nodes.js b/public/nodes.js index 7ac57a8f2..745abb854 100644 --- a/public/nodes.js +++ b/public/nodes.js @@ -582,6 +582,32 @@ let directNode = null; // set when navigating directly to #/nodes/:pubkey + // #259: the full-screen node view's Escape-to-go-back handler. Module level + // so every init() registers the same reference and destroy() can remove it. + function _nodesEsc(e) { + if (e.key === 'Escape') { + document.removeEventListener('keydown', _nodesEsc); + location.hash = '#/nodes'; + } + } + + // #259: the list view's Escape-to-close-the-detail-panel handler, same class + // of leak. It used to be a fresh closure registered inside renderLeft(), and + // renderLeft() runs on every load of the list -- a visit, a region change, a + // filter change -- so the listeners stacked and were never removed. Module + // level means a repeat add is a DOM no-op and destroy() can take it off. + function _nodesPanelEsc(e) { + if (e.key !== 'Escape') return; + const panel = document.getElementById('nodesRight'); + if (!panel || panel.classList.contains('empty')) return; + closeDetailView(); + panel.classList.add('empty'); + panel.innerHTML = 'Select a node to view details'; + selectedKey = null; + history.replaceState(null, '', '#/nodes'); + renderRows(); + } + let regionChangeHandler = null; function init(app, routeParam) { @@ -606,13 +632,12 @@ // would stack listeners. document.getElementById('nodeFullBody').addEventListener('click', onFullBodyClick); loadFullNode(directNode); - // Escape to go back to nodes list - document.addEventListener('keydown', function nodesEsc(e) { - if (e.key === 'Escape') { - document.removeEventListener('keydown', nodesEsc); - location.hash = '#/nodes'; - } - }); + // Escape to go back to nodes list. #259: one listener, not one per visit. + // It used to be a fresh closure per init() that only unhooked itself when + // Escape fired, so node A -> B -> C stacked three and one Escape wrote the + // same hash three times. _nodesEsc is a stable module-level reference, so + // a repeat add is a DOM no-op, and destroy() takes it off again. + document.addEventListener('keydown', _nodesEsc); return; } @@ -1250,6 +1275,11 @@ function destroy() { if (wsHandler) offWS(wsHandler); wsHandler = null; + // #259: both Escape handlers are document-level, so they outlive the + // page's DOM unless they are removed here -- _nodesEsc for the + // full-screen view, _nodesPanelEsc for the list view. + document.removeEventListener('keydown', _nodesEsc); + document.removeEventListener('keydown', _nodesPanelEsc); closeDetailView(); if (regionChangeHandler) RegionFilter.offChange(regionChangeHandler); regionChangeHandler = null; @@ -1664,20 +1694,10 @@ tbody.addEventListener('keydown', handler); } - // Escape to close node detail panel - document.addEventListener('keydown', function nodesPanelEsc(e) { - if (e.key === 'Escape') { - const panel = document.getElementById('nodesRight'); - if (panel && !panel.classList.contains('empty')) { - closeDetailView(); - panel.classList.add('empty'); - panel.innerHTML = 'Select a node to view details'; - selectedKey = null; - history.replaceState(null, '', '#/nodes'); - renderRows(); - } - } - }); + // Escape to close node detail panel. #259: one listener, not one per + // render -- _nodesPanelEsc is a stable module-level reference, so this add + // is a no-op on every render after the first, and destroy() removes it. + document.addEventListener('keydown', _nodesPanelEsc); // #630: Close button for node detail panel (important for mobile full-screen overlay) document.getElementById('nodesRight').addEventListener('click', function(e) { diff --git a/public/packets.js b/public/packets.js index af6933528..4d89d8c64 100644 --- a/public/packets.js +++ b/public/packets.js @@ -2428,6 +2428,18 @@ return !!(window.MobilePageActions && window.MobilePageActions.isMobile()); } + // #259: a group expanded on desktop used to keep its child rows after the + // layout crossed to the mobile mode, where the expand column is hidden and + // the row only selects — nothing was left to collapse it with (the dead end + // upstream #1461 #7 describes). The hash stays in expandedHashes, so the + // children come back on desktop and a phone rotation does not throw the + // state away; it is only the rendered slice that leaves them out. This also + // covers the first render at a narrow width, e.g. the #866 deep link + // #/packets//, which expands the hash before any render. + function groupIsExpandedInView(hash) { + return expandedHashes.has(hash) && !groupRowSelectsOnActivate(); + } + // A group row's action and aria-expanded depend on that breakpoint, so a // resize that crosses it (e.g. a phone rotating) re-renders the visible rows. let _groupRowsSelect = false; @@ -2439,6 +2451,9 @@ if (selects === _groupRowsSelect) return; _groupRowsSelect = selects; if (!_displayGrouped) return; + // #259: crossing the breakpoint changes how many DOM rows an expanded + // group produces, so the cached per-entry counts are stale. + _invalidateRowCounts(); _lastVisibleStart = -1; _lastVisibleEnd = -1; renderVisibleRows(); @@ -2447,7 +2462,7 @@ // Build HTML for a single grouped packet row function buildGroupRowHtml(p, entryIdx = -1) { - const isExpanded = expandedHashes.has(p.hash); + const isExpanded = groupIsExpandedInView(p.hash); let headerObserverId = p.observer_id; let headerPathJson = p.path_json; if (_observerFilterSet && p._children?.length) { @@ -2593,7 +2608,7 @@ // Used by both row counting and renderVisibleRows to avoid divergence (#424). function _getRowCount(p) { if (!_displayGrouped) return 1; - if (!expandedHashes.has(p.hash) || !p._children) return 1; + if (!groupIsExpandedInView(p.hash) || !p._children) return 1; let childCount = p._children.length; if (_observerFilterSet) { childCount = p._children.filter(c => _observerFilterSet.has(String(c.observer_id))).length; @@ -4353,6 +4368,12 @@ _setPackets: function(p) { packets = p; }, _setFilter: function(k, v) { filters[k] = v; }, _setExpanded: function(hash, on) { if (on) expandedHashes.add(hash); else expandedHashes.delete(hash); }, + // #259: at <= 600 px groupIsExpandedInView() renders a hash that *is* in + // expandedHashes exactly like a collapsed row, so the DOM alone can no + // longer tell "did not expand" from "expanded invisibly". The mobile E2E + // assertions read the set itself through this. + _isExpanded: function(hash) { return expandedHashes.has(hash); }, + _setDisplayGrouped: function(on) { _displayGrouped = !!on; }, }; } diff --git a/test-all.sh b/test-all.sh index 99f429be6..6f57b31bd 100755 --- a/test-all.sh +++ b/test-all.sh @@ -60,6 +60,7 @@ run test-url-state.js run test-node-adverts.js run test-issue-254-affinity-debug-toggle.js run test-issue-258-column-widths.js +run test-issue-259-nodes-esc-listener.js run test-perf-go-runtime.js run test-channel-psk-ux.js run test-channel-sidebar-layout.js diff --git a/test-issue-254-affinity-debug-toggle.js b/test-issue-254-affinity-debug-toggle.js index 7e388e0ae..9c98754ee 100644 --- a/test-issue-254-affinity-debug-toggle.js +++ b/test-issue-254-affinity-debug-toggle.js @@ -155,6 +155,45 @@ test('collapsed shows exactly one caret, caret-right', () => { assert.deepStrictEqual(caretsIn(html), ['caret-right'], 'no other caret in the card'); }); +// #259 (3): the cases above all exercise renderAffinityDebugCard() in isolation, +// so they stay green if the loadFullNode template keeps the old inline-onclick +// card and the renderer is merely exported. The review of #255 showed that +// mutant passing 9/9 here while only the E2E caught it. These three read the +// source so the fast layer catches it too. +console.log('\n=== #259 the node page actually uses the renderer ==='); + +const SRC = fs.readFileSync('public/nodes.js', 'utf8'); +const CARD_ID = 'id="node-affinity-debug"'; + +test('#259: the loadFullNode template interpolates ${renderAffinityDebugCard()}', () => { + assert( + SRC.includes('${renderAffinityDebugCard()}'), + 'public/nodes.js must render the card through the renderer, not inline markup' + ); +}); + +test('#259: the card markup lives only in renderAffinityDebugCard()', () => { + const n = SRC.split(CARD_ID).length - 1; + assert.strictEqual(n, 1, 'exactly one ' + CARD_ID + ' in public/nodes.js, found ' + n); + const body = /function renderAffinityDebugCard\(\)\s*\{[\s\S]*?\n \}/.exec(SRC); + assert(body, 'renderAffinityDebugCard() is defined in public/nodes.js'); + assert(body[0].includes(CARD_ID), 'that one occurrence is the renderer\'s own'); +}); + +test('#259: no inline on*= handler in the Affinity Debug card source', () => { + // A window around every occurrence of the card id, so a reinstated inline + // onclick on the heading is caught wherever in the file it sits. + let from = 0, i; + while ((i = SRC.indexOf(CARD_ID, from)) !== -1) { + const window_ = SRC.slice(Math.max(0, i - 200), i + 800); + const handlers = window_.match(/\son[a-z]+\s*=\s*["'`]/gi); + assert(!handlers, 'inline handler(s) near ' + CARD_ID + ': ' + JSON.stringify(handlers)); + from = i + CARD_ID.length; + } + const affinityOnclick = /onclick="[^"]*affinity/i.exec(SRC); + assert(!affinityOnclick, 'inline affinity handler: ' + (affinityOnclick && affinityOnclick[0])); +}); + console.log('\n=== #254 Affinity Debug card: toggle ==='); test('the toggle opens the body: aria-expanded true, caret-down', () => { diff --git a/test-issue-254-affinity-toggle-mobile-aria-e2e.js b/test-issue-254-affinity-toggle-mobile-aria-e2e.js index b85361ba6..a2a1276ad 100644 --- a/test-issue-254-affinity-toggle-mobile-aria-e2e.js +++ b/test-issue-254-affinity-toggle-mobile-aria-e2e.js @@ -13,8 +13,11 @@ * the "Seed grouped-packet row for #1486" step in deploy.yml): * - 1400 px: the #189 toggle, data-action="toggle-select" with aria-expanded; * - 390 px (touch): mobile-page-actions.js (#1461 #7) makes activation - * select, so the row is select-hash without aria-expanded, and a tap or - * Enter opens the detail sheet instead of expanding the group; + * select, so the row is select-hash without aria-expanded, and a tap, + * Enter or Space (#259 item 4) opens the detail sheet instead of + * expanding the group -- checked both through expandedHashes (the test + * hook, because at 390 px an expanded group renders exactly like a + * collapsed one) and by resizing back to 1400 px; * - resizing 1400 → 390 → 1400 re-renders the row for each side. * * Usage: BASE_URL=http://localhost:13581 node test-issue-254-affinity-toggle-mobile-aria-e2e.js @@ -126,19 +129,35 @@ async function affinityCard(browser, theme) { // --- 2. Packets group rows --------------------------------------------------- +// #259: since groupIsExpandedInView(), a hash in expandedHashes renders +// exactly like a collapsed row at <= 600 px -- no `expanded` class, no child +// rows. The DOM therefore cannot distinguish "did not expand" from "expanded +// invisibly", so the mobile steps below read expandedHashes itself through +// packets.js's test hook. `inExpandedHashes` is the string 'NO-HOOK' if the +// hook is gone, so a missing hook fails the assertion instead of passing it. async function rowState(page) { return page.evaluate(([sel, hash]) => { const tr = document.querySelector(sel); if (!tr) return null; + const api = window._packetsTestAPI; return { action: tr.getAttribute('data-action'), aria: tr.getAttribute('aria-expanded'), expanded: tr.classList.contains('expanded'), children: document.querySelectorAll('#pktBody tr.group-child[data-parent-hash="' + hash + '"]').length, + inExpandedHashes: api && typeof api._isExpanded === 'function' ? api._isExpanded(hash) : 'NO-HOOK', }; }, [ROW, SEED_HASH]); } +// "Activating the row selected it and did not expand the group": nothing +// visible, and nothing in expandedHashes either. +function assertNotExpanded(s, what) { + assert(s, what + ': no row'); + assert(!s.expanded && s.children === 0, what + ' left no expansion on screen: ' + JSON.stringify(s)); + assert(s.inExpandedHashes === false, what + ' did not expand the group: ' + JSON.stringify(s)); +} + const isDesktopRow = (s) => !!s && s.action === 'toggle-select' && s.aria === 'false'; const isMobileRow = (s) => !!s && s.action === 'select-hash' && s.aria === null; @@ -271,7 +290,7 @@ async function openPackets(page) { return !!s && s.classList.contains('open'); }, null, { timeout: 8000 }); const s = await rowState(page); - assert(s && !s.expanded && s.children === 0, 'the group did not expand: ' + JSON.stringify(s)); + assertNotExpanded(s, 'the tap'); assert(isMobileRow(s), 'still the mobile row after the re-render: ' + JSON.stringify(s)); await shot(page, 'packets-390-sheet'); await page.click('#mobileSheetClose').catch(() => {}); @@ -286,7 +305,35 @@ async function openPackets(page) { return !!s && s.classList.contains('open'); }, null, { timeout: 8000 }); const s = await rowState(page); - assert(s && !s.expanded && s.children === 0, 'Enter did not expand the group: ' + JSON.stringify(s)); + assertNotExpanded(s, 'Enter'); + }); + + // #259 (4): the row handler treats Enter and Space the same + // (packets.js, the keydown branch); the original #254 E2E only pressed + // Enter, so a mutant that dropped Space went unnoticed here. + await step('390 px: Space on the focused row selects too, it does not expand', async () => { + await page.evaluate(() => { const s = document.getElementById('mobileDetailSheet'); if (s) s.classList.remove('open'); }); + await page.focus(ROW); + await page.keyboard.press('Space'); + await page.waitForFunction(() => { + const s = document.getElementById('mobileDetailSheet'); + return !!s && s.classList.contains('open'); + }, null, { timeout: 8000 }); + const s = await rowState(page); + assertNotExpanded(s, 'Space'); + assert(isMobileRow(s), 'still the mobile row after Space: ' + JSON.stringify(s)); + }); + + // The same conclusion at the render level, independently of the hook: a + // group that one of the three activations above expanded silently shows up + // as soon as the layout goes back over the breakpoint -- which is what a + // user rotating the phone to landscape would see. + await step('back at 1400 px the group is still collapsed, so nothing expanded it', async () => { + await page.evaluate(() => { const s = document.getElementById('mobileDetailSheet'); if (s) s.classList.remove('open'); }); + await page.setViewportSize({ width: 1400, height: 900 }); + const s = await waitRow(page, isDesktopRow, 'desktop'); + assert(!s.expanded && s.children === 0, 'no child rows came back: ' + JSON.stringify(s)); + assert(s.inExpandedHashes === false, 'and the hash never entered expandedHashes: ' + JSON.stringify(s)); }); await ctx.close(); } diff --git a/test-issue-259-mobile-expanded-group-e2e.js b/test-issue-259-mobile-expanded-group-e2e.js new file mode 100644 index 000000000..a538aa931 --- /dev/null +++ b/test-issue-259-mobile-expanded-group-e2e.js @@ -0,0 +1,212 @@ +/** + * E2E (#259 item 1): a packet group expanded on desktop must not leave visible + * child rows behind when the layout crosses to the mobile breakpoint. + * + * At <= 600 px mobile-page-actions.js (#1461 #7) hides the expand column and + * makes activating a group row select it, and #255 renders the row as + * select-hash without aria-expanded. Before this fix the row still carried + * class="expanded" and its child rows stayed on screen with nothing left to + * collapse them: the dead end upstream #1461 #7 describes. + * + * The fix keeps the hash in expandedHashes but leaves the children out of the + * rendered slice while the mobile mode is active, so: + * - 1400 px: expanding shows the children and aria-expanded="true" (#248/#255); + * - 390 px: no child rows, no expanded class, no aria-expanded; + * - back at 1400 px: the children and the expanded state are back. + * + * The touch block at the end checks the other direction: at 390 px an expanded + * group renders exactly like a collapsed one, so "activating the row did not + * expand it" is read off expandedHashes through packets.js's test hook and + * confirmed by resizing back over the breakpoint. + * + * Runs against the fixture's seeded 3-observation group (see the "Seed + * grouped-packet row for #1486" step in deploy.yml). + * + * Usage: BASE_URL=http://localhost:13581 node test-issue-259-mobile-expanded-group-e2e.js + * SCREENSHOT_DIR= also saves screenshots (390 px and 1400 px, light and dark). + */ +'use strict'; +const path = require('path'); +const { chromium } = require('playwright'); + +const BASE = process.env.BASE_URL || 'http://localhost:13581'; +const SHOTS = process.env.SCREENSHOT_DIR || ''; +const SEED_HASH = 'fae0c9e6d357a814'; +const ROW = `#pktBody tr[data-hash="${SEED_HASH}"]`; + +let passed = 0, failed = 0; +async function step(name, fn) { + try { await fn(); passed++; console.log(' ✓ ' + name); } + catch (e) { failed++; console.error(' ✗ ' + name + ': ' + e.message); } +} +function assert(c, m) { if (!c) throw new Error(m || 'assertion failed'); } + +async function shot(page, name) { + if (!SHOTS) return; + await page.waitForTimeout(400); + await page.screenshot({ path: path.join(SHOTS, '259-' + name + '.png'), fullPage: false }); +} + +// The rendered state of the seeded group row, plus whether its children are +// actually on screen (getClientRects, not just attached) and whether anything +// is left to collapse the group with. +async function rowState(page) { + return page.evaluate(([sel, hash]) => { + const tr = document.querySelector(sel); + if (!tr) return null; + const kids = [...document.querySelectorAll('#pktBody tr.group-child[data-parent-hash="' + hash + '"]')]; + const expandCell = tr.querySelector('td.col-expand'); + return { + action: tr.getAttribute('data-action'), + aria: tr.getAttribute('aria-expanded'), + expandedClass: tr.classList.contains('expanded'), + children: kids.length, + visibleChildren: kids.filter((k) => k.getClientRects().length > 0).length, + expandCellVisible: !!expandCell && expandCell.getClientRects().length > 0, + carets: expandCell ? [...expandCell.querySelectorAll('use')].map((u) => (u.getAttribute('href') || '').split('#')[1]) : [], + // At <= 600 px a hash in expandedHashes renders exactly like a collapsed + // row, so "did not expand" has to be read off the set, not the DOM. + // 'NO-HOOK' if packets.js's test hook is gone, so its absence fails. + inExpandedHashes: window._packetsTestAPI && typeof window._packetsTestAPI._isExpanded === 'function' + ? window._packetsTestAPI._isExpanded(hash) : 'NO-HOOK', + }; + }, [ROW, SEED_HASH]); +} + +async function waitRow(page, want) { + try { + await page.waitForFunction(([sel, w]) => { + const tr = document.querySelector(sel); + if (!tr) return false; + const action = tr.getAttribute('data-action'); + return w === 'desktop' ? action === 'toggle-select' : action === 'select-hash'; + }, [ROW, want], { timeout: 5000 }); + } catch (_) { /* reported below with the actual state */ } + const s = await rowState(page); + const action = want === 'desktop' ? 'toggle-select' : 'select-hash'; + assert(s && s.action === action, 'expected the ' + want + ' row, got ' + JSON.stringify(s)); + return s; +} + +async function openPackets(page) { + await page.goto(BASE + '/#/packets?hash=' + SEED_HASH + '&timeWindow=0', { waitUntil: 'domcontentloaded' }); + await page.waitForSelector(ROW, { timeout: 12000 }); + await page.waitForTimeout(300); +} + +async function expandAtDesktop(page) { + await waitRow(page, 'desktop'); + await page.click(ROW + ' td.col-expand'); + await page.waitForFunction((sel) => document.querySelector(sel).classList.contains('expanded'), ROW, { timeout: 8000 }); +} + +async function run(browser, theme) { + const ctx = await browser.newContext({ viewport: { width: 1400, height: 900 } }); + await ctx.addInitScript((t) => { localStorage.setItem('meshcore-theme', t); }, theme); + const page = await ctx.newPage(); + page.setDefaultTimeout(10000); + page.on('pageerror', (e) => console.error('[pageerror]', e.message)); + + await step(`[${theme}] 1400 px: expanding the group shows its children`, async () => { + await openPackets(page); + await expandAtDesktop(page); + const s = await rowState(page); + assert(s.aria === 'true', 'aria-expanded="true" on desktop: ' + JSON.stringify(s)); + assert(s.expandedClass, 'the row is marked expanded'); + assert(s.visibleChildren > 0, 'the children are on screen: ' + JSON.stringify(s)); + assert(s.carets.length === 1 && s.carets[0] === 'ph-caret-down', 'down caret while expanded: ' + JSON.stringify(s.carets)); + await shot(page, '1400-expanded-' + theme); + }); + + await step(`[${theme}] resize 1400 -> 390: no child row is left visible`, async () => { + await page.setViewportSize({ width: 390, height: 844 }); + await waitRow(page, 'mobile'); + const s = await rowState(page); + assert(s.visibleChildren === 0, 'no visible child rows on mobile: ' + JSON.stringify(s)); + assert(s.children === 0, 'the child rows are not rendered at all: ' + JSON.stringify(s)); + assert(!s.expandedClass, 'the row does not claim to be expanded: ' + JSON.stringify(s)); + assert(s.aria === null, 'and it announces no expanded state (#255)'); + assert(s.inExpandedHashes === true, 'the expansion is kept in state, not cleared: ' + JSON.stringify(s)); + assert(!s.expandCellVisible, 'the expand column is hidden at 390 px, as before'); + await shot(page, '390-after-resize-' + theme); + }); + + await step(`[${theme}] resize 390 -> 1400: the children and the state come back`, async () => { + await page.setViewportSize({ width: 1400, height: 900 }); + await waitRow(page, 'desktop'); + const s = await rowState(page); + assert(s.aria === 'true', 'the expansion was kept, not cleared: ' + JSON.stringify(s)); + assert(s.expandedClass, 'the row is marked expanded again'); + assert(s.visibleChildren > 0, 'the children are back: ' + JSON.stringify(s)); + assert(s.carets.length === 1 && s.carets[0] === 'ph-caret-down', 'down caret again: ' + JSON.stringify(s.carets)); + }); + + await step(`[${theme}] 1400 px: the group still collapses from the caret`, async () => { + await page.click(ROW + ' td.col-expand'); + await page.waitForFunction((sel) => !document.querySelector(sel).classList.contains('expanded'), ROW, { timeout: 8000 }); + const s = await rowState(page); + assert(s.aria === 'false' && s.children === 0, 'collapsed again: ' + JSON.stringify(s)); + assert(s.carets.length === 1 && s.carets[0] === 'ph-caret-right', 'right caret while collapsed: ' + JSON.stringify(s.carets)); + }); + + await ctx.close(); +} + +(async () => { + const browser = await chromium.launch({ + headless: true, + executablePath: process.env.CHROMIUM_PATH || undefined, + args: ['--no-sandbox', '--disable-gpu', '--disable-dev-shm-usage'], + }); + + console.log(`\n=== #259 expanded group across the mobile breakpoint, against ${BASE} ===`); + + await run(browser, 'light'); + await run(browser, 'dark'); + + // A touch context that starts at 390 px, reached by ?hash= so the group is + // never rendered at desktop width first: activating the row must select it, + // and must neither render children nor put the hash into expandedHashes. + // + // ?hash= only filters -- it does not expand anything. The #866 deep link + // that *does* expand before the first render is #/packets/?obs=; + // the first-render-at-narrow-width case it produces is covered by the + // test-packets.js unit cases, which seed expandedHashes and render at 390 px. + { + const ctx = await browser.newContext({ viewport: { width: 390, height: 844 }, isMobile: true, hasTouch: true }); + const page = await ctx.newPage(); + page.setDefaultTimeout(10000); + page.on('pageerror', (e) => console.error('[pageerror]', e.message)); + + await step('390 px touch: a tap selects and leaves no child rows behind', async () => { + await openPackets(page); + await waitRow(page, 'mobile'); + await page.tap(ROW + ' td.col-time'); + await page.waitForFunction(() => { + const s = document.getElementById('mobileDetailSheet'); + return !!s && s.classList.contains('open'); + }, null, { timeout: 8000 }); + const s = await rowState(page); + assert(s.visibleChildren === 0 && !s.expandedClass, 'no children after a tap: ' + JSON.stringify(s)); + assert(s.inExpandedHashes === false, 'the tap did not expand the group: ' + JSON.stringify(s)); + await shot(page, '390-touch-sheet'); + }); + + // The same conclusion at the render level, without the hook: an expansion + // the tap made invisibly would be on screen as soon as the layout crosses + // back over the breakpoint. + await step('390 px touch -> 1400 px: the group is still collapsed', async () => { + await page.evaluate(() => { const s = document.getElementById('mobileDetailSheet'); if (s) s.classList.remove('open'); }); + await page.setViewportSize({ width: 1400, height: 900 }); + const s = await waitRow(page, 'desktop'); + assert(s.aria === 'false', 'aria-expanded="false" over the breakpoint: ' + JSON.stringify(s)); + assert(!s.expandedClass && s.children === 0, 'no children appeared: ' + JSON.stringify(s)); + assert(s.inExpandedHashes === false, 'and nothing entered expandedHashes: ' + JSON.stringify(s)); + }); + await ctx.close(); + } + + await browser.close(); + console.log(`\n${passed} passed, ${failed} failed`); + process.exit(failed === 0 ? 0 : 1); +})().catch((e) => { console.error(e); process.exit(1); }); diff --git a/test-issue-259-nodes-esc-listener.js b/test-issue-259-nodes-esc-listener.js new file mode 100644 index 000000000..f4be23692 --- /dev/null +++ b/test-issue-259-nodes-esc-listener.js @@ -0,0 +1,310 @@ +/** + * #259 (2): the node page's Escape-to-go-back listener must not leak. + * + * public/nodes.js init() wires a document-level `keydown` handler (`nodesEsc`) + * for the full-screen node view. It used to be a fresh closure per init() and + * was only ever removed when Escape actually fired, so navigating node A -> B + * -> C left three live listeners on `document` and one Escape then wrote the + * same hash three times. + * + * The nodes *list* view has a sibling of the same class: `nodesPanelEsc` in + * renderLeft() (Escape closes the detail panel). renderLeft() runs on every + * load of the list -- a visit, a region change, a filter change -- so that one + * leaked one listener per render too. Both are covered here. + * + * This test drives the real nodes.js in a vm sandbox whose + * document.addEventListener / removeEventListener record the live listeners and + * deduplicate on (type, handler) exactly as the DOM does. It runs the router's + * destroy/init cycle for three node pages and asserts: + * - at most one document `keydown` listener survives; + * - Escape writes location.hash exactly once; + * - destroy() leaves no document `keydown` listener behind; + * - the listener still works on a page opened after a destroy; + * - the list view's own Escape listener (`nodesPanelEsc`) does not stack + * across renders or visits, and destroy() removes it. + * + * The list-view cases await the asynchronous loadNodes() before counting: the + * listener is only registered once renderLeft() has run, so a synchronous + * assertion would pass no matter what the page does. + * + * Usage: node test-issue-259-nodes-esc-listener.js + */ +'use strict'; +const vm = require('vm'); +const fs = require('fs'); +const assert = require('assert'); + +let passed = 0, failed = 0; +const queue = []; +function test(name, fn) { queue.push([name, fn]); } +async function runQueue() { + for (const [name, fn] of queue) { + try { await fn(); passed++; console.log(' ✅ ' + name); } + catch (e) { failed++; console.log(' ❌ ' + name + ': ' + e.message); } + } +} + +// nodes.js's list view renders from the async loadNodes(); the sandbox's api() +// resolves immediately, so draining the microtask queue is enough to get the +// real renderLeft() to run. Nothing here depends on timers (the sandbox's are +// no-ops on purpose, so nothing fires behind the assertions). +async function settle() { + for (let i = 0; i < 50; i++) await Promise.resolve(); +} + +// A sandbox that records every document-level listener and the hash writes. +function loadNodes() { + const noop = () => {}; + const docListeners = []; // { type, fn } + const hashWrites = []; + const replaceStateWrites = []; + const errors = []; // anything nodes.js's catch blocks log + let regionChange = null; // the list view's RegionFilter.onChange handler + const pages = {}; + const els = new Map(); + function el(id) { + if (!els.has(id)) { + // A real Set-backed classList, so nodesPanelEsc's "only act while a + // detail panel is open" guard behaves as it does in the browser. + const classes = new Set(); + els.set(id, { + id, innerHTML: '', textContent: '', style: {}, isConnected: true, + dataset: {}, value: '', + classList: { + add: (c) => classes.add(c), remove: (c) => classes.delete(c), + toggle: (c, on) => (on ? classes.add(c) : classes.delete(c)), + contains: (c) => classes.has(c), + }, + addEventListener: noop, removeEventListener: noop, + querySelector: () => null, querySelectorAll: () => [], + appendChild: noop, setAttribute: noop, getAttribute: () => null, + closest: () => null, focus: noop, scrollIntoView: noop, + }); + } + return els.get(id); + } + const location = { + _hash: '#/nodes/aa', + get hash() { return this._hash; }, + set hash(v) { hashWrites.push(v); this._hash = v; }, + search: '', pathname: '/', href: 'http://localhost/', + }; + const ctx = { + window: { + addEventListener: noop, removeEventListener: noop, dispatchEvent: noop, + innerWidth: 1400, innerHeight: 900, location, + }, + document: { + readyState: 'complete', + body: { classList: { add: noop, remove: noop, toggle: noop, contains: () => false } }, + createElement: () => el('created'), + head: { appendChild: noop }, + getElementById: (id) => el(id), + // Deduplicated on (type, fn) exactly as the DOM does, so a stable + // handler reference registered twice counts once — the test measures + // real listener count, not add() calls. + addEventListener: (type, fn) => { + if (docListeners.some((l) => l.type === type && l.fn === fn)) return; + docListeners.push({ type, fn }); + }, + removeEventListener: (type, fn) => { + const i = docListeners.findIndex((l) => l.type === type && l.fn === fn); + if (i >= 0) docListeners.splice(i, 1); + }, + querySelectorAll: () => [], + querySelector: () => null, + }, + console: { log: noop, warn: noop, debug: noop, + // Surfaced by the list-view cases: loadNodes() swallows its own + // exceptions, so a missing stub would otherwise read as "no listener". + error: (...a) => { errors.push(a.map(String).join(' ')); } }, + Date, Math, Array, Object, String, Number, JSON, RegExp, Error, TypeError, + Map, Set, Promise, URLSearchParams, parseInt, parseFloat, isNaN, isFinite, + encodeURIComponent, decodeURIComponent, + setTimeout: noop, clearTimeout: noop, setInterval: noop, clearInterval: noop, + fetch: () => Promise.resolve({ ok: true, json: () => Promise.resolve({}) }), + performance: { now: () => Date.now() }, + localStorage: { getItem: () => null, setItem: noop, removeItem: noop }, + location, + history: { + replaceState: (_s, _t, url) => { replaceStateWrites.push(url); }, + pushState: noop, + }, + CustomEvent: class CustomEvent {}, + requestAnimationFrame: noop, + ROLE_COLORS: {}, ROLE_STYLE: {}, TYPE_COLORS: {}, + getNodeStatus: () => 'active', + getHealthThresholds: () => ({ staleMs: 1, degradedMs: 2, silentMs: 3 }), + timeAgo: () => '', truncate: (s) => s, escapeHtml: (s) => String(s == null ? '' : s), + payloadTypeName: () => '', payloadTypeColor: () => '', + registerPage: (name, page) => { pages[name] = page; }, + RegionFilter: { + init: noop, offChange: noop, getRegionParam: () => '', + onChange: (fn) => { regionChange = fn; return fn; }, + }, + debouncedOnWS: () => null, onWS: noop, offWS: noop, debounce: (fn) => fn, + api: () => Promise.resolve({}), invalidateApiCache: noop, + CLIENT_TTL: { nodeList: 1, nodeDetail: 1, nodeHealth: 1 }, + initTabBar: noop, getFavorites: () => [], favStar: () => '', bindFavStars: noop, + makeColumnsResizable: noop, + getHashParams: () => new URLSearchParams(''), + AreaFilter: { + init: noop, onChange: noop, offChange: noop, getParam: () => '', + getAreaParam: () => '', getSelected: () => null, get: () => null, + }, + nodePassesGeoFilter: () => true, + }; + ctx.window.document = ctx.document; + // The real markup renders the right panel as class="panel-right empty", and + // nodesPanelEsc only acts while it is not .empty. Seed that, so the guard is + // the browser's guard and the cases have to open the panel to exercise it. + el('nodesRight').classList.add('empty'); + vm.createContext(ctx); + vm.runInContext(fs.readFileSync('public/nodes.js', 'utf8'), ctx, { filename: 'public/nodes.js' }); + return { + page: pages.nodes, + app: el('app'), + panel: el('nodesRight'), + keydownListeners: () => docListeners.filter((l) => l.type === 'keydown'), + hashWrites, + replaceStateWrites, + errors, + // Re-runs the list view's load the way a region change does: one more + // renderLeft() inside the same visit. + regionChange: () => regionChange && regionChange(), + pressEscape: () => { + for (const l of docListeners.filter((x) => x.type === 'keydown').slice()) { + l.fn({ key: 'Escape', preventDefault: () => {}, stopPropagation: () => {} }); + } + }, + }; +} + +console.log('\n=== #259 node page: one Escape listener, not one per visit ==='); + +test('nodes.js registers a page with init and destroy', () => { + const s = loadNodes(); + assert(s.page, 'registerPage("nodes", …) ran'); + assert.strictEqual(typeof s.page.init, 'function', 'init'); + assert.strictEqual(typeof s.page.destroy, 'function', 'destroy'); +}); + +test('a single node page leaves exactly one document keydown listener', () => { + const s = loadNodes(); + s.page.init(s.app, 'aa'); + assert.strictEqual(s.keydownListeners().length, 1, 'got ' + s.keydownListeners().length); +}); + +test('navigating A -> B -> C does not stack listeners', () => { + const s = loadNodes(); + for (const pk of ['aa', 'bb', 'cc']) { + s.page.destroy(); // the router destroys the current page first + s.page.init(s.app, pk); + } + const n = s.keydownListeners().length; + assert(n <= 1, 'expected at most one keydown listener after three visits, got ' + n); +}); + +test('after A -> B -> C one Escape writes the hash exactly once', () => { + const s = loadNodes(); + for (const pk of ['aa', 'bb', 'cc']) { + s.page.destroy(); + s.page.init(s.app, pk); + } + s.hashWrites.length = 0; + s.pressEscape(); + assert.deepStrictEqual(s.hashWrites, ['#/nodes'], 'hash writes: ' + JSON.stringify(s.hashWrites)); +}); + +test('destroy() removes the Escape listener', () => { + const s = loadNodes(); + s.page.init(s.app, 'aa'); + s.page.destroy(); + assert.strictEqual(s.keydownListeners().length, 0, 'got ' + s.keydownListeners().length); + s.hashWrites.length = 0; + s.pressEscape(); + assert.deepStrictEqual(s.hashWrites, [], 'a destroyed page does not navigate: ' + JSON.stringify(s.hashWrites)); +}); + +test('Escape still goes back on a page opened after a destroy', () => { + const s = loadNodes(); + s.page.init(s.app, 'aa'); + s.page.destroy(); + s.page.init(s.app, 'bb'); + s.hashWrites.length = 0; + s.pressEscape(); + assert.deepStrictEqual(s.hashWrites, ['#/nodes'], 'hash writes: ' + JSON.stringify(s.hashWrites)); +}); + +// --- the list view's own Escape listener (nodesPanelEsc) -------------------- +// +// The previous version of this block asserted that the list view adds no +// Escape listener at all. That passed for the wrong reason: it counted before +// the asynchronous loadNodes() -> renderLeft() had run. The list view does add +// one, and these cases await the render and hold it to one. + +test('the list view registers exactly one Escape listener once its load resolves', async () => { + const s = loadNodes(); + s.page.init(s.app, null); + await settle(); + assert.deepStrictEqual(s.errors, [], 'the list view rendered without errors'); + assert.strictEqual(s.keydownListeners().length, 1, + 'one list-view Escape listener, got ' + s.keydownListeners().length); +}); + +test('a second render inside the same visit does not add a second listener', async () => { + const s = loadNodes(); + s.page.init(s.app, null); + await settle(); + s.regionChange(); // a region change re-runs loadNodes() -> renderLeft() + await settle(); + assert.deepStrictEqual(s.errors, [], 'both renders ran without errors'); + const n = s.keydownListeners().length; + assert.strictEqual(n, 1, 'still one listener after two renders, got ' + n); +}); + +test('list -> node -> list -> node -> list does not stack listeners', async () => { + const s = loadNodes(); + for (const pk of [null, 'aa', null, 'bb', null]) { + s.page.destroy(); // the router destroys the current page first + s.page.init(s.app, pk); + await settle(); + } + assert.deepStrictEqual(s.errors, [], 'every render ran without errors'); + const n = s.keydownListeners().length; + assert.strictEqual(n, 1, 'the list view ends with one listener, got ' + n); +}); + +test('one Escape closes the list view\'s detail panel exactly once', async () => { + const s = loadNodes(); + for (const pk of [null, 'aa', null]) { + s.page.destroy(); + s.page.init(s.app, pk); + await settle(); + } + // nodesPanelEsc only acts while the right panel is not .empty, so open it. + s.panel.classList.remove('empty'); + s.replaceStateWrites.length = 0; + s.pressEscape(); + assert.deepStrictEqual(s.replaceStateWrites, ['#/nodes'], + 'one panel close, got ' + JSON.stringify(s.replaceStateWrites)); +}); + +test('destroy() removes the list view\'s Escape listener', async () => { + const s = loadNodes(); + s.page.init(s.app, null); + await settle(); + s.page.destroy(); + assert.strictEqual(s.keydownListeners().length, 0, + 'got ' + s.keydownListeners().length); + s.panel.classList.remove('empty'); + s.replaceStateWrites.length = 0; + s.pressEscape(); + assert.deepStrictEqual(s.replaceStateWrites, [], + 'a destroyed list view does not close anything: ' + JSON.stringify(s.replaceStateWrites)); +}); + +runQueue().then(() => { + console.log(`\n${passed} passed, ${failed} failed`); + process.exit(failed === 0 ? 0 : 1); +}); diff --git a/test-packets.js b/test-packets.js index d49a4e125..46aa1a9a8 100644 --- a/test-packets.js +++ b/test-packets.js @@ -1306,6 +1306,101 @@ console.log('\n=== packets.js: group row action and aria-expanded by viewport (# }); } +// #259 (1): a group expanded on desktop must not leave visible child rows behind +// when the layout flips to the mobile mode, where the expand column is hidden +// and the row only selects — there would be no way to collapse it again. The +// hash stays in expandedHashes, so the children come back on desktop; the +// rendered row is the collapsed one while the mobile mode is active. +console.log('\n=== packets.js: an expanded group across the mobile breakpoint (#259) ==='); +{ + const ctx = loadPacketsSandbox(); + loadInCtx(ctx, 'public/mobile-page-actions.js'); + const api = ctx._packetsTestAPI; + const mkChild = (id, obs) => ({ + id, observer_id: obs, hash: 'grp259', raw_hex: 'aabbcc', payload_type: 0, + route_type: 0, decoded_json: '{}', path_json: '[]', timestamp: '2024-01-01T00:00:00Z' + }); + const group = { + hash: 'grp259', count: 3, latest: '2024-01-01T00:00:00Z', + observer_id: null, raw_hex: 'aabbcc', payload_type: 0, + route_type: 0, decoded_json: '{}', path_json: '[]', + observation_count: 3, observer_count: 3, + _children: [mkChild(1, '1'), mkChild(2, '2'), mkChild(3, '3')] + }; + const header = (html) => /]*>/.exec(html)[0]; + const rowClass = (html) => (/ (html.match(/ { + const prev = ctx.window.innerWidth; + ctx.window.innerWidth = w; + try { return fn(); } finally { ctx.window.innerWidth = prev; } + }; + + // _getRowCount only counts children in grouped mode; the hook lets the + // sandbox say so. Guarded so this file still runs against a tree without it. + if (typeof api._setDisplayGrouped === 'function') api._setDisplayGrouped(true); + api._setExpanded(group.hash, true); + + test('#259: at 1400 px the expanded group renders its children (#248 unchanged)', () => atWidth(1400, () => { + const html = api.buildGroupRowHtml(group); + assert.strictEqual(childRows(html), 3, 'three child rows: ' + childRows(html)); + assert(/\bexpanded\b/.test(rowClass(html)), 'the row is marked expanded: ' + rowClass(html)); + assert(header(html).includes('aria-expanded="true"'), header(html)); + assert(expandCell(html).includes(phIcon('caret-down')), 'down caret while expanded'); + })); + + test('#259: at 390 px the same expanded group renders no child rows', () => atWidth(390, () => { + const html = api.buildGroupRowHtml(group); + assert.strictEqual(childRows(html), 0, 'no visible children on mobile, got ' + childRows(html)); + })); + + test('#259: at 390 px the row does not claim the expanded class or caret', () => atWidth(390, () => { + const html = api.buildGroupRowHtml(group); + assert(!/\bexpanded\b/.test(rowClass(html)), 'no expanded class on mobile: ' + rowClass(html)); + assert(header(html).includes('data-action="select-hash"'), header(html)); + assert(!header(html).includes('aria-expanded'), 'still no aria-expanded on mobile'); + assert(!expandCell(html).includes(phIcon('caret-down')), 'no down caret on mobile'); + })); + + test('#259: _getRowCount matches the rendered rows on both sides of the breakpoint', () => { + atWidth(390, () => { + assert.strictEqual(api._getRowCount(group), 1, 'mobile: the group is one row'); + }); + atWidth(1400, () => { + assert.strictEqual(api._getRowCount(group), 4, 'desktop: the group plus three children'); + }); + }); + + test('#259: 600 px hides the children and 601 px shows them again', () => { + atWidth(600, () => { + assert.strictEqual(childRows(api.buildGroupRowHtml(group)), 0, 'at the breakpoint the children are hidden'); + }); + atWidth(601, () => { + assert.strictEqual(childRows(api.buildGroupRowHtml(group)), 3, 'just above it they are back'); + }); + }); + + test('#259: the expansion survives 1400 -> 390 -> 1400, it is not cleared', () => { + atWidth(390, () => { api.buildGroupRowHtml(group); }); + atWidth(1400, () => { + const html = api.buildGroupRowHtml(group); + assert.strictEqual(childRows(html), 3, 'the children are back on desktop: ' + childRows(html)); + assert(header(html).includes('aria-expanded="true"'), 'and the state is still expanded'); + }); + }); + + test('#259: a collapsed group is unaffected at either width', () => { + api._setExpanded(group.hash, false); + try { + for (const w of [390, 1400]) atWidth(w, () => { + const html = api.buildGroupRowHtml(group); + assert.strictEqual(childRows(html), 0, w + ' px: a collapsed group has no children'); + assert(!/\bexpanded\b/.test(rowClass(html)), w + ' px: ' + rowClass(html)); + }); + } finally { api._setExpanded(group.hash, true); } + }); +} + // Without mobile-page-actions.js there is no #1461 #7 redirect, so a group row // toggles at any width. console.log('\n=== packets.js: group row without mobile-page-actions.js (#254) ===');