diff --git a/client/src/common/components/LanguageDialog/styles.sass b/client/src/common/components/LanguageDialog/styles.sass index 9d3157d3b..3f9e2311d 100644 --- a/client/src/common/components/LanguageDialog/styles.sass +++ b/client/src/common/components/LanguageDialog/styles.sass @@ -31,6 +31,12 @@ $dialog-chrome-allowance: 13rem // over a language when the list ran out. overscroll-behavior: contain padding-right: 0.25rem + // Slack under the last row, because scrollTop clamps on integers while a + // 125% or 150% display lays the rows out on fractions: at maximum scroll + // the last row's bottom sat a third of a pixel below the crop line, and the + // hover repaint decided whether its straddling border was drawn at all. + // With the slack, the clamp error eats padding rather than border. + padding-bottom: 0.25rem @media screen and (max-height: 500px) diff --git a/server/routes/speedtests.js b/server/routes/speedtests.js index 2f596cf7e..bef628ea8 100644 --- a/server/routes/speedtests.js +++ b/server/routes/speedtests.js @@ -178,7 +178,15 @@ app.get("/status", password(true), async (req, res) => { * Null is the answer this route already gives when nothing is scheduled, so * the status bar's existing branch for that covers the visitor too. */ - const nextTest = isUntrustedReader(req) ? null : timer.nextRun( + // The run that has already fired and is sleeping its schedule offset, + // asked ahead of the cron: during that sleep the cron's next occurrence is + // the slot AFTER the pending one, so the bar rolled from "~19:00" to + // "~19:30" while the 19:00 test was still on its way - which read as it + // having been skipped. The wake moment is exact, so the approximation + // flag below drops with it. + const pendingRun = timer.pendingRunAt(); + + const nextTest = isUntrustedReader(req) ? null : pendingRun ?? timer.nextRun( await config.getValue("cron"), { // The quiet window too, or the countdown names a test the scheduler @@ -209,8 +217,10 @@ app.get("/status", password(true), async (req, res) => { nextTest, // The offset delays each run by up to a few minutes so that every // instance does not test on the same tick, which makes the cron time the - // earliest it could start rather than when it will. - nextTestApproximate: nextTest !== null && await config.getValue("scheduleOffset") === "true" + // earliest it could start rather than when it will. A pending run's wake + // moment is not an estimate, so it is announced without the tilde. + nextTestApproximate: nextTest !== null && pendingRun === null + && await config.getValue("scheduleOffset") === "true" }); }); diff --git a/server/tasks/timer.js b/server/tasks/timer.js index 5c8faf46e..130ecca7e 100644 --- a/server/tasks/timer.js +++ b/server/tasks/timer.js @@ -71,7 +71,9 @@ export const currentJob = () => job; * caller decides what to do about it by asking scheduleChangedSince. */ export const delayRun = (ms) => new Promise((resolve) => { - const entry = {resolve}; + // The wake moment travels with the handle so pendingRunAt below can name + // it; the status bar has no other way to know a run is merely asleep. + const entry = {resolve, until: Date.now() + ms}; entry.id = setTimeout(() => { pendingDelays.delete(entry); @@ -81,6 +83,29 @@ export const delayRun = (ms) => new Promise((resolve) => { pendingDelays.add(entry); }); +/** + * When the run currently sleeping its schedule offset will wake, or null when + * none is. + * + * The status bar's countdown is cron arithmetic from now, and the offset makes + * that wrong for the whole of the sleep: the 19:00 job has fired, the run is + * asleep until 19:03, and the cron's next occurrence is already 19:30 - so the + * bar rolled to the next slot while the 19:00 test was still on its way, which + * read as it having been skipped. /status asks this first and only falls back + * to the cron when nothing is pending. + * + * The earliest entry answers, though the set only ever holds one today: there + * is a single job, and every teardown releases the delays it started. + */ +export const pendingRunAt = () => { + let earliest = null; + + for (const {until} of pendingDelays) + if (earliest === null || until < earliest) earliest = until; + + return earliest === null ? null : new Date(earliest).toISOString(); +}; + const calculateMaxDelay = (cron) => { try { const parser = CronExpressionParser.parse(cron); diff --git a/tests/client/dropdownEscapeConsistency.test.js b/tests/client/dropdownEscapeConsistency.test.js new file mode 100644 index 000000000..f36dbe317 --- /dev/null +++ b/tests/client/dropdownEscapeConsistency.test.js @@ -0,0 +1,219 @@ +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; +import { readSource, withoutJsComments } from "../helpers/source.js"; + +const dropdownSource = readSource("client/src/common/components/Dropdown/DropdownComponent.jsx"); +const dialogSource = readSource("client/src/common/contexts/Dialog/DialogContext.jsx"); +const alertSource = readSource("client/src/common/contexts/Alert/AlertContext.jsx"); +const chartSource = readSource("client/src/common/components/ChartModal/ChartModal.jsx"); +const pickerSource = readSource("client/src/common/components/DateRangePicker/DateRangePicker.jsx"); + +describe("DropdownComponent keyboard event inconsistency verification", () => { + it("verifies DropdownComponent listens to keyup instead of keydown", () => { + assert.match(dropdownSource, /document\.addEventListener\("keyup",\s*onPress\)/, + "DropdownComponent must be listening to keyup"); + assert.doesNotMatch(dropdownSource, /document\.addEventListener\("keydown"/, + "DropdownComponent does not yet listen to keydown"); + }); + + it("verifies DropdownComponent checks event.code === 'Escape' instead of event.key", () => { + assert.match(dropdownSource, /event\.code === "Escape"/, + "DropdownComponent checks event.code rather than event.key"); + assert.doesNotMatch(dropdownSource, /event\.key === "Escape"/, + "DropdownComponent does not yet use event.key"); + }); + + it("verifies DialogContext, AlertContext, ChartModal, and DateRangePicker listen to keydown with event.key === 'Escape'", () => { + assert.match(dialogSource, /document\.addEventListener\("keydown"/); + assert.match(dialogSource, /e\.key === "Escape"/); + assert.match(dialogSource, /e\.preventDefault\(\)/); + assert.match(dialogSource, /e\.defaultPrevented/); + + assert.match(alertSource, /document\.addEventListener\("keydown"/); + assert.match(alertSource, /e\.key === "Escape"/); + assert.match(alertSource, /e\.preventDefault\(\)/); + assert.match(alertSource, /e\.defaultPrevented/); + + assert.match(chartSource, /document\.addEventListener\("keydown"/); + assert.match(chartSource, /e\.key !== "Escape"/); + assert.match(chartSource, /e\.preventDefault\(\)/); + + assert.match(pickerSource, /document\.addEventListener\("keydown"/); + assert.match(pickerSource, /event\.key !== "Escape"/); + assert.match(pickerSource, /event\.preventDefault\(\)/); + }); + + it("verifies DropdownComponent lacks preventDefault and defaultPrevented checks", () => { + const effectSlice = dropdownSource.slice( + dropdownSource.indexOf("useEffect(() => {"), + dropdownSource.indexOf("[isOpen, switchDropdown]);") + ); + assert.doesNotMatch(effectSlice, /preventDefault/, + "DropdownComponent does not call preventDefault()"); + assert.doesNotMatch(effectSlice, /defaultPrevented/, + "DropdownComponent does not inspect defaultPrevented"); + }); +}); + +describe("Proof of event mismatch when modal is open over dropdown", () => { + const createEvent = (type, key, code, defaultPrevented = false) => { + let prevented = defaultPrevented; + return { + type, + key, + code, + get defaultPrevented() { + return prevented; + }, + preventDefault() { + prevented = true; + } + }; + }; + + it("demonstrates how keydown (dialog) and keyup (dropdown) cause double dismissal", () => { + let dialogClosed = false; + let dropdownClosed = false; + + // Current DialogContext handler (on keydown) + const dialogKeyDownHandler = (e) => { + if (e.defaultPrevented) return; + if (e.key === "Escape") { + e.preventDefault(); + dialogClosed = true; + } + }; + + // Current DropdownComponent handler (on keyup) + const dropdownKeyUpHandler = (e) => { + if (e.code === "Escape") { + dropdownClosed = true; + } + }; + + // User presses Escape: browser dispatches keydown + const keyDownEvent = createEvent("keydown", "Escape", "Escape", false); + dialogKeyDownHandler(keyDownEvent); + + assert.equal(dialogClosed, true, "Dialog closed on keydown"); + assert.equal(keyDownEvent.defaultPrevented, true, "Dialog called preventDefault on keydown"); + assert.equal(dropdownClosed, false, "Dropdown did not close on keydown"); + + // User releases Escape: browser dispatches keyup (fresh Event object in DOM) + const keyUpEvent = createEvent("keyup", "Escape", "Escape", false); + dropdownKeyUpHandler(keyUpEvent); + + assert.equal(dropdownClosed, true, + "Dropdown closed on keyup despite dialog consuming the Escape press! Mismatch proved."); + }); + + it("demonstrates event.code failure on remapped or non-standard keyboards", () => { + let dropdownClosed = false; + const currentDropdownHandler = (e) => { + if (e.code === "Escape") { + dropdownClosed = true; + } + }; + + // A remapped Escape (e.g. CapsLock mapped to Escape or virtual key) + const remappedEvent = createEvent("keyup", "Escape", "CapsLock", false); + currentDropdownHandler(remappedEvent); + + assert.equal(dropdownClosed, false, + "event.code === 'Escape' failed to recognize remapped Escape key"); + }); +}); + +describe("Standardized Dropdown handler on keydown and event.key === 'Escape'", () => { + const createEvent = (type, key, code, defaultPrevented = false) => { + let prevented = defaultPrevented; + return { + type, + key, + code, + get defaultPrevented() { + return prevented; + }, + preventDefault() { + prevented = true; + } + }; + }; + + const createStandardizedHandler = (isOpen, switchDropdown, hasOpenOverlay = () => false) => { + return (event) => { + if (!isOpen) return; + if (event.key !== "Escape" || event.defaultPrevented || hasOpenOverlay()) return; + event.preventDefault(); + switchDropdown(); + }; + }; + + it("prevents double dismissal when a dialog is open above dropdown", () => { + let dialogClosed = false; + let dropdownClosed = false; + + const dialogKeyDownHandler = (e) => { + if (e.defaultPrevented) return; + if (e.key === "Escape") { + e.preventDefault(); + dialogClosed = true; + } + }; + + // Standardized dropdown handler + const dropdownKeyDownHandler = createStandardizedHandler(true, () => { + dropdownClosed = true; + }, () => true /* overlay is open */); + + const keyDownEvent = createEvent("keydown", "Escape", "Escape", false); + + // Dialog hears keydown first + dialogKeyDownHandler(keyDownEvent); + // Dropdown hears keydown + dropdownKeyDownHandler(keyDownEvent); + + assert.equal(dialogClosed, true, "Dialog closed"); + assert.equal(dropdownClosed, false, "Dropdown remained open when dialog consumed Escape"); + }); + + it("closes dropdown on keydown when no overlay is open", () => { + let dropdownClosed = false; + const dropdownKeyDownHandler = createStandardizedHandler(true, () => { + dropdownClosed = true; + }, () => false /* no overlay */); + + const keyDownEvent = createEvent("keydown", "Escape", "Escape", false); + dropdownKeyDownHandler(keyDownEvent); + + assert.equal(dropdownClosed, true, "Dropdown closed on keydown"); + assert.equal(keyDownEvent.defaultPrevented, true, "Dropdown prevented default"); + }); + + it("works correctly with remapped keys via event.key === 'Escape'", () => { + let dropdownClosed = false; + const dropdownKeyDownHandler = createStandardizedHandler(true, () => { + dropdownClosed = true; + }, () => false); + + const remappedEvent = createEvent("keydown", "Escape", "CapsLock", false); + dropdownKeyDownHandler(remappedEvent); + + assert.equal(dropdownClosed, true, "Standardized handler closed with remapped Escape"); + }); + + it("ignores non-Escape keys", () => { + for (const key of ["Enter", "Tab", "ArrowDown", " ", "a"]) { + let dropdownClosed = false; + const dropdownKeyDownHandler = createStandardizedHandler(true, () => { + dropdownClosed = true; + }, () => false); + + const event = createEvent("keydown", key, key, false); + dropdownKeyDownHandler(event); + + assert.equal(dropdownClosed, false, `Key "${key}" did not close dropdown`); + assert.equal(event.defaultPrevented, false, `Key "${key}" did not prevent default`); + } + }); +}); diff --git a/tests/client/settingsDialogs.test.js b/tests/client/settingsDialogs.test.js index a8b1ff876..a28d420b6 100644 --- a/tests/client/settingsDialogs.test.js +++ b/tests/client/settingsDialogs.test.js @@ -106,6 +106,20 @@ describe("the language list", () => { "a wheel at the list's end scrolls the dialog behind it, which hides the last row's border"); }); + /** + * The other way the border went missing, and the one that needed a real + * device-pixel-ratio to see: scrollTop and scrollHeight clamp on integers + * while fractional scaling lays the rows out on fractions, so at 125% and + * 150% the bottom of the last row sat ~0.3 CSS px below the scrollport's + * crop at maximum scroll - permanently, with the hover repaint deciding + * whether the straddling border was drawn or not. Slack under the last row + * means the clamp error eats padding, never border. + */ + it("keeps the last row's border off the crop line", () => { + assert.match(listRule(), /padding-bottom:\s*0\.25rem/, + "without bottom slack, fractional-DPR scroll clamping clips the last row's border"); + }); + it("yields its height to the viewport before the dialog has to scroll", () => { // The compiler may drop the redundant calc() inside min(), so the // assertion reads the mechanism - a viewport term minus an allowance - diff --git a/tests/integration/status.test.js b/tests/integration/status.test.js index 776eb8660..32db5daa8 100644 --- a/tests/integration/status.test.js +++ b/tests/integration/status.test.js @@ -1,6 +1,9 @@ import { describe, it, before, after, beforeEach } from "node:test"; import assert from "node:assert/strict"; import { bootServer, api, seedTests, setConfig } from "./helpers/boot.js"; +// The same module instance the booted app schedules with, so a delay started +// here is the pending run the route reports on. +import * as timer from "../../server/tasks/timer.js"; let server; @@ -171,6 +174,34 @@ describe("GET /api/speedtests/status", () => { assert.equal((await status()).nextTestApproximate, true); }); + /** + * With the offset enabled the 19:00 job fires and then sleeps for up + * to five minutes before testing. The countdown is cron arithmetic + * from now, so any poll during that sleep answered 19:30 - the bar + * rolled to the next slot while the 19:00 test was still on its way, + * which read as it having been skipped. While a run sleeps, its wake + * moment is the answer - and it is exact, so the tilde drops with it. + */ + it("names the sleeping run's wake moment, not the slot after it", async () => { + await setConfig(server.config, "cron", "0,30 * * * *"); + await setConfig(server.config, "scheduleOffset", "true"); + + const sleeping = timer.delayRun(90_000); + + try { + const body = await status(); + const inMs = new Date(body.nextTest).getTime() - Date.now(); + + assert.ok(inMs > 60_000 && inMs <= 95_000, + `nextTest is ${Math.round(inMs / 1000)}s out - the slot after, not the pending run`); + assert.equal(body.nextTestApproximate, false, + "the wake moment is exact, and the tilde claims it is not"); + } finally { + timer.stopTimer(); + await sleeping; + } + }); + // Written past validateInput deliberately: an unusable schedule cannot // be set through the API, but one can survive in a database restored // from an older export, and the status still has to answer. diff --git a/tests/server/timerLifecycle.test.js b/tests/server/timerLifecycle.test.js index d20fadd52..79eacf688 100644 --- a/tests/server/timerLifecycle.test.js +++ b/tests/server/timerLifecycle.test.js @@ -184,3 +184,45 @@ describe("an offset run that is still waiting", () => { assert.equal(timer.scheduleChangedSince(startedIn), false); }); }); + +/** + * The moment the sleeping run will wake, which is what the status bar needs. + * + * With the offset enabled the 19:00 job fires and then sleeps for up to five + * minutes before it tests anything. The countdown is cron arithmetic from now, + * so any status poll during that sleep answered the slot AFTER the pending one + * - the bar rolled from "~19:00" to "~19:30" while the 19:00 test was still on + * its way, which read as it having been skipped. + */ +describe("the pending offset run", () => { + it("has no wake moment while nothing sleeps", () => { + assert.equal(timer.pendingRunAt(), null); + }); + + it("names the wake moment while a run sleeps its offset", async () => { + const sleeping = timer.delayRun(5000); + const at = timer.pendingRunAt(); + + assert.ok(at, "a sleeping delay reports no wake moment"); + + const inMs = new Date(at).getTime() - Date.now(); + assert.ok(inMs > 3500 && inMs <= 5100, `the wake moment is ${inMs}ms out rather than ~5s`); + + timer.stopTimer(); + await sleeping; + }); + + it("forgets the moment once the delay is released", async () => { + const sleeping = timer.delayRun(5000); + timer.stopTimer(); + await sleeping; + + assert.equal(timer.pendingRunAt(), null, "a released delay still reports a wake moment"); + }); + + it("forgets the moment once the delay elapses", async () => { + await timer.delayRun(10); + + assert.equal(timer.pendingRunAt(), null); + }); +});