fix: keep the flash line to its one line at any message length (closes #252)
The flash line reserves one line, so a message that wrapped pushed the screen below it down. #flash-msg no longer wraps: text too long for it is cut with an ellipsis, and showFlash() puts the whole message in its title. Every message is also reworded to at most 50 characters so none is cut, and the add-token screens flash a fixed line for any error other than the two lookup messages, logging the detail. A new end-to-end test writes a message several lines long into the line and fails if the line or the screen below it moves. Model: opus-5-5
This commit was merged in pull request #420.
This commit is contained in:
@@ -99,12 +99,13 @@ describe("the flash line the message is shown in", () => {
|
||||
// length, including one that wrapped to two lines and pushed the
|
||||
// settings view down 12px.
|
||||
//
|
||||
// The assertion that actually measures — empty line vs. the message,
|
||||
// real Chromium, documented 360x600 popup — is
|
||||
// "a rejected dust threshold shifts no layout (#233)" in
|
||||
// tests/e2e/run.js, run by make test-e2e. It is not in make check
|
||||
// because REPO_POLICIES.md caps make test at 20 seconds and a browser
|
||||
// suite does not fit; run it before changing the wording.
|
||||
// The line cuts a message too long for it with an ellipsis (see
|
||||
// showFlash() in src/popup/views/helpers.js). The assertions that
|
||||
// measure that, in a real browser at the documented 360x600 popup, are
|
||||
// "a rejected dust threshold shifts no layout (#233)" and "an over-long
|
||||
// flash message keeps to one line (#252)" in tests/e2e/run.js, run by
|
||||
// make test-e2e. They are not in make check because REPO_POLICIES.md
|
||||
// caps make test at 20 seconds and a browser suite does not fit.
|
||||
test("reserves its height in the markup", () => {
|
||||
const flashLine = POPUP_HTML.match(
|
||||
/<div\s+id="flash-msg"\s+class="([^"]*)"/,
|
||||
|
||||
+62
-13
@@ -1320,17 +1320,13 @@ async function waitForFilledFlashLine(page) {
|
||||
}
|
||||
|
||||
// README, No Layout Shift: the rejection message goes into #flash-msg,
|
||||
// whose min-h-[1.25rem] reserves exactly ONE line at text-xs. Reserving
|
||||
// the space is not enough on its own — a message too long for one line
|
||||
// wraps and pushes everything below it down anyway, which is what the
|
||||
// first version of this change shipped: 75 characters, 32px, the settings
|
||||
// view and the threshold field 12px lower than with an empty line.
|
||||
//
|
||||
// So this measures rather than inspects markup. It is the only assertion
|
||||
// in the repo that can see the wording grow: the unit suite runs on the
|
||||
// node environment with no layout engine, where every height is zero (see
|
||||
// the note in tests/dustThreshold.test.js). Lengthen
|
||||
// DUST_THRESHOLD_MESSAGE past one line and this test goes red.
|
||||
// whose min-h-[1.25rem] reserves exactly ONE line at text-xs, and which
|
||||
// cuts a message too long for that line with an ellipsis rather than wrap
|
||||
// it. This shows the real message and measures that nothing moves; the
|
||||
// test after it does the same with a message several lines long. Both
|
||||
// measure rather than inspect markup: the unit suite runs on the node
|
||||
// environment with no layout engine, where every height is zero (see the
|
||||
// note in tests/dustThreshold.test.js).
|
||||
test("a rejected dust threshold shifts no layout (#233)", async (env) => {
|
||||
const page = await openPopup(env.ctx, env.popupUrl);
|
||||
try {
|
||||
@@ -1377,11 +1373,11 @@ test("a rejected dust threshold shifts no layout (#233)", async (env) => {
|
||||
);
|
||||
assert(
|
||||
after.flashHeight === before.flashHeight,
|
||||
"the message does not fit the reserved line: " +
|
||||
"the message does not keep to the reserved line: " +
|
||||
before.flashHeight +
|
||||
"px empty vs " +
|
||||
after.flashHeight +
|
||||
"px with the message. Shorten DUST_THRESHOLD_MESSAGE",
|
||||
"px with the message",
|
||||
);
|
||||
assert(
|
||||
after.settingsTop === before.settingsTop,
|
||||
@@ -1400,6 +1396,59 @@ test("a rejected dust threshold shifts no layout (#233)", async (env) => {
|
||||
}
|
||||
});
|
||||
|
||||
// ------------------------------------------------ the flash line (#252)
|
||||
|
||||
// #flash-msg never wraps: a message too long for its one line is cut with an
|
||||
// ellipsis (see showFlash() in src/popup/views/helpers.js). This puts a
|
||||
// message several lines long into it and measures that the line and the
|
||||
// screen below it stay where they were.
|
||||
test("an over-long flash message keeps to one line (#252)", async (env) => {
|
||||
const page = await openPopup(env.ctx, env.popupUrl);
|
||||
try {
|
||||
await page.setViewportSize(POPUP_VIEWPORT);
|
||||
await openSettings(page);
|
||||
|
||||
const before = await page.evaluate(measureFlashLine);
|
||||
const overflows = await page.evaluate(() => {
|
||||
const line = document.getElementById("flash-msg");
|
||||
line.textContent =
|
||||
"This message is far too long for one line. ".repeat(5);
|
||||
return line.scrollWidth > line.clientWidth;
|
||||
});
|
||||
const after = await page.evaluate(measureFlashLine);
|
||||
|
||||
assert(
|
||||
after.flashHeight === before.flashHeight,
|
||||
"the flash line is " +
|
||||
before.flashHeight +
|
||||
"px before and " +
|
||||
after.flashHeight +
|
||||
"px with an over-long message, so it wraps",
|
||||
);
|
||||
assert(
|
||||
after.settingsTop === before.settingsTop,
|
||||
"the settings view moved " +
|
||||
(after.settingsTop - before.settingsTop) +
|
||||
"px when the message appeared",
|
||||
);
|
||||
assert(
|
||||
after.fieldTop === before.fieldTop,
|
||||
"the dust threshold field moved " +
|
||||
(after.fieldTop - before.fieldTop) +
|
||||
"px when the message appeared",
|
||||
);
|
||||
// Checked last: a line that wraps does not run past its right edge,
|
||||
// so this only shows the message really was cut once nothing moved.
|
||||
assert(
|
||||
overflows,
|
||||
"the message fits on the line, so it proves nothing: " +
|
||||
JSON.stringify(after.text),
|
||||
);
|
||||
} finally {
|
||||
await page.close();
|
||||
}
|
||||
});
|
||||
|
||||
// --------------------------------------------- confirmation screen (#238)
|
||||
//
|
||||
// The screen that decides what gets signed. The arithmetic underneath it
|
||||
|
||||
@@ -0,0 +1,130 @@
|
||||
// The flash line (#252). #flash-msg reserves one line and cuts a message too
|
||||
// long for it with an ellipsis; that is measured in a real browser by
|
||||
// tests/e2e/run.js. Here: showFlash() keeps the whole message readable in the
|
||||
// line's title, and the two add-token screens flash a fixed line, not the text
|
||||
// of whatever error adding the token threw.
|
||||
|
||||
const ADDRESS = "0x1111111111111111111111111111111111111111";
|
||||
|
||||
let elements;
|
||||
|
||||
function fakeElement() {
|
||||
return {
|
||||
value: "",
|
||||
textContent: "",
|
||||
title: "",
|
||||
style: {},
|
||||
listeners: {},
|
||||
addEventListener(event, handler) {
|
||||
this.listeners[event] = handler;
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
// Stands in for document.getElementById(): one fake element per id.
|
||||
function element(id) {
|
||||
return (elements[id] ||= fakeElement());
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
jest.resetModules();
|
||||
elements = {};
|
||||
globalThis.document = { getElementById: element };
|
||||
// state.js reads chrome.storage.local at load.
|
||||
globalThis.chrome = {
|
||||
storage: { local: { get: async () => ({}), set: async () => {} } },
|
||||
};
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
jest.dontMock("../src/popup/views/helpers");
|
||||
jest.dontMock("../src/shared/state");
|
||||
jest.dontMock("../src/shared/balances");
|
||||
jest.restoreAllMocks();
|
||||
jest.useRealTimers();
|
||||
delete globalThis.document;
|
||||
delete globalThis.chrome;
|
||||
});
|
||||
|
||||
test("showFlash() puts the whole message in the title, and clears both", () => {
|
||||
jest.useFakeTimers();
|
||||
const { showFlash } = require("../src/popup/views/helpers");
|
||||
|
||||
showFlash("Saved.");
|
||||
expect(element("flash-msg").textContent).toBe("Saved.");
|
||||
expect(element("flash-msg").title).toBe("Saved.");
|
||||
|
||||
jest.advanceTimersByTime(2000);
|
||||
expect(element("flash-msg").textContent).toBe("");
|
||||
expect(element("flash-msg").title).toBe("");
|
||||
});
|
||||
|
||||
describe.each([
|
||||
["addToken", "add-token-address", "btn-add-token-confirm"],
|
||||
[
|
||||
"settingsAddToken",
|
||||
"settings-addtoken-address",
|
||||
"btn-settings-addtoken-manual",
|
||||
],
|
||||
])("adding a token on %s", (view, field, button) => {
|
||||
let flashes;
|
||||
let errors;
|
||||
|
||||
// Clicks the screen's add button with lookupTokenInfo() and saveState()
|
||||
// replaced by the given functions.
|
||||
async function add(lookupTokenInfo, saveState) {
|
||||
flashes = [];
|
||||
errors = jest.spyOn(console, "error").mockImplementation(() => {});
|
||||
jest.spyOn(console, "log").mockImplementation(() => {});
|
||||
jest.doMock("../src/shared/balances", () => ({ lookupTokenInfo }));
|
||||
jest.doMock("../src/shared/state", () => ({
|
||||
state: { trackedTokens: [] },
|
||||
saveState,
|
||||
}));
|
||||
jest.doMock("../src/popup/views/helpers", () => ({
|
||||
$: element,
|
||||
showView: () => {},
|
||||
showFlash: (msg) => flashes.push(msg),
|
||||
escapeHtml: (s) => s,
|
||||
goBack: () => {},
|
||||
}));
|
||||
|
||||
require("../src/popup/views/" + view).init({
|
||||
doRefreshAndRender: () => {},
|
||||
});
|
||||
element(field).value = ADDRESS;
|
||||
await element(button).listeners.click();
|
||||
}
|
||||
|
||||
test("a failed save flashes a fixed line and logs the error", async () => {
|
||||
const detail = "A sentence about the stored record. ".repeat(4);
|
||||
|
||||
await add(
|
||||
async () => ({ symbol: "TKN", decimals: 18, name: "Token" }),
|
||||
async () => {
|
||||
throw new Error(detail);
|
||||
},
|
||||
);
|
||||
|
||||
expect(flashes).toEqual(["Could not add the token."]);
|
||||
expect(errors).toHaveBeenCalledWith(
|
||||
"[AutistMask]",
|
||||
"Adding token failed for",
|
||||
ADDRESS,
|
||||
detail,
|
||||
);
|
||||
});
|
||||
|
||||
test("a contract that is not a token flashes the lookup message", async () => {
|
||||
const detail = "Not a valid ERC-20 token (symbol() failed).";
|
||||
|
||||
await add(
|
||||
async () => {
|
||||
throw new Error(detail);
|
||||
},
|
||||
async () => {},
|
||||
);
|
||||
|
||||
expect(flashes).toEqual([detail]);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user