refactor(web): list pages share the async primitives; dialogs hide the page behind
Keys, secrets, audit, steps and workflows each carried the same loading/error/empty ternary with its own copy of the spinner and its own wording for a failed fetch — five of them had drifted to five different sentences for "the request did not come back". They go through AsyncBoundary now, which means a skeleton in place of a spinner, a retry button on failure, and backend messages passed through friendlyMessage rather than printed raw. Steps gets the filtered-empty state the fleet just got: "no steps match that filter" is not "no steps yet", and only one of them should offer to create the first one. Two more from review: Modal's effect ran before `mounted` flipped, so a dialog rendered already open found null refs and took no focus at all. It depends on `mounted` now. aria-modal was a claim with no mechanism behind it — portalled to the body, the app tree is a sibling of the dialog and a screen reader's virtual cursor still browsed the page underneath. The body's other children are marked aria-hidden while any dialog is open, refcounted alongside the scroll lock. This does mean a toast raised while a dialog is open is not announced, which is the correct trade for a modal: ConfirmDialog shows its own errors inline.
This commit is contained in:
@@ -20,6 +20,9 @@ const stack: symbol[] = [];
|
||||
let lockCount = 0;
|
||||
let lockedOverflow = "";
|
||||
let lockedPadding = "";
|
||||
let hidden: HTMLElement[] = [];
|
||||
|
||||
const PORTAL_ATTR = "data-vantage-dialog";
|
||||
|
||||
function lockScroll() {
|
||||
const { body } = document;
|
||||
@@ -31,6 +34,18 @@ function lockScroll() {
|
||||
const gap = window.innerWidth - document.documentElement.clientWidth;
|
||||
body.style.overflow = "hidden";
|
||||
if (gap > 0) body.style.paddingRight = `${gap}px`;
|
||||
|
||||
/*
|
||||
* aria-modal is a claim, not a mechanism. Portalled to the body, the
|
||||
* app tree is a plain sibling of the dialog, so a screen reader's
|
||||
* virtual cursor happily browses the page underneath — which is the
|
||||
* exact thing the overlay exists to prevent. Hiding the siblings is
|
||||
* what makes the claim true.
|
||||
*/
|
||||
hidden = Array.from(body.children).filter(
|
||||
(el): el is HTMLElement => el instanceof HTMLElement && !el.hasAttribute(PORTAL_ATTR),
|
||||
);
|
||||
for (const el of hidden) el.setAttribute("aria-hidden", "true");
|
||||
}
|
||||
lockCount++;
|
||||
}
|
||||
@@ -40,6 +55,8 @@ function unlockScroll() {
|
||||
if (lockCount === 0) {
|
||||
document.body.style.overflow = lockedOverflow;
|
||||
document.body.style.paddingRight = lockedPadding;
|
||||
for (const el of hidden) el.removeAttribute("aria-hidden");
|
||||
hidden = [];
|
||||
}
|
||||
}
|
||||
|
||||
@@ -93,7 +110,11 @@ export function Modal({
|
||||
useEffect(() => setMounted(true), []);
|
||||
|
||||
useEffect(() => {
|
||||
if (!open) return;
|
||||
// `mounted` is a dependency, not just a guard: on the first client
|
||||
// render it is false and the component returns null, so a Modal that
|
||||
// mounts already open would run this against null refs and never take
|
||||
// focus at all.
|
||||
if (!open || !mounted) return;
|
||||
|
||||
const id = idRef.current;
|
||||
stack.push(id);
|
||||
@@ -162,7 +183,7 @@ export function Modal({
|
||||
if (restoreRef.current?.isConnected) restoreRef.current.focus();
|
||||
restoreRef.current = null;
|
||||
};
|
||||
}, [open]);
|
||||
}, [open, mounted]);
|
||||
|
||||
if (!open || !mounted) return null;
|
||||
|
||||
@@ -172,7 +193,7 @@ export function Modal({
|
||||
* not part of the content it covers.
|
||||
*/
|
||||
return createPortal(
|
||||
<div className="fixed inset-0 z-50 flex items-end justify-center p-0 sm:items-center sm:p-4">
|
||||
<div {...{ [PORTAL_ATTR]: "" }} className="fixed inset-0 z-50 flex items-end justify-center p-0 sm:items-center sm:p-4">
|
||||
<div className="absolute inset-0 bg-black/60" onClick={onClose} aria-hidden="true" />
|
||||
<div
|
||||
ref={panelRef}
|
||||
|
||||
Reference in New Issue
Block a user