diff --git a/public/channels.js b/public/channels.js index 8acb72835..ac27ca50a 100644 --- a/public/channels.js +++ b/public/channels.js @@ -263,6 +263,24 @@ } let autoScroll = true; + // #314: the #chMessages scroll handler. A stable module-level reference, + // not a fresh closure per init(): a repeat addEventListener of the same + // function is a DOM no-op, so a visit leaves at most one listener, and + // destroy() can take it off the element it was added to again (the same + // leak class as #259/#282, scoped to the element instead of `document`). + // _chScrollEl is only the bookkeeping for add/remove; the handler reads the + // pane off the event, so it works whether or not that pane is still in the + // document. + let _chScrollEl = null; + function onChMessagesScroll(e) { + const el = e.currentTarget; + const atBottom = el.scrollHeight - el.scrollTop - el.clientHeight < 60; + autoScroll = atBottom; + // Optional-chained like every other #chScrollBtn call site in this file: + // a scroll event can reach a detached #chMessages after the page is gone, + // and the button is no longer in the document by then. + document.getElementById('chScrollBtn')?.classList.toggle('hidden', atBottom); + } let nodeCache = {}; let selectedNode = null; let observerIataById = {}; @@ -1660,12 +1678,12 @@ }); const msgEl = document.getElementById('chMessages'); - msgEl.addEventListener('scroll', () => { - const atBottom = msgEl.scrollHeight - msgEl.scrollTop - msgEl.clientHeight < 60; - autoScroll = atBottom; - document.getElementById('chScrollBtn').classList.toggle('hidden', atBottom); - }); - document.getElementById('chScrollBtn').addEventListener('click', scrollToBottom); + // #314: drop the previous #chMessages before taking the new one, so a + // re-init cannot leave a live handler on a detached element. + if (_chScrollEl && _chScrollEl !== msgEl) _chScrollEl.removeEventListener('scroll', onChMessagesScroll); + _chScrollEl = msgEl; + msgEl.addEventListener('scroll', onChMessagesScroll); + document.getElementById('chScrollBtn')?.addEventListener('click', scrollToBottom); // Event delegation for node clicks and hovers (click + touchend for mobile reliability) function handleNodeTap(e) { @@ -2021,6 +2039,9 @@ function destroy() { if (window.ChannelProposals) window.ChannelProposals.unmount(); + // #314: the message pane's scroll listener goes with the page. + if (_chScrollEl) _chScrollEl.removeEventListener('scroll', onChMessagesScroll); + _chScrollEl = null; if (wsHandler) offWS(wsHandler); wsHandler = null; if (timeAgoTimer) clearInterval(timeAgoTimer); diff --git a/public/style.css b/public/style.css index 27475bbd0..6096cf097 100644 --- a/public/style.css +++ b/public/style.css @@ -4515,6 +4515,20 @@ button.region-pill-active:hover { opacity: 0.85; color: var(--text-on-accent); } .multi-select-trigger { height: 48px; } + /* #314: two controls the #239 sweep did not cover. Measured at 375x812 + with a touch user agent, the swipe tip's "Got it" was 26 px tall and + the packets reveal pill 18 px. Both are compact by design on desktop + (a hint chip and a column-header badge), so the bump is touch-only, + like the rest of this block. Both need the flex centring too: their + labels are smaller than the box they now sit in. */ + .gesture-hint-dismiss, + .col-hidden-pill { + min-height: 48px; + min-width: 48px; + display: inline-flex; + align-items: center; + justify-content: center; + } } /* #630-3: Status text labels — visually hidden text for screen readers */ diff --git a/test-all.sh b/test-all.sh index 1b13852ea..cdb2aab9e 100755 --- a/test-all.sh +++ b/test-all.sh @@ -64,6 +64,7 @@ run test-issue-258-column-widths.js run test-issue-259-nodes-esc-listener.js run test-issue-282-pktesc-listener.js run test-issue-282-comment-guards.js +run test-issue-314-chscroll-listener.js run test-issue-322-comment-guards.js run test-perf-go-runtime.js run test-channel-psk-ux.js diff --git a/test-issue-314-chscroll-listener.js b/test-issue-314-chscroll-listener.js new file mode 100644 index 000000000..ea539f496 --- /dev/null +++ b/test-issue-314-chscroll-listener.js @@ -0,0 +1,352 @@ +/** + * #314 (1): the channel message scroll listener must not throw after the page + * is gone, and must not outlive it. + * + * public/channels.js registered an anonymous `scroll` listener on #chMessages + * inside init() whose body did + * + * document.getElementById('chScrollBtn').classList.toggle('hidden', atBottom) + * + * with no guard. The router (public/app.js) destroys the current page and then + * lets the next page's init() rebuild #app, which detaches #chMessages. A + * `scroll` event can still reach that detached element -- resetting the scroll + * position of a removed overflow box queues one -- and by then #chScrollBtn is + * no longer in the document, so getElementById() returns null and the listener + * threw: + * + * Uncaught TypeError: Cannot read properties of null (reading 'classList') + * at HTMLDivElement. (channels.js:1668) + * + * The fix optional-chains the lookup, the way every other #chScrollBtn call + * site in the file already does, and makes the handler a stable module-level + * function so destroy() can take it off the element it was added to again -- + * the same leak class as #259/#282, scoped to the element instead of + * `document`. + * + * The harness drives the real channels.js in a vm sandbox (modelled on + * test-channels-client-state-152.js) whose elements record listeners and + * deduplicate on (type, handler) exactly as the DOM does, so the counts below + * are real listener counts and not addEventListener call counts. + * + * The harness models element identity the way the browser does: assigning + * #app's innerHTML detaches every element the previous page created, so a + * second init() gets a *new* #chMessages. That is what makes the leak + * measurable here -- one detached #chMessages per visit, each pinned by a + * live handler -- instead of collapsing onto a single reused element. + * + * Usage: node test-issue-314-chscroll-listener.js + */ +'use strict'; + +const vm = require('vm'); +const fs = require('fs'); +const path = require('path'); +const assert = require('assert'); +const { webcrypto } = require('crypto'); + +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); } + } +} + +async function flush(n) { + for (let i = 0; i < (n || 20); i++) await new Promise((r) => setImmediate(r)); +} + +const noop = () => {}; + +function makeHarness() { + const storage = {}; + let elements = {}; + // Every element the harness has ever handed out, so a test can ask what the + // *detached* ones are still holding. + const everyEl = []; + // The harness cannot parse HTML, so getElementById() fabricates whatever id + // the page asks for -- which models "the markup init() just wrote is there". + // Off while no channels markup is in the document (before the first init, + // and after the router has handed #app to another page). + let markupPresent = false; + + function makeFakeEl(id) { + const classes = new Set(); + const listeners = {}; + const el = { + id: id || '', textContent: '', value: '', checked: false, + scrollTop: 0, scrollHeight: 0, clientHeight: 0, + style: {}, dataset: {}, + isConnected: true, + classList: { + add: (c) => { classes.add(c); }, + remove: (c) => { classes.delete(c); }, + toggle: (c, on) => { + if (on === undefined) { if (classes.has(c)) classes.delete(c); else classes.add(c); return; } + if (on) classes.add(c); else classes.delete(c); + }, + contains: (c) => classes.has(c), + }, + // Deduplicated on (type, handler) exactly as the DOM does: a stable + // handler registered twice counts once. + addEventListener(type, fn) { + const l = (listeners[type] = listeners[type] || []); + if (l.indexOf(fn) === -1) l.push(fn); + }, + removeEventListener(type, fn) { + const l = listeners[type]; + if (!l) return; + const i = l.indexOf(fn); + if (i >= 0) l.splice(i, 1); + }, + listenerCount(type) { return (listeners[type] || []).length; }, + dispatch(type, extra) { + const ev = Object.assign({ type, target: el, currentTarget: el, preventDefault: noop, stopPropagation: noop }, extra || {}); + for (const fn of (listeners[type] || []).slice()) fn(ev); + }, + querySelector() { this._q = this._q || {}; return this._q.one || (this._q.one = makeFakeEl()); }, + querySelectorAll() { return []; }, + getAttribute() { return null; }, setAttribute: noop, removeAttribute: noop, + hasAttribute: () => false, closest: () => null, + getBoundingClientRect: () => ({ width: 240, height: 0, top: 0, left: 0, right: 0, bottom: 0 }), + appendChild: noop, removeChild: noop, remove: noop, insertBefore: noop, + focus: noop, blur: noop, scrollIntoView: noop, + }; + // Writing #app's innerHTML is what the router/page does on every visit: + // the previous page's markup leaves the document, and the new markup is + // there to be looked up. Exactly the moment the old #chMessages detaches. + let html = ''; + Object.defineProperty(el, 'innerHTML', { + get: () => html, + set: (v) => { + html = String(v); + if (el.id === 'app') { replaceAppMarkup(); markupPresent = true; } + }, + }); + everyEl.push(el); + return el; + } + // Detach everything the previous page's markup held and forget it, so the + // next getElementById() builds a fresh element. + function replaceAppMarkup() { + for (const id of Object.keys(elements)) { + if (id === 'app') continue; + elements[id].isConnected = false; + delete elements[id]; + } + } + function el(id) { + if (!elements[id]) elements[id] = makeFakeEl(id); + return elements[id]; + } + + const windowListeners = {}; + const errors = []; + const h = { elements }; + + const ctx = { + window: { + addEventListener(type, fn) { (windowListeners[type] = windowListeners[type] || []).push(fn); }, + removeEventListener: noop, + matchMedia: () => ({ matches: false, addEventListener: noop, removeEventListener: noop }), + }, + document: { + readyState: 'complete', + documentElement: { getAttribute: () => null, setAttribute: noop, classList: { add: noop, remove: noop, toggle: noop, contains: () => false } }, + createElement: () => makeFakeEl(), + head: { appendChild: noop }, + body: { appendChild: noop, removeChild: noop, contains: () => false }, + // Once the router has handed #app to another page, the channels markup -- + // #chMessages, #chScrollBtn -- is no longer in the document. + getElementById: (id) => (id === 'app' || markupPresent ? el(id) : null), + addEventListener: noop, removeEventListener: noop, + querySelector: () => null, + querySelectorAll: () => [], + }, + console: Object.assign({}, console, { error: (...a) => { errors.push(a.map(String).join(' ')); } }), + Date, Math, Array, Object, String, Number, JSON, RegExp, Error, TypeError, RangeError, + Infinity, NaN, Set, Map, Promise, parseInt, parseFloat, isNaN, isFinite, + encodeURIComponent, decodeURIComponent, + setTimeout: (fn) => { Promise.resolve().then(fn); return 0; }, + clearTimeout: noop, setInterval: () => 0, clearInterval: noop, + requestAnimationFrame: (cb) => { Promise.resolve().then(cb); return 0; }, + cancelAnimationFrame: noop, + fetch: () => Promise.resolve({ ok: true, json: () => Promise.resolve({}) }), + performance: { now: () => Date.now() }, + localStorage: { + getItem: (k) => Object.prototype.hasOwnProperty.call(storage, k) ? storage[k] : null, + setItem: (k, v) => { storage[k] = String(v); }, + removeItem: (k) => { delete storage[k]; }, + }, + location: { hash: '#/channels', search: '', pathname: '/' }, + history: { replaceState: noop, pushState: noop }, + crypto: webcrypto, TextEncoder, TextDecoder, + Uint8Array, Uint16Array, Uint32Array, Int8Array, Int16Array, Int32Array, ArrayBuffer, + URLSearchParams, + CustomEvent: class CustomEvent {}, + MutationObserver: class MutationObserver { observe() {} disconnect() {} }, + matchMedia: () => ({ matches: false, addEventListener: noop, removeEventListener: noop }), + addEventListener: noop, dispatchEvent: noop, + getHashParams: () => new URLSearchParams(), + btoa: (s) => Buffer.from(String(s), 'binary').toString('base64'), + atob: (s) => Buffer.from(String(s), 'base64').toString('binary'), + CSS: { escape: (s) => String(s) }, + }; + ctx.self = ctx; + ctx.globalThis = ctx; + + ctx.onWS = noop; + ctx.offWS = noop; + ctx.debouncedOnWS = (fn) => fn; + ctx.debounce = (fn) => fn; + ctx.api = () => Promise.resolve({ channels: [], messages: [], packets: [], observers: [] }); + ctx.invalidateApiCache = noop; + ctx.CLIENT_TTL = { channels: 15000, observers: 120000, channelMessages: 10000, nodeDetail: 10000 }; + ctx.escapeHtml = (s) => String(s == null ? '' : s); + ctx.truncate = (s) => String(s || ''); + ctx.formatHashHex = (x) => String(x); + ctx.formatSecondsAgo = () => ''; + ctx.timeAgo = () => ''; + ctx.payloadTypeName = () => 'GRP_TXT'; + ctx.ROLE_EMOJI = {}; + ctx.ROLE_LABELS = {}; + ctx.RegionFilter = { + init: noop, onChange: (fn) => fn, offChange: noop, + getRegionParam: () => '', getSelected: () => null, + }; + ctx.ChannelColors = { get: () => null, remove: noop }; + ctx.ChannelColorPicker = { open: noop, show: noop }; + + let pageMod = null; + ctx.registerPage = (name, mod) => { if (name === 'channels') pageMod = mod; }; + + vm.createContext(ctx); + function load(file) { + vm.runInContext(fs.readFileSync(path.join(__dirname, file), 'utf8'), ctx, { filename: file }); + for (const k of Object.keys(ctx.window)) ctx[k] = ctx.window[k]; + } + load('public/vendor/aes-ecb.js'); + load('public/channel-decrypt.js'); + load('public/channel-proposals.js'); + ctx.window.ChannelProposals.mount = noop; + ctx.window.ChannelProposals.unmount = noop; + load('public/channels.js'); + + h.page = pageMod; + h.errors = errors; + h.msgEl = () => el('chMessages'); + h.scrollBtn = () => el('chScrollBtn'); + // Scroll listeners on the #chMessages that is currently in the document. + h.scrollListeners = () => el('chMessages').listenerCount('scroll'); + // Scroll listeners on every #chMessages ever built, detached ones included. + // This is the leak: a detached element with a live handler stays reachable. + h.allScrollListeners = () => everyEl.reduce((n, e) => n + (e.id === 'chMessages' ? e.listenerCount('scroll') : 0), 0); + h.init = async () => { await pageMod.init(el('app'), null); await flush(); }; + // What the router does on the way out: destroy the page, then hand #app to + // the next page's init(). The old #chMessages is detached and nothing from + // the channels markup is findable any more. + h.navigateAway = () => { replaceAppMarkup(); markupPresent = false; }; + return h; +} + +console.log('\n=== #314 channels: the message scroll listener survives teardown ==='); + +test('channels.js registers a page with init and destroy', () => { + const h = makeHarness(); + assert(h.page, 'registerPage("channels", …) ran'); + assert.strictEqual(typeof h.page.init, 'function', 'init'); + assert.strictEqual(typeof h.page.destroy, 'function', 'destroy'); +}); + +test('a single #/channels visit leaves exactly one scroll listener on #chMessages', async () => { + const h = makeHarness(); + await h.init(); + assert.deepStrictEqual(h.errors, [], 'init rendered without errors'); + assert.strictEqual(h.scrollListeners(), 1, 'got ' + h.scrollListeners()); +}); + +test('the scroll listener toggles #chScrollBtn while the page is live', async () => { + const h = makeHarness(); + await h.init(); + const msgEl = h.msgEl(); + // Scrolled well away from the bottom: the "new messages" button shows. + msgEl.scrollHeight = 2000; msgEl.clientHeight = 400; msgEl.scrollTop = 0; + msgEl.dispatch('scroll'); + assert.strictEqual(h.scrollBtn().classList.contains('hidden'), false, + 'away from the bottom the button must be visible'); + // Back at the bottom: it hides again. + msgEl.scrollTop = 1600; + msgEl.dispatch('scroll'); + assert.strictEqual(h.scrollBtn().classList.contains('hidden'), true, + 'at the bottom the button must be hidden'); +}); + +// The acceptance criterion from the issue: #/channels -> #/live, with a +// `scroll` still in flight on the message pane that just left the document. +test('a scroll event on a detached #chMessages does not throw', async () => { + const h = makeHarness(); + await h.init(); + const msgEl = h.msgEl(); + msgEl.scrollHeight = 2000; msgEl.clientHeight = 400; msgEl.scrollTop = 0; + h.navigateAway(); // the router handed #app to the next page + assert.strictEqual(msgEl.isConnected, false, 'the pane really is detached'); + assert.doesNotThrow(() => msgEl.dispatch('scroll'), + 'the listener must tolerate #chScrollBtn being gone'); +}); + +test('destroy() removes the scroll listener', async () => { + const h = makeHarness(); + await h.init(); + h.page.destroy(); + assert.strictEqual(h.scrollListeners(), 0, 'got ' + h.scrollListeners()); +}); + +test('entering and leaving #/channels several times leaves one live listener', async () => { + const h = makeHarness(); + for (let i = 0; i < 4; i++) { + h.page.destroy(); // the router destroys the current page first + await h.init(); // ... then the next init() rebuilds #app + } + assert.deepStrictEqual(h.errors, [], 'every visit rendered without errors'); + assert.strictEqual(h.scrollListeners(), 1, + 'one listener on the live pane, got ' + h.scrollListeners()); + // The four panes from the earlier visits are detached; none may still hold + // a handler, or each visit pins its old subtree. + assert.strictEqual(h.allScrollListeners(), 1, + 'one scroll listener across every #chMessages ever built, got ' + h.allScrollListeners()); +}); + +// The router always destroys before it re-inits (public/app.js), so this is +// the belt-and-braces half of the guarantee: init() drops the pane it held +// before it takes the new one, so even a re-init without a destroy cannot +// leave a handler on the detached one. +test('a re-init without a destroy leaves no handler on the old pane', async () => { + const h = makeHarness(); + await h.init(); + const first = h.msgEl(); + await h.init(); + assert.notStrictEqual(h.msgEl(), first, 'the second init() built a new pane'); + assert.strictEqual(first.listenerCount('scroll'), 0, + 'the detached pane must be clean, got ' + first.listenerCount('scroll')); + assert.strictEqual(h.allScrollListeners(), 1, 'got ' + h.allScrollListeners()); +}); + +test('the listener still works on a page opened after a destroy', async () => { + const h = makeHarness(); + await h.init(); + h.page.destroy(); + await h.init(); + assert.strictEqual(h.scrollListeners(), 1, 'got ' + h.scrollListeners()); + const msgEl = h.msgEl(); + msgEl.scrollHeight = 2000; msgEl.clientHeight = 400; msgEl.scrollTop = 0; + msgEl.dispatch('scroll'); + assert.strictEqual(h.scrollBtn().classList.contains('hidden'), false, + 'the reopened page still toggles the button'); +}); + +runQueue().then(() => { + console.log(`\n${passed} passed, ${failed} failed`); + process.exit(failed === 0 ? 0 : 1); +}); diff --git a/test-touch-targets.js b/test-touch-targets.js index d33f4320e..2a17d79b3 100644 --- a/test-touch-targets.js +++ b/test-touch-targets.js @@ -89,6 +89,18 @@ const BUTTON_SELECTORS = [ ['.analytics-time-range button', 'button', '', '
{}
'], ['.leaflet-control-zoom a','a', '', '
{}
'], ['.live-leaflet-toggle a', 'a', '', '
{}
'], + // #314: the two controls the staging browser check still measured below + // 48 px (26 px and 18 px). Both sit in their real parent, because that is + // what decides the box: "Got it" is a flex item of .gesture-hint-inner, so + // its height is a cross-axis question, and the reveal pill is appended into + // a of the packets table by TableResponsive.apply(). The hint wrap is + // position: fixed in the app; it is pinned static here so it lays out in + // the harness flow like every other sample. + ['.gesture-hint-dismiss', 'button', 'gesture-hint-dismiss', + '
' + + 'Swipe left or right to switch tabs{}
'], + ['.col-hidden-pill', 'button', 'col-hidden-pill', + '
Time{}
x
'], ]; // Form controls and text buttons that only set a height. min-WIDTH is not