fix(sw): preserve open modals when tab-hide auto-reload would fire (#3184)

* fix(sw): preserve open modals when tab-hide auto-reload would fire

Scenario: a Pro user opens the Clerk sign-in modal, enters their email,
and switches to their mail app to fetch the code. If a deploy happens
while they wait and the SW update toast's 5 s dwell window has elapsed,
`visibilitychange: hidden` triggers `window.location.reload()` — which
wipes the Clerk flow, so the code in the inbox is for a now-dead attempt
and the user has to re-request. Same failure applies to UnifiedSettings,
the ⌘K search modal, story/signal popups, and anything else with modal
semantics: leaving the tab = lose your place.

Fix: in `sw-update.ts`, the hidden-tab auto-reload now checks for any
open modal/dialog via a compound selector (`[aria-modal="true"],
[role="dialog"], .modal, .cl-modalBackdrop, dialog[open]`) and
suppresses the reload when one matches. Covers Clerk's `.cl-modalBackdrop`,
the site-wide `.modal` convention (UnifiedSettings, WidgetChatModal),
and any well-authored dialog. The reload stays armed — next tab-hide
after the modal closes fires it. Manual "Reload" button click is
unaffected (explicit user intent).

Over-matching is safe (worst case: user clicks Reload manually).
Under-matching keeps the bug, so the selector errs generous.

Tests: three new cases cover modal-open suppression, re-arming after
modal close, and manual-click bypass. 25/25 sw-update tests pass.

Follow-up ticket worth filing: add `aria-modal="true"` + `role="dialog"`
to the modals that are missing them (SearchModal, StoryModal, SignalModal,
WidgetChatModal, McpConnectModal, MobileWarningModal, CountryIntelModal,
UnifiedSettings). That's the proper long-term a11y fix and would let us
narrow the selector once coverage is complete.

* fix(sw): filter modal guard by actual visibility, not just DOM presence

Addresses review feedback on #3184:

The previous selector (`[role="dialog"]` etc.) matched the UnifiedSettings
overlay, which is created in its constructor at app startup
(App.ts:977 → UnifiedSettings.ts:68-71 sets role="dialog") and stays in
the DOM for the whole session. That meant auto-reload was effectively
disabled for every user, not just those with an actually-open modal.

Fix: don't just check for selector matches — check whether the matched
element is actually rendered. Persistent modal overlays hide themselves
via `display: none` (main.css:6744: `.modal-overlay { display: none }`)
and reveal via an `.active` class (main.css:6750: `.active { display: flex }`),
so `offsetParent === null` cleanly distinguishes closed from open. We
prefer `checkVisibility()` where available (Chrome 105+, Safari 17.4+,
Firefox 125+, which covers virtually all current WM users) and fall back
to `offsetParent` otherwise.

This also handles future modals automatically, without needing us to
enumerate every `.xxx-modal-overlay.active` class the site might
introduce.

New tests:
- Modal mounted AND visible → reload suppressed (original Clerk case)
- Modal mounted but hidden → reload fires (reviewer's regression case)
- Modal visible, then hidden on return → reload fires on next tab-hide
- Manual Reload click unaffected in all cases

26/26 sw-update tests pass.

* fix(sw): replace offsetParent fallback with getClientRects for fixed overlays

Addresses second review finding on #3184:

The previous fallback `el.offsetParent !== null` silently failed on every
`position: fixed` overlay — which is every modal in this app:

- `.modal-overlay` (main.css:6737) — UnifiedSettings, WidgetChatModal
- `.story-modal-overlay` (main.css:3442)
- `.country-intel-modal-overlay` active state (main.css:18415)

MDN: `offsetParent` is specified to return null for any `position: fixed`
element, regardless of visibility. So on Firefox <125 or Safari <17.4
(where `Element.checkVisibility()` is unavailable), `isModalOpen` would
return false for actually-open modals → auto-reload fires → Clerk sign-in
and every other fixed-position flow gets wiped exactly as PR #3184 was
meant to prevent.

Fix: fall back to `getClientRects().length > 0`. This returns 0 for
`display: none` elements (how `.modal-overlay` hides when `.active` is
absent) and non-zero for rendered elements, including position:fixed.
It's universally supported and matches the semantics we want.

New tests exercise the fallback path explicitly with a `supportsCheckVisibility`
toggle on the fake env:

- visible position:fixed modal + no checkVisibility → reload suppressed
- hidden mounted modal + no checkVisibility → reload fires

28/28 sw-update tests pass.

* fix(a11y): add role=dialog + aria-modal=true to five missing modals

Addresses third review finding on #3184.

SW auto-reload guard uses a `[role="dialog"]` selector but five modals
were missing the attribute, so `isModalOpen()` returned false and the
page could still auto-reload mid-flow on those screens. Broadening the
selector to enumerate specific class names was rejected because the app
has many non-modal `-overlay` classes (`#deckgl-overlay`,
`.conflict-label-overlay`, `.layer-warn-overlay`, `.mobile-menu-overlay`)
that would cause false positives and permanently disable auto-reload.

Instead, standardize on the existing convention used by UnifiedSettings:
every modal overlay sets `role="dialog"` + `aria-modal="true"` at
creation. This makes the SW selector work AND improves screen-reader
behavior (focus trap, background element suppression).

Modals updated:
- SearchModal (⌘K search) — both mobile sheet and desktop variants use
  the same element, single set-attributes call at create time
- StoryModal (news story detail)
- SignalModal (instability spike detail)
- CountryIntelModal (country deep-dive overlay)
- MobileWarningModal (mobile device warning)

No change to sw-update.ts — the existing selector already covers the
newly-attributed elements. All 28 sw-update tests still pass; typecheck
clean.
This commit is contained in:
Elie Habib
2026-04-18 22:54:58 +04:00
committed by GitHub
parent 55ac431c3f
commit d1e084061d
7 changed files with 222 additions and 1 deletions

View File

@@ -1,6 +1,12 @@
interface VisibleElementLike {
checkVisibility?: () => boolean;
getClientRects?: () => { length: number };
}
interface DocumentLike {
readonly visibilityState: string;
querySelector: (sel: string) => Element | null;
querySelectorAll: (sel: string) => Iterable<Element & VisibleElementLike>;
createElement: (tag: string) => HTMLElement;
body: { appendChild: (el: Element) => void; contains: (el: Element | null) => boolean };
addEventListener: (type: string, cb: () => void) => void;
@@ -37,6 +43,44 @@ export interface SwUpdateHandlerOptions {
export const SW_DEBUG_LOG_KEY = 'wm-sw-debug-log';
const SW_DEBUG_LOG_MAX = 30;
// Selectors that identify a modal/dialog candidate. Many site modals mount
// at app startup and stay in the DOM (e.g. UnifiedSettings sets
// role="dialog" in its constructor), so a raw selector match alone would
// permanently disable auto-reload. We only treat a match as "open" when
// the element is actually rendered — see isModalOpen() below.
export const OPEN_MODAL_SELECTOR =
'[aria-modal="true"], [role="dialog"], .cl-modalBackdrop, .modal-overlay, dialog[open]';
/**
* Any candidate that's actually visible → a real open modal.
*
* Preferred: `element.checkVisibility()` (Chrome 105+, Safari 17.4+, FF 125+).
*
* Fallback for older engines: `getClientRects().length > 0`. This returns 0
* when the element has `display: none` (exactly how persistent overlays
* hide — see main.css `.modal-overlay { display: none }` /
* `.active { display: flex }`) and non-zero for rendered elements,
* including `position: fixed` overlays. We cannot use `offsetParent` here:
* MDN specifies it returns `null` for every `position: fixed` element
* regardless of visibility, so it would false-negative on the Story overlay
* (main.css:3442), the active Country Intel overlay (main.css:18415), and
* `.modal-overlay` itself — all of which are fixed-positioned.
*/
function isModalOpen(doc: DocumentLike): boolean {
for (const el of doc.querySelectorAll(OPEN_MODAL_SELECTOR)) {
const checkVisibility = el.checkVisibility;
if (typeof checkVisibility === 'function') {
if (checkVisibility.call(el)) return true;
continue;
}
const getClientRects = el.getClientRects;
if (typeof getClientRects === 'function' && getClientRects.call(el).length > 0) {
return true;
}
}
return false;
}
function appendDebugLog(entry: Record<string, unknown>): void {
try {
const raw = sessionStorage.getItem(SW_DEBUG_LOG_KEY);
@@ -163,6 +207,14 @@ export function installSwUpdateHandler(options: SwUpdateHandlerOptions = {}): vo
}
logSw('visibility-hidden', { autoReloadAllowed, dismissed });
if (!dismissed && autoReloadAllowed && doc.body.contains(toast)) {
// Don't interrupt an in-flight modal flow (Clerk email-code wait,
// Settings, ⌘K search, etc.). The reload stays armed — next tab-hide
// after the modal closes will fire it. User can also click Reload
// in the toast manually at any time.
if (isModalOpen(doc)) {
logSw('auto-reload-suppressed-modal-open');
return;
}
logSw('auto-reload-triggered');
reload();
}

View File

@@ -51,6 +51,8 @@ export class CountryIntelModal {
constructor() {
this.overlay = document.createElement('div');
this.overlay.className = 'country-intel-overlay';
this.overlay.setAttribute('role', 'dialog');
this.overlay.setAttribute('aria-modal', 'true');
this.overlay.innerHTML = `
<div class="country-intel-modal">
<div class="country-intel-header">

View File

@@ -10,6 +10,8 @@ export class MobileWarningModal {
constructor() {
this.element = document.createElement('div');
this.element.className = 'mobile-warning-overlay';
this.element.setAttribute('role', 'dialog');
this.element.setAttribute('aria-modal', 'true');
this.element.innerHTML = `
<div class="mobile-warning-modal">
<div class="mobile-warning-header">

View File

@@ -200,6 +200,8 @@ export class SearchModal {
private createModal(): void {
this.overlay = document.createElement('div');
this.overlay.setAttribute('role', 'dialog');
this.overlay.setAttribute('aria-modal', 'true');
if (this.isMobile) {
this.overlay.className = 'search-overlay search-mobile';

View File

@@ -17,6 +17,8 @@ export class SignalModal {
constructor() {
this.element = document.createElement('div');
this.element.className = 'signal-modal-overlay';
this.element.setAttribute('role', 'dialog');
this.element.setAttribute('aria-modal', 'true');
this.element.innerHTML = `
<div class="signal-modal">
<div class="signal-modal-header">

View File

@@ -18,6 +18,8 @@ export function openStoryModal(data: StoryData): void {
modalEl = document.createElement('div');
modalEl.className = 'story-modal-overlay';
modalEl.setAttribute('role', 'dialog');
modalEl.setAttribute('aria-modal', 'true');
modalEl.innerHTML = `
<div class="story-modal">
<button class="story-close-x" aria-label="${t('modals.story.close')}">

View File

@@ -1,6 +1,6 @@
import { describe, it, beforeEach } from 'node:test';
import assert from 'node:assert/strict';
import { installSwUpdateHandler } from '../src/bootstrap/sw-update.ts';
import { installSwUpdateHandler, OPEN_MODAL_SELECTOR } from '../src/bootstrap/sw-update.ts';
// ---------------------------------------------------------------------------
// Fake environment
@@ -17,14 +17,35 @@ interface FakeElement {
remove(): void;
addEventListener(type: string, cb: (e: unknown) => void): void;
closest(sel: string): { dataset: Record<string, string> } | null;
checkVisibility?: () => boolean;
getClientRects?: () => { length: number };
}
interface FakeEnv {
doc: {
visibilityState: string;
setVisibilityState(v: string): void;
/**
* Test helpers modeling modal state. Overlays in this app are always
* `position: fixed`, so `offsetParent` would always be null — the
* visibility check must use `checkVisibility()` or `getClientRects()`.
*
* - modalMounted: element exists in DOM (matches OPEN_MODAL_SELECTOR
* on query). Maps to UnifiedSettings, SignalModal, etc. — mounted in
* their constructor at app startup and left in the DOM for the whole
* session.
* - modalVisible: the mounted element is actually rendered
* (checkVisibility() returns true / getClientRects().length > 0).
* - supportsCheckVisibility: test knob. When false, the fake element
* omits `checkVisibility` so the code path exercises the
* `getClientRects` fallback (simulates Firefox <125 / Safari <17.4).
*/
modalMounted: boolean;
modalVisible: boolean;
supportsCheckVisibility: boolean;
_removedListeners: Array<() => void>;
querySelector(sel: string): FakeElement | null;
querySelectorAll(sel: string): Iterable<FakeElement>;
createElement(tag: string): FakeElement;
body: {
appendChild(el: FakeElement): void;
@@ -56,6 +77,9 @@ function makeEnv(): FakeEnv {
const doc: FakeEnv['doc'] = {
get visibilityState() { return _visibilityState; },
setVisibilityState(v: string) { _visibilityState = v; },
modalMounted: false,
modalVisible: false,
supportsCheckVisibility: true,
_removedListeners: [],
querySelector(sel: string): FakeElement | null {
@@ -63,6 +87,35 @@ function makeEnv(): FakeEnv {
return null;
},
querySelectorAll(sel: string): Iterable<FakeElement> {
if (sel !== OPEN_MODAL_SELECTOR) return [];
if (!this.modalMounted && !this.modalVisible) return [];
const isVisible = this.modalVisible;
const el: FakeElement = {
tagName: 'DIV',
className: '',
innerHTML: '',
dataset: {},
_listeners: {},
_removed: false,
classList: {
_classes: new Set<string>(),
add() {}, remove() {}, has() { return false; },
},
remove() {},
addEventListener() {},
closest() { return null; },
// getClientRects is always available in real DOM; mirrors `display: none`
// semantics (empty list when hidden, non-empty when rendered — including
// `position: fixed` elements, unlike offsetParent).
getClientRects: () => ({ length: isVisible ? 1 : 0 }),
};
if (this.supportsCheckVisibility) {
el.checkVisibility = () => isVisible;
}
return [el];
},
createElement(_tag: string): FakeElement {
const el: FakeElement = {
tagName: _tag.toUpperCase(),
@@ -469,6 +522,112 @@ describe('installSwUpdateHandler', () => {
assert.equal(env.reloadCalls.length, 1, 'reload fires when user switches away after seeing toast');
});
// --- modal-open guard (preserves Clerk sign-in, Settings, etc.) ------------
it('does NOT auto-reload when a modal is visibly open while the tab goes hidden', () => {
env.swContainer._controller = {};
install(env);
env.swContainer.fireControllerChange();
fireDwellTimer(env); // autoReloadAllowed = true
// Simulate e.g. Clerk sign-in modal: mounted AND visible
env.doc.modalMounted = true;
env.doc.modalVisible = true;
env.doc.setVisibilityState('hidden');
fireVisibility(env);
assert.equal(env.reloadCalls.length, 0, 'reload suppressed while modal is visibly open');
});
it('DOES auto-reload when a modal is mounted-but-hidden (persistent dialog case)', () => {
// Regression for the reviewer-flagged bug: UnifiedSettings mounts in its
// constructor with role="dialog" and stays in the DOM forever, but hides
// via `display: none` when .active is not set. A naive selector match
// would permanently disable auto-reload. The visibility filter fixes this.
env.swContainer._controller = {};
install(env);
env.swContainer.fireControllerChange();
fireDwellTimer(env);
env.doc.modalMounted = true; // dialog element exists in DOM
env.doc.modalVisible = false; // but it's hidden (display:none, no .active)
env.doc.setVisibilityState('hidden');
fireVisibility(env);
assert.equal(env.reloadCalls.length, 1, 'reload fires when mounted dialog is not actually visible');
});
it('auto-reloads on the NEXT tab-hide after the modal is closed (hidden)', () => {
env.swContainer._controller = {};
install(env);
env.swContainer.fireControllerChange();
fireDwellTimer(env);
// First hide with modal visibly open — suppressed
env.doc.modalMounted = true;
env.doc.modalVisible = true;
env.doc.setVisibilityState('hidden');
fireVisibility(env);
assert.equal(env.reloadCalls.length, 0);
// User returns, closes modal (mounted stays true, visible goes false),
// then switches tabs again
env.doc.setVisibilityState('visible');
fireVisibility(env);
env.doc.modalVisible = false;
env.doc.setVisibilityState('hidden');
fireVisibility(env);
assert.equal(env.reloadCalls.length, 1, 'reload fires on next hide after modal hidden');
});
it('manual Reload button click still works while a modal is open', () => {
env.swContainer._controller = {};
install(env);
env.swContainer.fireControllerChange();
env.doc.modalMounted = true;
env.doc.modalVisible = true;
clickToastButton(env, 'reload');
assert.equal(env.reloadCalls.length, 1, 'explicit click bypasses modal guard');
});
// --- fallback path (engines without Element.checkVisibility) ---------------
it('uses getClientRects() fallback to detect visible position:fixed modals', () => {
// Regression for the reviewer-flagged fallback bug: offsetParent would
// return null for every `position: fixed` overlay even when visible, so
// on Firefox <125 / Safari <17.4 the modal guard would false-negative
// and auto-reload would wipe the Clerk sign-in. getClientRects() does
// not have this flaw.
env.swContainer._controller = {};
install(env);
env.swContainer.fireControllerChange();
fireDwellTimer(env);
env.doc.supportsCheckVisibility = false; // simulate older engine
env.doc.modalMounted = true;
env.doc.modalVisible = true; // fixed-position overlay, visible
env.doc.setVisibilityState('hidden');
fireVisibility(env);
assert.equal(env.reloadCalls.length, 0, 'reload suppressed via getClientRects fallback');
});
it('fallback path still allows reload when a mounted modal is hidden', () => {
env.swContainer._controller = {};
install(env);
env.swContainer.fireControllerChange();
fireDwellTimer(env);
env.doc.supportsCheckVisibility = false;
env.doc.modalMounted = true;
env.doc.modalVisible = false; // display:none → getClientRects().length === 0
env.doc.setVisibilityState('hidden');
fireVisibility(env);
assert.equal(env.reloadCalls.length, 1, 'reload fires when fallback reports not-rendered');
});
// --- listener leak regression -----------------------------------------------
it('removes the previous visibilitychange handler when a newer deploy replaces the toast', () => {