From aa58c0cca7f8bf2c9522613c8b5bc596c21c9457 Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Fri, 7 Aug 2026 19:57:38 -0400 Subject: [PATCH] fix(web): Escape did not close the sign-in dialog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit wraps the handle field and calls preventDefault() on every Escape it sees — unconditionally, to clear a suggestion list it does not first check for — and a prevented keydown never becomes the dialog's `cancel`. The field is focused the moment the dialog opens, so Escape did nothing from the one control a player was already in. Tabbing to "Not now" first made it work, which nobody would think to try. The dialog answers the key itself now, in the capture phase, before the typeahead has it. The cost is that Escape with suggestions showing closes the whole dialog rather than dismissing just the list: the component keeps that state in a closed shadow root, so there is no way to ask. --- web/scripts/signin-form.test.mjs | 36 +++++++++++++++++++++++++++++++ web/src/signin-modal.ts | 37 +++++++++++++++++++++++++++++--- 2 files changed, 70 insertions(+), 3 deletions(-) diff --git a/web/scripts/signin-form.test.mjs b/web/scripts/signin-form.test.mjs index a776cae..df71793 100644 --- a/web/scripts/signin-form.test.mjs +++ b/web/scripts/signin-form.test.mjs @@ -86,3 +86,39 @@ test("the dialog is named by the form's heading", () => { "the dialog is unnamed; a modal with no name announces only 'dialog'", ); }); + +test("the dialog answers Escape itself, because the browser's own will not", () => { + // wraps the handle field and preventDefaults every Escape + // it sees, and a prevented keydown never becomes the dialog's `cancel`. The + // field is focused the moment the dialog opens, so relying on the browser + // meant Escape did nothing from the one control a player was already in. + // + // Nothing about that is visible from the source of this repo alone — it + // reads as a plain , and plain closes on Escape — so the + // handler is the sort of thing a tidy-up removes. + // The call spans several lines and its own body contains a `);`, so it is + // read to the closing paren at this call's indent rather than by a lazy + // regex — which stops inside the handler. + const start = modal.indexOf('dialog.addEventListener(\n "keydown"'); + assert.notEqual( + start, + -1, + "the dialog no longer handles Escape, and the typeahead eats it", + ); + const end = modal.indexOf("\n );", start); + assert.notEqual(end, -1, "the keydown listener has no end"); + const listener = modal.slice(start, end); + + assert.match( + listener, + /event\.key !== "Escape"/, + "the keydown handler is not about Escape any more", + ); + // Capture. In the bubble phase the typeahead has already had the key. + assert.match( + listener, + /\n {4}true,\s*$/, + "the Escape handler is not in the capture phase, so it runs after the " + + "typeahead has already prevented the key", + ); +}); diff --git a/web/src/signin-modal.ts b/web/src/signin-modal.ts index 79f203a..32f2caf 100644 --- a/web/src/signin-modal.ts +++ b/web/src/signin-modal.ts @@ -9,8 +9,8 @@ * A modal rather than an inline panel: neither caller has room to grow. The * editor is a grid of blocks sized to each other and shifts everything the * player is looking at if one of them gets taller; the masthead is a 3.5rem - * bar. `` also brings focus trapping, Escape and the backdrop for - * free. + * bar. `` brings focus trapping and the backdrop for free. Escape is + * the one it does not — see below. * * Loaded on demand — the form pulls in a handle typeahead that is a chunk of * its own, and nobody who does not click Login should pay for either. @@ -58,7 +58,38 @@ export function openSignIn({ onBeforeConnect }: SignInOptions = {}): void { dialog.remove(); }; cancel.addEventListener("click", close); - // Escape closes a dialog on its own, but leaves the element in the document. + + /** + * Escape, which a dialog would answer on its own if nothing inside it wanted + * the key. + * + * `` wraps the handle field and calls preventDefault() on + * every Escape it sees — unconditionally, to clear a suggestion list it does + * not first check for — and a prevented keydown never becomes the dialog's + * `cancel`. The field is focused the moment this opens, so what a player + * actually got was a modal that ignored Escape from the one control they + * were already in. Tabbing to "Not now" first made it work, which is not + * something anyone would think to try. + * + * Capture, so this runs before the typeahead rather than after it has eaten + * the key. The cost is that Escape with suggestions showing now closes the + * whole dialog instead of dismissing just the list: the component keeps that + * state in a closed shadow root, so there is no way to ask. A modal that + * cannot be dismissed with Escape is the worse of the two, and the list goes + * with it either way. + */ + dialog.addEventListener( + "keydown", + (event) => { + if (event.key !== "Escape") return; + event.preventDefault(); + close(); + }, + true, + ); + + // The backdrop and "Not now" both go through close(); this catches any other + // way the dialog ends, and stops a closed one being left in the document. dialog.addEventListener("close", () => dialog.remove()); dialog.addEventListener("click", (event) => { if (event.target === dialog) close(); -- 2.51.2