Fix/status countdown and language list

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)

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"
     });
 });
 

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);

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`);
+        }
+    });
+});

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 -

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.