diff --git a/README.md b/README.md index ee2bf5a..4f88cac 100644 --- a/README.md +++ b/README.md @@ -610,6 +610,15 @@ ExportPrivKey and ShowRecoveryPhrase — are deliberately absent from that list, so the popup can never reopen onto one of them with no password prompt in front of it. +A reopened popup renders the wallet list and the one screen it restores onto, +and nothing else, so every screen on the stack behind that one is still the +blank template from `index.html`. "Back" therefore renders its target rather +than only unhiding it, through the same dispatch and data guards as the restore +(`src/popup/viewRouter.js`), and falls back to Home when the state the target +would render is gone. Forward navigation renders as it goes and does not go +through that dispatch: rendering a screen a second time would re-fetch and +clobber whatever it has in flight. + Every screen that holds secret material in the page registers a cleanup with `onViewLeave()` (`src/popup/views/helpers.js`), which `showView()` runs on every exit from that screen rather than only on its "Back" button, so nothing secret diff --git a/TODO.md b/TODO.md index ab9ae00..9ce1702 100644 --- a/TODO.md +++ b/TODO.md @@ -45,6 +45,19 @@ undefined identifiers, which is how # Completed Steps +- 2026-08-12: "Back" now renders the screen it lands on instead of only unhiding + it. A reopened popup renders the wallet list and the one screen it restores + onto, so every screen further down the stack was still the blank template from + `index.html`, and Back walked straight onto it — an empty address, no + balances, no QR code. The Back path now goes through the same per-view + dispatch and data guards as the restore (`src/popup/viewRouter.js`, shared + with `restoreView()`), falling back to Home when the state the target would + render is gone, and declining any view the popup does not render from + persisted state, which can only be on the stack from the current page load and + was rendered on the way in. Forward navigation is untouched, so nothing + renders twice. Covered by unit tests on the real `goBack()` and by two + end-to-end cases against the real popup, both demonstrated failing on the + unfixed build ([#268](https://git.eeqj.de/sneak/AutistMask/issues/268)). - 2026-08-12: A containerized Firefox end-to-end harness (`make test-e2e-firefox`) drives the real popup in a real Firefox with the MV2 build installed as a temporary add-on. Zero npm dependencies — a WebDriver diff --git a/src/popup/index.js b/src/popup/index.js index b609adc..4185491 100644 --- a/src/popup/index.js +++ b/src/popup/index.js @@ -9,16 +9,17 @@ const { $, showView, updateDebugBanner, - setRenderMain, + setBackRenderer, pushCurrentView, goBack, clearViewStack, } = require("./views/helpers"); const { applyTheme } = require("./theme"); -// Views that can be fully re-rendered from persisted state. All others fall -// back to the nearest restorable parent; see the module for why the -// secret-bearing views are absent. -const { RESTORABLE_VIEWS } = require("./restorableViews"); +// Renders a view the popup lands on without having navigated to it forward: +// on restore here, and on Back. Only the views that can be fully re-rendered +// from persisted state (RESTORABLE_VIEWS, src/popup/restorableViews.js) go +// through it; anything else falls back to the nearest restorable parent. +const { renderView, makeBackRenderer } = require("./viewRouter"); const home = require("./views/home"); const welcome = require("./views/welcome"); @@ -108,91 +109,22 @@ const ctx = { }, }; -function needsAddress(view) { - return ( - view === "address" || - view === "address-token" || - view === "receive" || - view === "transaction" - ); -} - -function hasValidAddress() { - return ( - state.selectedWallet !== null && - state.selectedAddress !== null && - state.wallets[state.selectedWallet] && - state.wallets[state.selectedWallet].addresses[state.selectedAddress] - ); -} +// The view modules the router renders through, keyed as it expects them. +const viewModules = { + main: { show: () => fallbackView() }, + addressDetail, + addressToken, + receive, + settings, + settingsAddToken, + confirmTx, + transactionDetail, + txStatus, +}; function restoreView() { - const view = state.currentView; - if (!view || !RESTORABLE_VIEWS.has(view)) { - return fallbackView(); - } - - if (needsAddress(view) && !hasValidAddress()) { - return fallbackView(); - } - - if (view === "address-token" && !state.selectedToken) { - return fallbackView(); - } - - switch (view) { - case "address": - addressDetail.show(); - break; - case "address-token": - addressToken.show(); - break; - case "receive": - receive.show(); - break; - case "settings": - settings.show(); - break; - case "settings-addtoken": - settingsAddToken.show(); - break; - case "confirm-tx": - if (state.viewData && state.viewData.pendingTx) { - confirmTx.restore(); - } else { - fallbackView(); - } - break; - case "transaction": - if (state.viewData && state.viewData.tx) { - transactionDetail.render(); - } else { - fallbackView(); - } - break; - case "wait-tx": - // Resumes the receipt poll from the persisted broadcast time. - if (!txStatus.restoreWait()) { - fallbackView(); - } - break; - case "success-tx": - if (state.viewData && state.viewData.hash) { - txStatus.renderSuccess(); - } else { - fallbackView(); - } - break; - case "error-tx": - if (state.viewData && state.viewData.message) { - txStatus.renderError(); - } else { - fallbackView(); - } - break; - default: - fallbackView(); - break; + if (!renderView(state.currentView, state, viewModules)) { + fallbackView(); } } @@ -247,7 +179,7 @@ async function init() { settings.show(); }); - setRenderMain(renderWalletList); + setBackRenderer(makeBackRenderer(state, viewModules)); welcome.init(ctx); addWallet.init(ctx); diff --git a/src/popup/viewRouter.js b/src/popup/viewRouter.js new file mode 100644 index 0000000..efc318d --- /dev/null +++ b/src/popup/viewRouter.js @@ -0,0 +1,116 @@ +// Rendering a view the popup lands on without having navigated to it +// forward: on restore, and on Back. In both cases the view may never have +// been rendered in this page load — a reopened popup renders only the +// wallet list and the view it restores onto, so every other view is still +// the blank static template from index.html — so unhiding it is not enough. +// +// Forward navigation renders as it goes and must NOT come through here: +// rendering a second time would re-fetch and clobber whatever the view has +// in flight. +// +// The view modules are injected and nothing here touches the DOM, so the +// dispatch and its data guards can be tested directly; src/popup/index.js +// cannot be required outside a browser. + +const { RESTORABLE_VIEWS } = require("./restorableViews"); + +// Views that render an address the user picked and cannot be rendered +// without one. +const ADDRESS_VIEWS = new Set([ + "address", + "address-token", + "receive", + "transaction", +]); + +function needsAddress(view) { + return ADDRESS_VIEWS.has(view); +} + +function hasValidAddress(state) { + return Boolean( + state.selectedWallet !== null && + state.selectedAddress !== null && + state.wallets[state.selectedWallet] && + state.wallets[state.selectedWallet].addresses[state.selectedAddress], + ); +} + +// Render `view` from persisted state. Each view module shows itself, so a +// true return means the view is both rendered and on screen. +// +// Returns false when the view is not one the popup renders from state, or +// when the state it would render is gone — a token no longer selected, a +// transaction no longer persisted. The caller falls back rather than +// putting an empty template on screen. +function renderView(view, state, views) { + if (!view || !RESTORABLE_VIEWS.has(view)) return false; + if (needsAddress(view) && !hasValidAddress(state)) return false; + if (view === "address-token" && !state.selectedToken) return false; + + const data = state.viewData || {}; + switch (view) { + case "main": + views.main.show(); + return true; + case "address": + views.addressDetail.show(); + return true; + case "address-token": + views.addressToken.show(); + return true; + case "receive": + views.receive.show(); + return true; + case "settings": + views.settings.show(); + return true; + case "settings-addtoken": + views.settingsAddToken.show(); + return true; + case "confirm-tx": + if (!data.pendingTx) return false; + views.confirmTx.restore(); + return true; + case "transaction": + if (!data.tx) return false; + views.transactionDetail.render(); + return true; + case "wait-tx": + // Resumes the receipt poll from the persisted broadcast time, + // and answers false when there is nothing resumable left. + return Boolean(views.txStatus.restoreWait()); + case "success-tx": + if (!data.hash) return false; + views.txStatus.renderSuccess(); + return true; + case "error-tx": + if (!data.message) return false; + views.txStatus.renderError(); + return true; + default: + return false; + } +} + +// The Back-path renderer, registered with setBackRenderer() in +// views/helpers.js. Returns false for a view the popup does not render from +// persisted state, leaving goBack() to unhide it: the restored stack is +// filtered against RESTORABLE_VIEWS, so such a view can only be on the +// stack from this page load, where forward navigation already rendered it. +function makeBackRenderer(state, views) { + return function renderBack(view) { + if (!RESTORABLE_VIEWS.has(view)) return false; + if (!renderView(view, state, views)) { + views.main.show(); + } + return true; + }; +} + +module.exports = { + renderView, + makeBackRenderer, + needsAddress, + hasValidAddress, +}; diff --git a/src/popup/views/helpers.js b/src/popup/views/helpers.js index ab8468a..9824e36 100644 --- a/src/popup/views/helpers.js +++ b/src/popup/views/helpers.js @@ -111,12 +111,19 @@ function updateDebugBanner(viewName) { } } -// Callback to re-render the main/home view when navigating back to it. -// Set once by index.js via setRenderMain(). -let _renderMain = null; +// Callback that renders a view being navigated BACK onto. Set once by +// index.js via setBackRenderer(), which routes the view through the same +// per-view render and data guards restoreView() uses. +// +// It answers true when it took the navigation — the view is rendered and +// shown, or its backing data was gone and it fell back — and false for a +// view the popup does not render from persisted state. Those can only be +// on the stack from this page load, because the stack is filtered on load, +// so they have already been rendered and only need unhiding. +let _renderBack = null; -function setRenderMain(fn) { - _renderMain = fn; +function setBackRenderer(fn) { + _renderBack = fn; } // Push the current view onto the navigation stack so goBack() can @@ -136,9 +143,11 @@ function goBack() { } else { target = "main"; } - if (target === "main" && _renderMain) { - _renderMain(); - } + // A popped view is landed on, not navigated to. If the popup has been + // closed and reopened since the view was pushed, nothing has ever + // rendered it in this page load and its template is still blank, so it + // has to be rendered here rather than merely unhidden. + if (_renderBack && _renderBack(target)) return; showView(target); } @@ -470,7 +479,7 @@ module.exports = { showView, onViewLeave, updateDebugBanner, - setRenderMain, + setBackRenderer, pushCurrentView, goBack, clearViewStack, diff --git a/tests/backNavigation.test.js b/tests/backNavigation.test.js new file mode 100644 index 0000000..2beef08 --- /dev/null +++ b/tests/backNavigation.test.js @@ -0,0 +1,243 @@ +// Back after reopening the popup (#268). +// +// A reopened popup renders the wallet list and the one view it restores +// onto; every other view is still the blank static template from +// index.html. goBack() used to only unhide its target, so Back landed on +// that blank template for any view the popup had not rendered in this page +// load. These tests drive the real goBack() with the real router wired to +// recording view modules, so what is asserted is which view render ran — +// the thing that was missing. +// +// The rendering itself is asserted against the real popup in a real +// browser by tests/e2e/run.js; here the DOM is a stub, because goBack() +// only needs showView() to work. + +const els = new Map(); + +function fakeEl() { + return { + textContent: "", + innerHTML: "", + classList: { + toggle() {}, + add() {}, + remove() {}, + contains: () => false, + }, + remove() {}, + }; +} + +globalThis.document = { + getElementById(id) { + if (!els.has(id)) els.set(id, fakeEl()); + return els.get(id); + }, +}; + +// helpers.js pulls in state.js, which reads chrome.storage.local at load. +globalThis.chrome = { + storage: { local: { get: async () => ({}), set: async () => {} } }, +}; + +const { + showView, + goBack, + setBackRenderer, + pushCurrentView, +} = require("../src/popup/views/helpers"); +const { makeBackRenderer } = require("../src/popup/viewRouter"); +const { state } = require("../src/shared/state"); + +const ADDRESS = "0x1111111111111111111111111111111111111111"; +const TOKEN = "0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48"; + +let calls; + +// Stand-ins for the view modules. Each records itself and then shows its +// view, which is what every real view render ends with — so the assertions +// can tell "rendered and shown" apart from "merely unhidden". +function recorder(name, view) { + return () => { + calls.push(name); + showView(view); + }; +} + +function makeViews() { + return { + main: { show: recorder("main", "main") }, + addressDetail: { show: recorder("addressDetail", "address") }, + addressToken: { show: recorder("addressToken", "address-token") }, + receive: { show: recorder("receive", "receive") }, + settings: { show: recorder("settings", "settings") }, + settingsAddToken: { + show: recorder("settingsAddToken", "settings-addtoken"), + }, + confirmTx: { restore: recorder("confirmTx", "confirm-tx") }, + transactionDetail: { + render: recorder("transactionDetail", "transaction"), + }, + txStatus: { + restoreWait: () => { + calls.push("waitTx"); + showView("wait-tx"); + return true; + }, + renderSuccess: recorder("successTx", "success-tx"), + renderError: recorder("errorTx", "error-tx"), + }, + }; +} + +// The popup as it stands just after a reopen: one wallet with one address, +// the view the popup restored onto, and the stack behind it. +function reopenedOn(view, stack, extra) { + calls = []; + state.wallets = [ + { + name: "Wallet 1", + addresses: [{ address: ADDRESS, balance: "0", tokenBalances: [] }], + }, + ]; + state.selectedWallet = 0; + state.selectedAddress = 0; + state.selectedToken = null; + state.viewData = null; + state.currentView = view; + state.viewStack = stack.slice(); + Object.assign(state, extra || {}); + setBackRenderer(makeBackRenderer(state, makeViews())); +} + +// The reproduction from the issue, step for step. +describe("Back onto a view the reopened popup never rendered", () => { + test("Back from settings renders the address detail underneath", () => { + reopenedOn("settings", ["main", "address"]); + goBack(); + expect(calls).toEqual(["addressDetail"]); + expect(state.currentView).toBe("address"); + expect(state.viewStack).toEqual(["main"]); + }); + + test("Back onto the token detail renders it", () => { + reopenedOn("settings", ["main", "address", "address-token"], { + selectedToken: TOKEN, + }); + goBack(); + expect(calls).toEqual(["addressToken"]); + expect(state.currentView).toBe("address-token"); + }); + + test("Back onto Receive renders it", () => { + reopenedOn("settings", ["main", "address", "receive"]); + goBack(); + expect(calls).toEqual(["receive"]); + expect(state.currentView).toBe("receive"); + }); + + test("Back onto the transaction detail renders it", () => { + reopenedOn("settings", ["main", "transaction"], { + viewData: { tx: { hash: "0xdead" } }, + }); + goBack(); + expect(calls).toEqual(["transactionDetail"]); + expect(state.currentView).toBe("transaction"); + }); + + test("Back onto the transaction confirmation restores it", () => { + reopenedOn("settings", ["main", "confirm-tx"], { + viewData: { pendingTx: { to: ADDRESS, amount: "1" } }, + }); + goBack(); + expect(calls).toEqual(["confirmTx"]); + expect(state.currentView).toBe("confirm-tx"); + }); + + test("Back onto Home renders the wallet list", () => { + reopenedOn("settings", ["main"]); + goBack(); + expect(calls).toEqual(["main"]); + expect(state.currentView).toBe("main"); + }); + + test("Back with an empty stack renders Home", () => { + reopenedOn("settings", []); + goBack(); + expect(calls).toEqual(["main"]); + expect(state.currentView).toBe("main"); + }); +}); + +// The guards are restoreView()'s, so a popped view whose backing data is +// gone lands on Home rather than on an empty template. +describe("Back onto a view whose backing data is gone", () => { + test("the token detail with no token selected falls back to Home", () => { + reopenedOn("settings", ["main", "address-token"]); + goBack(); + expect(calls).toEqual(["main"]); + expect(state.currentView).toBe("main"); + }); + + test("the transaction detail with no transaction falls back to Home", () => { + reopenedOn("settings", ["main", "transaction"]); + goBack(); + expect(calls).toEqual(["main"]); + expect(state.currentView).toBe("main"); + }); + + test("the confirmation with no pending transaction falls back to Home", () => { + reopenedOn("settings", ["main", "confirm-tx"]); + goBack(); + expect(calls).toEqual(["main"]); + expect(state.currentView).toBe("main"); + }); + + test("an address view with no address selected falls back to Home", () => { + reopenedOn("settings", ["main", "receive"], { + selectedAddress: null, + }); + goBack(); + expect(calls).toEqual(["main"]); + expect(state.currentView).toBe("main"); + }); + + test("a wait that can no longer be resumed falls back to Home", () => { + reopenedOn("settings", ["main", "wait-tx"]); + const views = makeViews(); + views.txStatus.restoreWait = () => false; + setBackRenderer(makeBackRenderer(state, views)); + goBack(); + expect(calls).toEqual(["main"]); + expect(state.currentView).toBe("main"); + }); +}); + +// Forward navigation renders as it goes. Re-rendering a second time would +// re-fetch and clobber whatever the view has in flight, so the Back path is +// the only place this happens — and it declines any view the popup does not +// render from persisted state, since the restored stack is filtered against +// that same set and such a view can only have been rendered in this page +// load. +describe("what the Back path leaves alone", () => { + test("forward navigation renders nothing by itself", () => { + reopenedOn("address", ["main"]); + pushCurrentView(); + showView("send"); + expect(calls).toEqual([]); + expect(state.viewStack).toEqual(["main", "address"]); + }); + + test("Back onto a live-session view only unhides it", () => { + reopenedOn("confirm-tx", ["main", "address", "send"]); + goBack(); + expect(calls).toEqual([]); + expect(state.currentView).toBe("send"); + }); + + test("Back renders its target exactly once", () => { + reopenedOn("settings", ["main", "address"]); + goBack(); + expect(calls.filter((c) => c === "addressDetail")).toHaveLength(1); + }); +}); diff --git a/tests/e2e/run.js b/tests/e2e/run.js index 918dbac..d94432e 100644 --- a/tests/e2e/run.js +++ b/tests/e2e/run.js @@ -406,6 +406,136 @@ test("reopening the popup never lands on the phrase screen (#161)", async (env) assertWiped(st, env.phrase, "after reopening the popup"); }); +// ------------------------------- Back after reopening the popup (#268) + +// A reopened popup renders the wallet list and the view it restores onto, +// and nothing else: every other screen is still the blank static template +// from index.html. Back used to only unhide its target, which is why these +// have to run against the real popup — the template is present and +// well-formed, so only its emptiness distinguishes the defect, and only a +// real reopen produces it. + +// Everything the address screen must have on it, read out of the DOM. +function addressScreenState(page) { + return page.evaluate(() => { + const line = document.getElementById("address-line"); + const balances = document.getElementById("address-balances"); + return { + hidden: document + .getElementById("view-address") + .classList.contains("hidden"), + line: line ? line.innerText.trim() : "", + balances: balances ? balances.innerText.trim() : "", + }; + }); +} + +// Close and reopen the page rather than reload it: that is what the toolbar +// popup does, and it is the only thing that produces the unrendered views. +async function reopenPopup(env, restoredView) { + await env.page.close(); + env.page = await openPopup(env.ctx, env.popupUrl); + await visible(env.page, restoredView); +} + +// The reproduction from the issue, step for step. +test("Back after reopening the popup renders the address screen (#268)", async (env) => { + await openAddressDetail(env.page); + const before = await addressScreenState(env.page); + assert( + before.line.length > 0, + "the address screen was blank to begin with", + ); + + await env.page.click("#btn-settings"); + await visible(env.page, "#view-settings"); + + await reopenPopup(env, "#view-settings"); + + await env.page.click("#btn-settings-back"); + await visible(env.page, "#view-address"); + + const after = await addressScreenState(env.page); + assert( + after.line === before.line, + "the address line reads " + + JSON.stringify(after.line) + + ", expected " + + JSON.stringify(before.line), + ); + assert( + after.balances.includes("ETH"), + "the balances read " + JSON.stringify(after.balances), + ); +}); + +// The same defect one screen further in. Receive holds the address twice +// over — as text and as the QR code the sender scans — and a blank one is +// worse than a missing screen. +// Everything the Receive screen must have on it. The QR code is read as +// pixels, not as an element: the blank template carries the canvas too, a +// default 300x150 one with nothing drawn on it and every pixel fully +// transparent. A drawn QR paints an opaque background across the whole +// canvas, so a single opaque pixel is the whole question. +function receiveScreenState(page) { + return page.evaluate(() => { + const block = document.getElementById("receive-address-block"); + const canvas = document.getElementById("receive-qr"); + const px = canvas + .getContext("2d") + .getImageData(0, 0, canvas.width, canvas.height).data; + let opaque = 0; + for (let i = 3; i < px.length; i += 4) { + if (px[i] > 0) opaque += 1; + } + return { + address: block.dataset.full || "", + text: block.innerText.trim(), + qrOpaquePixels: opaque, + }; + }); +} + +test("Back after reopening the popup renders the Receive screen (#268)", async (env) => { + await openAddressDetail(env.page); + await env.page.click("#btn-receive"); + await visible(env.page, "#view-receive"); + const before = await receiveScreenState(env.page); + assert( + /^0x[0-9a-fA-F]{40}$/.test(before.address), + "Receive showed no address to begin with: " + + JSON.stringify(before.address), + ); + + await env.page.click("#btn-settings"); + await visible(env.page, "#view-settings"); + + await reopenPopup(env, "#view-settings"); + + await env.page.click("#btn-settings-back"); + await visible(env.page, "#view-receive"); + + const shown = await receiveScreenState(env.page); + assert( + shown.address === before.address, + "Receive shows " + + JSON.stringify(shown.address) + + ", expected " + + JSON.stringify(before.address), + ); + assert( + shown.text.includes(before.address), + "the Receive address is not on screen: " + JSON.stringify(shown.text), + ); + assert(shown.qrOpaquePixels > 0, "Receive shows an unpainted QR code"); + + // Leave the suite where it found it. + await env.page.click("#btn-receive-back"); + await visible(env.page, "#view-address"); + await env.page.click("#btn-address-back"); + await visible(env.page, "#view-main"); +}); + // -------------------------------------------- address removal (#162) // Number of address rows across every wallet in the list, counted in the DOM