From 50045bbd3ffcf0fc87210c64bdee2bfdea59b28c Mon Sep 17 00:00:00 2001 From: n30nex Date: Sun, 6 Sep 2026 15:04:03 -0400 Subject: [PATCH 1/2] fix(nodes): clearly mark the selected node in path chains (#1968) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Red commit: `0988bc0` (local browser: 3 passed, 8 behavior assertion failures before the fix). The selected node now has a compact outline in long “Paths Through This Node” chains, in the side panel and full detail page. Matching uses complete public keys without case sensitivity; same-prefix siblings and unresolved hops stay unmarked. Existing links, escaped names, warnings and ambiguity underlines are preserved. Fixes #1153. Its prerequisite #1144 is already merged. - E2E assertion added: `test-issue-1146-path-link-contrast-e2e.js:220`. The existing CI-selected harness passes 11 checks across 18-hop paths, both themes, desktop/mobile, and the renderer fallback. Review follow-up `bd8118f` verifies the marked ambiguous hop's dashed underline. - Browser verified: `http://127.0.0.1:55635`; desktop/mobile path screenshots were inspected. The broader smoke runner exited successfully with fixture-dependent skips. - Required frontend checks pass: 99 filter, 18 aging, 666 helpers. CSS variables, seven CSS self-tests, 31 XSS sink checks, 17 XSS gate self-tests and XSS diff preflight pass. - Three independent reviews found no blocking issues. Traversal remains linear with no new requests, settings, dependencies or cache invalidation; styling uses the existing customizer token. ## Preflight overrides - The external preflight runner is absent; corresponding scoped gates passed. Red browser evidence is local, with upstream CI approval tracked separately under the process in #1922. - Existing rapid-navigation map resize timer errors remain visible in browser logs and are outside this change. (cherry picked from commit a938176f831f979ef0442594ab33286e2585724f) Co-Authored-By: Claude Opus 5 --- public/nodes.js | 10 +- public/style.css | 10 +- test-issue-1146-path-link-contrast-e2e.js | 145 ++++++++++++++++++++-- 3 files changed, 149 insertions(+), 16 deletions(-) diff --git a/public/nodes.js b/public/nodes.js index db73c94b4..b1314b807 100644 --- a/public/nodes.js +++ b/public/nodes.js @@ -1070,17 +1070,18 @@ } document.querySelector('#fullPathsSection h4').textContent = `Paths Through This Node (${pathData.totalPaths} unique, ${pathData.totalTransmissions} transmissions)`; const COLLAPSE_LIMIT = 10; + const currentPubkey = n.public_key.toLowerCase(); function renderPaths(paths) { return paths.map(p => { const chain = p.hops.map(h => { - const isThis = h.pubkey === n.public_key; + const isThis = h.pubkey && h.pubkey.toLowerCase() === currentPubkey; if (window.HopDisplay) { const entry = { name: h.name, pubkey: h.pubkey, ambiguous: h.ambiguous, conflicts: h.conflicts, totalGlobal: h.totalGlobal, totalRegional: h.totalRegional, globalFallback: h.globalFallback, unreliable: h.unreliable }; const html = HopDisplay.renderHop(h.prefix, entry); return isThis ? html.replace('class="', 'class="hop-current ') : html; } const name = escapeHtml(h.name || h.prefix); - const link = h.pubkey ? `${name}` : `${name}`; + const link = h.pubkey ? `${name}` : `${name}`; return link; }).join(' → '); return `
@@ -1990,12 +1991,13 @@ document.querySelector('#pathsSection h4').textContent = `Paths Through This Node (${pathData.totalPaths} unique path${pathData.totalPaths !== 1 ? 's' : ''}, ${pathData.totalTransmissions} transmissions)`; const COLLAPSE_LIMIT = 10; const showAll = pathData.paths.length <= COLLAPSE_LIMIT; + const currentPubkey = n.public_key.toLowerCase(); function renderPaths(paths) { return paths.map(p => { const chain = p.hops.map(h => { - const isThis = h.pubkey === n.public_key; + const isThis = h.pubkey && h.pubkey.toLowerCase() === currentPubkey; const name = escapeHtml(h.name || h.prefix); - const link = h.pubkey ? `${name}` : `${name}`; + const link = h.pubkey ? `${name}` : `${name}`; return link; }).join(' → '); return `
diff --git a/public/style.css b/public/style.css index 6642eb74d..4205b5331 100644 --- a/public/style.css +++ b/public/style.css @@ -3119,7 +3119,15 @@ button.ch-item.ch-item-encrypted .ch-badge { filter: grayscale(0.6); } .hop-unreliable-btn { background: none; border: none; color: var(--status-yellow, #f59e0b); font-size: 13px; cursor: help; vertical-align: middle; margin-left: 2px; padding: 0 2px; line-height: 1; } .hop-global-fallback { border-bottom: 1px dashed var(--status-red); } -.hop-current { font-weight: 700 !important; color: var(--link-color) !important; } +.hop-current { + display: inline-block; + padding: 0 4px; + /* Outline preserves the shared renderer's dashed ambiguity border. */ + outline: 1px solid var(--link-color); + border-radius: 4px; + font-weight: 700 !important; + color: var(--link-color) !important; +} /* Self-loop subpath rows */ .subpath-selfloop { opacity: 0.6; } diff --git a/test-issue-1146-path-link-contrast-e2e.js b/test-issue-1146-path-link-contrast-e2e.js index 77a465fa6..7c496611b 100644 --- a/test-issue-1146-path-link-contrast-e2e.js +++ b/test-issue-1146-path-link-contrast-e2e.js @@ -14,15 +14,21 @@ * theme, then asserts the computed link colour vs. its background * yields a contrast ratio ≥ 4.5:1. * - * Currently FAILS (link color resolves to rgb(0,0,238)). - * After the style.css fix it PASSES. + * #1153 also checks that the selected node has a visible enclosing marker + * in an 18-hop chain, without marking a different node with the same prefix. + * Covers the desktop side/full views and mobile full view in both themes. * * Usage: BASE_URL=http://localhost:13581 node test-issue-1146-path-link-contrast-e2e.js + * Optional SCREENSHOT_DIR saves PNG evidence; use an ignored output directory. */ 'use strict'; const { chromium } = require('playwright'); const BASE = process.env.BASE_URL || 'http://localhost:13581'; +assert(['localhost', '127.0.0.1', '[::1]'].includes(new URL(BASE).hostname), + 'Browser regression tests must use a local server'); +const SCREENSHOT_DIR = process.env.SCREENSHOT_DIR; +if (SCREENSHOT_DIR) require('fs').mkdirSync(SCREENSHOT_DIR, { recursive: true }); let passed = 0, failed = 0; async function step(name, fn) { @@ -83,20 +89,41 @@ async function effectiveBgFor(page, selector) { console.log(`\n=== #1146 path-link contrast E2E against ${BASE} ===`); - const hopPubkey = 'a1b2c3d4e5f60718293a4b5c6d7e8f9001122334455667788990aabbccddeeff'; + const hopPubkey = 'a1'.repeat(32); + const targetName = 'Selected & relay'; + let selectedPubkey; + let targetHopKey; + let siblingKey; // Mock paths API for ANY node so test is deterministic. await page.route('**/api/nodes/*/paths*', (route) => { - route.fulfill({ + selectedPubkey = decodeURIComponent(new URL(route.request().url()).pathname.split('/')[3]); + // Full public-key identity must survive hex casing; a shared prefix alone + // must never qualify. Neither fixture node requires resolver changes. + targetHopKey = selectedPubkey === selectedPubkey.toUpperCase() + ? selectedPubkey.toLowerCase() : selectedPubkey.toUpperCase(); + siblingKey = targetHopKey.slice(0, -1) + (targetHopKey.endsWith('0') ? '1' : '0'); + const hops = Array.from({ length: 18 }, (_, i) => ({ + pubkey: hopPubkey.slice(0, -2) + i.toString(16).padStart(2, '0'), + prefix: 'a1', name: 'Relay ' + (i + 1), + })); + hops[8] = { pubkey: targetHopKey, prefix: targetHopKey.slice(0, 2), name: targetName, unreliable: true, ambiguous: true }; + hops[9] = { + pubkey: siblingKey, prefix: targetHopKey.slice(0, 2), name: 'Same-prefix sibling', ambiguous: true, + conflicts: [ + { pubkey: targetHopKey, name: targetName, regional: true }, + { pubkey: siblingKey, name: 'Same-prefix sibling', regional: true }, + ], + }; + hops[10] = { prefix: targetHopKey.slice(0, 2), name: 'Unresolved prefix' }; + return route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify({ totalPaths: 1, totalTransmissions: 5, paths: [{ - hops: [ - { pubkey: hopPubkey, prefix: 'a1', name: 'TestHop' }, - ], + hops, count: 5, lastSeen: new Date().toISOString(), sampleHash: 'deadbeef00', @@ -107,10 +134,10 @@ async function effectiveBgFor(page, selector) { await step('Load nodes page and force dark theme', async () => { await page.goto(BASE + '/#/nodes', { waitUntil: 'domcontentloaded' }); - await page.evaluate(() => { - document.documentElement.setAttribute('data-theme', 'dark'); - }); await page.waitForSelector('#nodesBody tr[data-key]', { timeout: 15000 }); + if (await page.locator('html').getAttribute('data-theme') !== 'dark') { + await page.locator('#darkModeToggle').click(); + } }); await step('Open side panel for first node and wait for paths', async () => { @@ -121,7 +148,7 @@ async function effectiveBgFor(page, selector) { const el = document.getElementById('pathsContent'); return el && el.querySelector('a[href^="#/nodes/"]'); }, - { timeout: 15000 } + null, { timeout: 15000 } ); }); @@ -145,6 +172,102 @@ async function effectiveBgFor(page, selector) { `covered by the .data-table td a rule and inherit UA blue.`); }); + async function assertCurrentHop(selector, hasHopDisplay) { + await page.waitForFunction((sel) => document.querySelector(sel)?.textContent.includes('Relay 18'), selector); + const targetSelector = selector + ' a[href="#/nodes/' + targetHopKey + '"]'; + const result = await page.evaluate(({ selector, targetHopKey, siblingKey }) => { + const container = document.querySelector(selector); + const chain = container.firstElementChild.firstElementChild; + const target = chain.querySelector('a[href="#/nodes/' + targetHopKey + '"]'); + const sibling = chain.querySelector('a[href="#/nodes/' + siblingKey + '"]'); + target.scrollIntoView({ block: 'center' }); + const style = getComputedStyle(target); + const rect = target.getBoundingClientRect(); + const chainRect = chain.getBoundingClientRect(); + const outline = parseFloat(style.outlineWidth) >= 1 && style.outlineStyle === 'solid'; + const border = ['Top', 'Right', 'Bottom', 'Left'].every(side => + parseFloat(style['border' + side + 'Width']) >= 1 && style['border' + side + 'Style'] === 'solid'); + return { + marked: Array.from(chain.querySelectorAll('.hop-current')).map(el => el.getAttribute('href')), + ring: outline || border, ringColor: outline ? style.outlineColor : style.borderTopColor, + outline, dashedBottom: style.borderBottomStyle === 'dashed' && parseFloat(style.borderBottomWidth) >= 1, + textColor: style.color, padding: parseFloat(style.paddingLeft), + text: target.textContent, injectedElements: target.childElementCount, + siblingMarked: sibling.classList.contains('hop-current'), + fits: rect.left >= chainRect.left - 1 && rect.right <= chainRect.right + 1, + fragments: target.getClientRects().length, + warning: !!chain.querySelector('.hop-unreliable-btn'), + conflict: !!chain.querySelector('.hop-conflict-btn'), + siblingAmbiguous: sibling.classList.contains('hop-ambiguous'), + hopCount: chain.children.length - chain.querySelectorAll('button').length, + }; + }, { selector, targetHopKey, siblingKey }); + assert(result.hopCount === 18, 'Expected all 18 hops in the rendered chain'); + assert(result.ring, 'Selected hop must have a visible enclosing border or outline, beyond bold/color'); + assert(result.marked.length === 1 && result.marked[0] === '#/nodes/' + targetHopKey, + 'Exactly the selected full public key must be marked, including mixed-case hex'); + assert(!result.siblingMarked, 'Same-prefix sibling must not be marked as this node'); + assert(result.padding > 0 && result.fits && result.fragments === 1, + 'Selected hop marker must enclose the name and fit within the wrapping chain'); + assert(result.text === targetName && result.injectedElements === 0, 'Selected hop name must stay escaped'); + const bg = parseRgb(await effectiveBgFor(page, targetSelector)); + assert(contrast(parseRgb(result.ringColor), bg) >= 3, 'Selected hop ring must contrast with the active theme'); + assert(contrast(parseRgb(result.textColor), bg) >= 4.5, 'Selected hop text must remain readable'); + if (hasHopDisplay) { + assert(result.warning && result.conflict && result.siblingAmbiguous, + 'Shared hop rendering must preserve unreliable and ambiguous indicators'); + assert(result.outline && result.dashedBottom, + 'The selected ambiguous hop must keep its dashed bottom border alongside the enclosing outline'); + } + if (SCREENSHOT_DIR) { + const theme = await page.locator('html').getAttribute('data-theme'); + const filename = `${selector.slice(1)}-${page.viewportSize().width}-${theme}${hasHopDisplay ? '' : '-plain'}.png`; + await page.locator(selector).screenshot({ path: require('path').join(SCREENSHOT_DIR, filename) }); + } + } + + await step('#1153 desktop side panel: selected hop stands out in an 18-hop dark chain', + () => assertCurrentHop('#pathsContent', false)); + + for (const theme of ['light', 'dark']) { + if (await page.locator('html').getAttribute('data-theme') !== theme) { + await page.locator('#darkModeToggle').click(); + } + await step('#1153 desktop side panel: ' + theme + ' theme', async () => { + await page.goto(BASE + '/#/nodes', { waitUntil: 'domcontentloaded' }); + assert(await page.locator('html').getAttribute('data-theme') === theme, 'Expected the selected theme'); + await page.locator('#nodesBody tr[data-key]').first().click(); + await assertCurrentHop('#pathsContent', false); + }); + await step('#1153 desktop full view: ' + theme + ' theme', async () => { + await page.locator('#nodesRight .node-detail-btn').click(); + await assertCurrentHop('#fullPathsContent', true); + }); + } + + for (const theme of ['light', 'dark']) { + // The theme toggle lives in the desktop header; exercise the same theme + // at mobile width without depending on the unrelated navigation drawer. + await page.setViewportSize({ width: 1280, height: 900 }); + if (await page.locator('html').getAttribute('data-theme') !== theme) { + await page.locator('#darkModeToggle').click(); + } + await page.setViewportSize({ width: 390, height: 844 }); + await step('#1153 mobile row click opens a readable full chain: ' + theme + ' theme', async () => { + await page.goto(BASE + '/#/nodes', { waitUntil: 'domcontentloaded' }); + assert(await page.locator('html').getAttribute('data-theme') === theme, 'Expected the selected theme'); + await page.locator('#nodesBody tr[data-key]').first().click(); + await assertCurrentHop('#fullPathsContent', true); + }); + } + + await step('#1153 full-view fallback also marks only the selected node', async () => { + await page.route('**/hop-display.js*', route => route.fulfill({ contentType: 'text/javascript', body: '' })); + await page.reload({ waitUntil: 'domcontentloaded' }); + await assertCurrentHop('#fullPathsContent', false); + assert(await page.evaluate(() => !window.HopDisplay), 'Expected the real full-view fallback'); + }); + await browser.close(); console.log(`\n${passed} passed, ${failed} failed`); From 69610d6a85e4bf5b9e69769dda046a76ae17f303 Mon Sep 17 00:00:00 2001 From: Dennis Jakobsen Date: Sun, 20 Sep 2026 14:34:41 +0200 Subject: [PATCH 2/2] reviewfix(nodes): mark the selected hop with a shadow ring, not an outline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent review found that the new marker uses outline, which is this app's focus affordance (style.css "Focus Indicators": a:focus-visible gets outline: 2px solid var(--accent)). Measured on the rendered chain, an unfocused marked hop drew a 1px blue outline and a focused one 2px in a near-identical blue — so the marker read as focus, and real focus was almost indistinguishable from it while tabbing an 18-hop chain. box-shadow: 0 0 0 1px draws the same ring, keeps the property the original comment was reaching for (it does not disturb the shared renderer's dashed ambiguity border, verified by the existing dashed-bottom assertion), and leaves outline exclusively to :focus-visible. The test's ring detection only recognised outline/border, so it now also accepts a shadow ring, and asserts the marker does NOT use outline. Both mutations fail: reverting to outline → 8 failures, removing the ring → 8. Not changed, and reported instead: the review traced that the case-sensitive pubkey compare this PR also fixes was never actually reachable — both sides come from the same lowercase nodes.public_key column (the uppercase domain is the observers table, which never feeds path hops). The lowercasing is kept as harmless normalisation, but it is hardening, not a fix for an observed bug. Co-Authored-By: Claude Opus 5 --- public/style.css | 9 +++++++-- test-issue-1146-path-link-contrast-e2e.js | 21 ++++++++++++++++++--- 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/public/style.css b/public/style.css index e1216bc8d..27fa83602 100644 --- a/public/style.css +++ b/public/style.css @@ -3148,8 +3148,13 @@ button.ch-item.ch-item-encrypted .ch-badge { filter: grayscale(0.6); } .hop-current { display: inline-block; padding: 0 4px; - /* Outline preserves the shared renderer's dashed ambiguity border. */ - outline: 1px solid var(--link-color); + /* box-shadow, not outline: outline is this app's focus affordance (see + "Focus Indicators" above), so an outlined-but-unfocused hop reads as + focused, and a genuinely focused one would differ only in ring width and + a near-identical blue. A shadow ring draws the same box while leaving + :focus-visible unambiguous, and — like outline — it does not disturb the + shared renderer's dashed ambiguity border. */ + box-shadow: 0 0 0 1px var(--link-color); border-radius: 4px; font-weight: 700 !important; color: var(--link-color) !important; diff --git a/test-issue-1146-path-link-contrast-e2e.js b/test-issue-1146-path-link-contrast-e2e.js index 7c496611b..43f24cd8c 100644 --- a/test-issue-1146-path-link-contrast-e2e.js +++ b/test-issue-1146-path-link-contrast-e2e.js @@ -187,9 +187,14 @@ async function effectiveBgFor(page, selector) { const outline = parseFloat(style.outlineWidth) >= 1 && style.outlineStyle === 'solid'; const border = ['Top', 'Right', 'Bottom', 'Left'].every(side => parseFloat(style['border' + side + 'Width']) >= 1 && style['border' + side + 'Style'] === 'solid'); + // A shadow ring counts too: outline is reserved for :focus-visible, so + // the marker deliberately uses box-shadow instead (see style.css). + const shadowMatch = /rgba?\([^)]*\)/.exec(style.boxShadow || ''); + const shadow = style.boxShadow !== 'none' && !!shadowMatch; return { marked: Array.from(chain.querySelectorAll('.hop-current')).map(el => el.getAttribute('href')), - ring: outline || border, ringColor: outline ? style.outlineColor : style.borderTopColor, + ring: outline || border || shadow, + ringColor: outline ? style.outlineColor : border ? style.borderTopColor : (shadowMatch && shadowMatch[0]), outline, dashedBottom: style.borderBottomStyle === 'dashed' && parseFloat(style.borderBottomWidth) >= 1, textColor: style.color, padding: parseFloat(style.paddingLeft), text: target.textContent, injectedElements: target.childElementCount, @@ -213,11 +218,21 @@ async function effectiveBgFor(page, selector) { const bg = parseRgb(await effectiveBgFor(page, targetSelector)); assert(contrast(parseRgb(result.ringColor), bg) >= 3, 'Selected hop ring must contrast with the active theme'); assert(contrast(parseRgb(result.textColor), bg) >= 4.5, 'Selected hop text must remain readable'); + // The marker must not impersonate the focus ring. `outline` is this app's + // focus affordance (style.css "Focus Indicators"), so an unfocused marked + // hop drawing its own outline would read as focused, and a genuinely + // focused one would differ only in ring width — leaving a keyboard user + // unable to tell which hop of an 18-hop chain actually has focus. + // (:focus-visible itself is not asserted here: it needs real keyboard + // interaction, and the rule that provides it is not part of this change.) + assert(!result.outline, + 'The selected-hop marker must not use outline — that is reserved for :focus-visible'); + if (hasHopDisplay) { assert(result.warning && result.conflict && result.siblingAmbiguous, 'Shared hop rendering must preserve unreliable and ambiguous indicators'); - assert(result.outline && result.dashedBottom, - 'The selected ambiguous hop must keep its dashed bottom border alongside the enclosing outline'); + assert(result.ring && result.dashedBottom, + 'The selected ambiguous hop must keep its dashed bottom border alongside the enclosing marker ring'); } if (SCREENSHOT_DIR) { const theme = await page.locator('html').getAttribute('data-theme');