diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index b839247..eb3a5ba 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -284,6 +284,12 @@ jobs: sudo apt-get install -y --no-install-recommends ffmpeg xvfb-run -a npm run test:gif + # Scrolling through a long unread run: that the view stays where it is + # read when something above it changes size, with the "new messages" bar + # up over the top of the log. + - name: scroll check + run: xvfb-run -a npm run test:scroll + # MOHO_BUILD is computed inside the npm script - a shell assignment on # the electron-builder line - so it cannot be injected from out here. # The only way to control what the artifact is called is to guarantee diff --git a/package.json b/package.json index 6faa223..2bf3e44 100644 --- a/package.json +++ b/package.json @@ -23,6 +23,7 @@ "test:ui": "electron-vite build && node scripts/ui-check.mjs", "test:tray": "electron-vite build && node scripts/tray-check.mjs", "test:gif": "electron-vite build && node scripts/gif-check.mjs", + "test:scroll": "electron-vite build && node scripts/scroll-check.mjs", "shots": "electron-vite build && node scripts/ui-shots.mjs", "pack": "npm run daemon && npm run build && MOHO_BUILD=$(git rev-parse --short HEAD)$(git diff --quiet HEAD || echo -dirty) electron-builder --linux AppImage", "pack:deb": "npm run daemon && npm run build && MOHO_BUILD=$(git rev-parse --short HEAD)$(git diff --quiet HEAD || echo -dirty) electron-builder --linux deb", diff --git a/scripts/scroll-check.mjs b/scripts/scroll-check.mjs new file mode 100644 index 0000000..6c63119 --- /dev/null +++ b/scripts/scroll-check.mjs @@ -0,0 +1,101 @@ +#!/usr/bin/env node +/** + * Checks that a reader scrolling through a long unread run is not put back. + * + * The "N new messages since" bar sits over the top of the log. What the list + * remembers about where the reader is came from asking the page what is at its + * top edge - which, while the bar is up, is the bar. So the place stopped being + * kept up to date the moment the bar appeared, and the next thing to change + * size (a card arriving, a picture loading) put the view back where the + * reader had been when it appeared. + * + * npm run test:scroll (builds first; needs the daemon built too) + */ +import { fakeIrc, launchApp, openIrcChannel, sleep, until } from './lib/harness.mjs' + +let failures = 0 +const check = (ok, what, detail = '') => { + console.log(`${ok ? 'ok ' : 'FAIL'} ${what}${ok ? '' : detail ? ` - ${detail}` : ''}`) + if (!ok) failures++ +} +setTimeout(() => { + console.log('FAIL timed out') + process.exit(1) +}, 90000).unref() + +const irc = await fakeIrc({ + people: ['bob'], + onJoin: (channel, _nick, send) => send(`:bob!b@h PRIVMSG ${channel} :hello there`) +}) +const { page, stop } = await launchApp({ env: { MOHO_UI_SHOTS: '1' } }) +try { + await until(page, 'window.__mohoShots', 'the harness hook') + await openIrcChannel(page, { port: irc.address().port, nick: 'checker', channels: ['#lobby'], waitFor: 'hello there' }) + + // Three hundred messages, the last two hundred of them unread. + await page.evaluate(`(() => { + const shots = window.__mohoShots + const s = shots.state() + const id = s.activeBufferId + const now = Math.floor(Date.now() / 1000) + const list = Array.from({ length: 300 }, (_, n) => ({ + id: 'm' + n, bufferId: id, from: n % 3 ? 'bob' : 'carol', senderId: n % 3 ? 'bob' : 'carol', + body: 'message number ' + n + ' with a little text in it', ts: now - 40000 + n * 60, + isAction: false, isHighlight: false, kind: 'chat', isOwn: false + })) + shots.patch({ + messagesByBuffer: { ...s.messagesByBuffer, [id]: list }, + dividerTsByBuffer: { ...s.dividerTsByBuffer, [id]: list[99].ts } + }) + })()`) + await until(page, `!!document.querySelector('.new-divider')`, 'the New line') + await sleep(600) + + // Reading down past the line, the way a wheel does: the bar comes up. + await page.evaluate(`(async () => { + const el = document.querySelector('.messagelist-scroll') + el.dispatchEvent(new WheelEvent('wheel', { deltaY: 100, bubbles: true })) + for (let i = 0; i < 8; i++) { + el.scrollTop += 120 + el.dispatchEvent(new WheelEvent('wheel', { deltaY: 120, bubbles: true })) + await new Promise((r) => setTimeout(r, 60)) + } + })()`) + await until(page, `!!document.querySelector('.unread-bar')`, 'the unread bar') + + // Further down, with the bar over the top of the log the whole way. + await page.evaluate(`(async () => { + const el = document.querySelector('.messagelist-scroll') + for (let i = 0; i < 12; i++) { + el.scrollTop += 150 + el.dispatchEvent(new WheelEvent('wheel', { deltaY: 150, bubbles: true })) + await new Promise((r) => setTimeout(r, 60)) + } + })()`) + await sleep(300) + const before = await page.evaluate(`document.querySelector('.messagelist-scroll').scrollTop`) + + // Something above the reader settles taller - a card arriving, a picture + // loading. The row under the reader's eye is what must stay put. + const moved = await page.evaluate(`(async () => { + const el = document.querySelector('.messagelist-scroll') + const box = el.getBoundingClientRect() + const rows = [...el.querySelectorAll('[data-msg-id]')] + const watched = rows.find((r) => r.getBoundingClientRect().top > box.top + 60) + const was = watched.getBoundingClientRect().top - box.top + const above = rows.find((r) => r.getBoundingClientRect().bottom < box.top - 50) + above.style.minHeight = (above.getBoundingClientRect().height + 240) + 'px' + await new Promise((r) => setTimeout(r, 400)) + return { id: watched.dataset.msgId, drift: watched.getBoundingClientRect().top - box.top - was } + })()`) + check(Math.abs(moved.drift) < 2, 'a row above growing leaves the row being read where it was', `moved ${moved.drift}px`) + + // And the position itself did not fall back towards where the bar appeared. + const after = await page.evaluate(`document.querySelector('.messagelist-scroll').scrollTop`) + check(after >= before - 4, 'the view did not go back up the log', `${before} -> ${after}`) +} finally { + await stop() + irc.close() +} +console.log(failures ? `${failures} FAILED` : 'all passed') +process.exit(failures ? 1 : 0) diff --git a/src/renderer/src/components/MessageList.tsx b/src/renderer/src/components/MessageList.tsx index 8c585f1..1c95499 100644 --- a/src/renderer/src/components/MessageList.tsx +++ b/src/renderer/src/components/MessageList.tsx @@ -52,6 +52,9 @@ const REANCHOR_THRESHOLD = 100 */ const SCROLL_JITTER = 2 +/** How far down from the top edge to look for the row being read, nearest first. */ +const PLACE_PROBES = [1, 24, 48, 80, 120, 180] + /** * How long after the reader last touched the list a scroll still counts as * theirs. @@ -363,17 +366,29 @@ export function MessageList(): JSX.Element { * * By hit test rather than by walking the rows: this runs on every scroll, * and a list of two thousand is not a thing to scan that often. + * + * Every element under the point, not the topmost one, and a few points down + * the page rather than one. The "N new messages since" bar is laid over the + * top of the log, so asking what was at the edge was answered with the bar + * for as long as it was up - and the place stopped being kept, which the + * next thing to change size then put the view back to. A date line or the + * gap between two groups at the edge gave the same non-answer for a moment. */ const notePlace = useCallback(() => { const el = scrollRef.current if (!el) return const box = el.getBoundingClientRect() - const hit = document.elementFromPoint(box.left + 8, box.top + 1) - const row = hit?.closest?.('[data-msg-id]') as HTMLElement | null - if (!row) return - placeRef.current = { - id: row.dataset.msgId ?? '', - offset: row.getBoundingClientRect().top - box.top + for (const down of PLACE_PROBES) { + for (const hit of document.elementsFromPoint(box.left + 8, box.top + down)) { + if (!el.contains(hit)) continue + const row = hit.closest?.('[data-msg-id]') as HTMLElement | null + if (!row) continue + placeRef.current = { + id: row.dataset.msgId ?? '', + offset: row.getBoundingClientRect().top - box.top + } + return + } } }, [])