Skip to content

fix: expose one dialog role and one live root per toast in ui - #1412

Merged
vivek7405 merged 11 commits into
mainfrom
fix/screen-reader-a11y-pass
Aug 15, 2026
Merged

fix: expose one dialog role and one live root per toast in ui#1412
vivek7405 merged 11 commits into
mainfrom
fix/screen-reader-a11y-pass

Conversation

@vivek7405

@vivek7405 vivek7405 commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

mise ~/.config/mise/config.toml tools: gh@2.97.0
mise ~/.config/mise/config.toml tools: gh@2.97.0
mise ~/.config/mise/config.toml tools: gh@2.97.0
mise ~/.config/mise/config.toml tools: gh@2.97.0
mise ~/.config/mise/config.toml tools: gh@2.97.0
mise ~/.config/mise/config.toml tools: gh@2.97.0
mise ~/.config/mise/config.toml tools: gh@2.97.0
mise ~/.config/mise/config.toml tools: gh@2.97.0
Closes #1245

Settles the two findings the #1080 accessibility audit marked needs-runtime-check, and that PR #1230 deliberately left alone. Both are now measured against Chrome's computed accessibility tree rather than guessed at, and both turned out to be real.

Why the tree, and not another DOM assertion

packages/ui/test/components/browser/ui-a11y.test.js already asserts the attributes these components write. That is the input. A screen reader consumes the platform accessibility tree, which is the output, and the gap between the two is exactly where both findings live. So this adds packages/ui/test/e2e/a11y-tree.e2e.mjs, which drives the live gallery in real Chromium and reads Accessibility.getFullAXTree over CDP.

The instrument landed BEFORE either fix, so what ships is decided by what it observed, not by what the audit feared.

What changed

dialog and alert-dialog exposed TWO nested dialog-family nodes, because the native <dialog> has an implicit role=dialog and the inner content div carried the ARIA. The role, the name target and the tab stop move onto the native element, so exactly one node is exposed. aria-modal goes with them: a showModal()-opened native dialog is already exposed as modal by the platform, which the tree confirms both before and after.

sonner resolved an ordinary toast under two nested live roots, its own role="status" and the polite viewport. The inner role bought nothing, since the viewport is already polite, so a non-error toast now carries no role. An error toast keeps role="alert", which is load-bearing: it is the only way to make one item assertive inside a polite viewport, and its second live root is the accepted cost. The attribute is branched rather than written as a nullish hole, which would serve role="" from the server renderer.

Surface Before After
dialog 2 nodes, [dialog, dialog] 1 node, dialog
alert-dialog 2 nodes, [alertdialog, dialog] 1 node, alertdialog
dialog, title-less 2 nodes, name Dialog 1 node, name Dialog
alert-dialog, title-less 2 nodes, name Alert dialog 1 node, name Alert dialog
default toast 2 live roots, both polite 1 live root, polite
error toast 2 roots, [assertive, polite] unchanged, by design

modal reads true throughout, so dropping aria-modal did not cost modality.

The role move deliberately does NOT bring tabindex="-1" with it. The HTML Standard says the tabindex attribute must not be specified on dialog elements, and it was doing no work anyway: showModal()'s focusing steps already fall back to the dialog itself when there is no focus delegate, and a tabindex="-1" descendant was never eligible to be that delegate, since a delegate has to be tabbable.

While wiring the name onto the native <dialog>, the naming pass also gained a re-resolve fix. It previously skipped when the panel already carried the attribute, but the panel keeps its attributes between opens, so it could never re-resolve: removing a title node and re-opening left a stale aria-labelledby pointing at a dead IDREF, which resolves to no name AND suppresses the floor that exists to make an unnamed modal impossible. Both failures are silent. Each pass now clears what the previous one wrote first. packages/ui/test/components/browser/ui-a11y.test.js covers the re-open, and it was seen red against the old guard.

A correction to the audit's reasoning

#1080 worried that the outer implicit role=dialog would be the one assistive tech reports, so the alertdialog role would never reach the user. Chrome does not do that: it exposes both, nearest-first, with alertdialog nearest. The urgency was not being lost. The double nesting is still a real defect and still worth removing, but it is a redundant-wrapper problem rather than a role-masking one, and the before state should not be described as an alert dialog that failed to announce itself.

Test plan

  • Unit packages/ui 219/219. Added two SSR cases to test/ssr-aria.test.js asserting the role lands on the native <dialog> and no aria-modal is served. Updated the aria-modal assertion in test/registry-contents.test.js, whose intent (the dialog is exposed as modal) is now asserted where it can actually be observed.
  • Browser 881 Chromium / 871 Firefox / 881 WebKit, 0 failed. Added an assertion that an ordinary toast carries no role and not an empty one. Moved the dialog panel lookups onto the native <dialog>. Re-pointed one ui-overlay.test.js locator that used [role="dialog"] to find the element carrying data-state; those were only ever the same element by coincidence.
  • e2e test/e2e/a11y-tree.e2e.mjs 16/16, test/e2e/touch.e2e.mjs 5/5, and the blog e2e 95/95 including the new /ui-demo dialog test.
  • Counterfactual, every layer (including both halves of the examples/blog change, see above). Fix committed, then source reverted to the pre-fix commit: the 2 SSR assertions fail, 15 browser assertions fail, and the tree assertions were seen red on the baseline run before any fix existed. Restored and re-run green.
  • Dogfood website boots 200 on /, /ui, /ui/button, /ui/dialog, /ui/alert-dialog, /ui/sonner in prod mode with no broken modulepreloads. webjs check clean on gallery, examples/blog and website.
  • Full node suite 4381/4387. The 5 failures are the documented linked-worktree artifacts (2 listener + 3 elision); test/bun/listener.test.mjs passes 1/1 in the primary checkout at main, and this diff touches nothing under test/bun/, elision, core or server.
  • Bun parity N/A. This is registry component markup plus a Chromium-only CDP probe. It touches no runtime-sensitive surface (serializer, listener, SSR/action/CSRF dispatch, streams, node:crypto, TS stripper, auth/session/cors), and the renderToString path these components ride is already covered cross-runtime by the core suite.
  • Manual screen-reader pass. Not run: neither Orca nor Firefox is installed on the dev machine, and installing them needs sudo. See below.

Docs

  • packages/ui/AGENTS.md: the dialog and alert-dialog inventory rows, the sonner row, and two sentences in the Accessibility section.
  • blog/accessible-web-components-by-default.md: a published post described the wiring this change removes, on two counts (the panel's role="dialog" plus aria-modal="true", and role="status" on an ordinary toast). The repo was shipping a test forbidding role="status" on a toast alongside a live page documenting it. The line about the Tier-1 alert helper's role="alert" vs role="status" is untouched and still correct. The corrected toast sentence claims only what this branch measures: live-root count and politeness, never what a reader utters, since the manual pass has not run.
  • A11y JSDoc blocks in dialog.ts, alert-dialog.ts and sonner.ts, each of which stated the old wiring as fact.
  • The wireDialogLabels() comment and the sonner render comment, both of which explained the old nesting rationale.
  • The website /ui/<name> pages render from the registry, so they follow automatically; confirmed by booting.

The vendored copy in examples/blog

examples/blog vendors its own components/ui/dialog.ts and app/ui-demo/page.ts imports it, so the deployed demo exposed the same two-nested-dialog-nodes shape the kit no longer has. This is now fixed here rather than left for later.

It took two attempts and the first one was wrong, which is worth recording. I initially moved the role there alone. That shipped with nothing asserting it, and, more importantly, the copy has NO accessible-name wiring at all, because it predates that work. Declaring a role in isolation produced a modal that announces itself as a dialog with nothing to read. Measured directly: the resolved name was the empty string.

So both halves land together. The role moves onto the native <dialog>, and the naming wiring comes with it, including the APG floor that makes an unnamed modal impossible. The title lookup also covers this app's <ui-dialog-title> / <ui-dialog-description> markup, which the vendored copy does not register and which therefore leaves no heading behind for the usual h1, h2, h3 fallback to find.

Covered at both layers it is observable from, since the two contracts land at different times. The role is emitted by render(), so test/examples/blog/smoke/blog-smoke.test.js asserts it on the first paint together with the absence of a second role and of aria-modal. Naming is wired at showModal(), so test/e2e/e2e.test.mjs opens the dialog and asserts the resolved name is non-empty. Both were seen red: reverting the role move reds the smoke assertion, removing the naming wiring reds the e2e.

What is NOT done

The manual screen-reader pass. The tree assertions pin the contract and the whole regression guard for finding 1, but whether a reader SPEAKS a toast twice is the announcement queue, one layer above the tree, and no browser artifact can answer it. That question is genuinely open. It needs sudo pacman -S orca firefox on the dev machine, then the listening script in #1245 step 9.

The two findings #1080 marked needs-runtime-check could not be settled by a
DOM assertion, which is what ui-a11y.test.js already does: an attribute
assertion re-checks the input and never the computed output. What a screen
reader consumes is the platform accessibility tree, so assert against that
directly, read over CDP from real Chromium.

This is the measurement instrument, added before any fix so the fixes are
decided by what it observes rather than by what the findings feared.
@vivek7405 vivek7405 self-assigned this Aug 14, 2026
Both findings the #1080 audit marked needs-runtime-check are real, measured
against Chrome's computed accessibility tree rather than guessed at.

dialog and alert-dialog each exposed TWO nested dialog-family nodes, because
the native <dialog> has an implicit role=dialog and the inner content div
carried the ARIA. The role, the name target and the tab stop move onto the
native element, so exactly one node is exposed. aria-modal goes with them:
a showModal()-opened native dialog is already exposed as modal by the
platform, which the tree confirms both before and after.

An ordinary toast resolved under two nested live roots, its own role=status
and the polite viewport. The inner role bought nothing, since the viewport is
already polite, so a non-error toast now carries no role. An error toast keeps
role=alert, which is load-bearing: it is the only way to make one item
assertive inside a polite viewport, and its second live root is the accepted
cost. The attribute is branched rather than written as a nullish hole, which
would serve role="" from the server renderer.

One correction to the audit's reasoning: it feared the outer implicit
role=dialog would mask the alertdialog role. Chrome exposes both, nearest
first, with alertdialog nearest, so the urgency was not being lost. The
redundant wrapper was still worth removing.
@vivek7405

Copy link
Copy Markdown
Collaborator Author

Design rationale: what the tree actually showed, and where the audit's reasoning was wrong

#1080 fenced these two off with "verify against a real screen reader BEFORE changing anything, explicitly do not fix blind". I want to record why the automated proof here is the accessibility tree rather than a reader, and what changed once I could see real numbers.

The two findings sit at different layers. Finding 1 asks which node the platform exposes, with what role and name. That IS the accessibility tree, the artifact Chrome hands to the platform accessibility APIs. A reader answers it once; the tree answers it on every CI run. Finding 2 asks whether a reader SPEAKS a toast twice, which is the announcement queue, one layer above the tree. The tree can prove which live root a toast resolves under and with what politeness, but the utterance count is not a browser artifact and never will be. So the tree carries the contract for both findings and the whole regression guard for finding 1, and exactly one question goes to a real reader.

What I did not expect: the audit's stated worry for alert-dialog does not reproduce. It reasoned that the outer implicit role=dialog "may be the one assistive tech actually reports", so the alertdialog role and its more urgent announcement never reach the user, and that the user would be told they can dismiss with Escape when alert-dialog blocks it. The baseline tree reads [alertdialog, dialog], nearest first, with alertdialog nearest. Chrome exposes both and the nearest one is the right one, so the urgency was not being lost. That reframes the fix: it is removing a redundant wrapper, not rescuing a masked role. Worth being precise about, because "we fixed the alert dialog not announcing itself as an alert" would be a false claim about the before state.

The sonner half went the other way and confirmed the finding exactly. A default toast resolved under two live roots, both polite, which is the nesting some readers double-announce, and the inner role="status" bought nothing because the viewport is already polite. So it goes. role="alert" stays on an error toast, because it is the only way to make one item assertive inside a polite viewport, and its second live root is a real cost accepted for the urgency rather than an oversight.

One thing I checked rather than waved through, since dropping role="status" also drops its implicit aria-atomic="true": a role-less toast inherits the viewport's aria-atomic="false", which would matter if a toast's text ever changed in place, because a non-atomic region announces only the changed part. It does not. toast.promise dismisses and re-adds rather than mutating, and _remove / addToast only ever add and remove whole items, so every announcement is the addition of a whole subtree, which the viewport's aria-relevant="additions text" already covers. The atomicity loss is inert here.

…ismissal

Three review findings.

The two comments explaining the toast icons and the A11y block both still said
the type is conveyed by role=alert vs role=status. No toast carries
role=status any more, and they sit in the file this change is about.

The branch that replaced the nullish role hole was justified by an SSR
divergence nothing actually asserted. The browser test runs the client
renderer, which removes a nullish attribute, so hasAttribute('role') === false
passes identically for the branch and for the hole it forbids, and the SSR
layer cannot reach a toast at all because a viewport renders zero of them
server-side. That leaves the template's shape as the only observable, so
assert it there. Verified by injecting the hole and watching it red.

Both toast probes raced the 4000ms auto-dismiss: 700ms of waiting plus a full
DOM.getDocument and getFullAXTree against a large page, after which a vanished
toast resolves to a null chain, which is a hard failure by design, in a step
that is now unconditional in the required e2e job. duration: 0 removes the
race and costs nothing, since the probe never asserts dismissal.

@vivek7405 vivek7405 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read the whole diff. The shape is right: the measurement instrument lands as its own commit before either fix, so the decision to change anything is made from what it observed rather than from what #1080 feared. Correcting the audit's alert-dialog reasoning in the body, rather than quietly shipping the fix under the original claim, is the part I most want kept.

Four things. Three are real and fixed; the fourth was half stale.

The one worth dwelling on is the sonner role guard. Branching the attribute instead of writing a nullish hole is correct, but the justification was an SSR divergence that nothing asserted anywhere. The browser test runs the client renderer, which removes a nullish attribute, so it passes identically for the branch and for the hole it is supposed to forbid, and the SSR layer genuinely cannot reach a toast because a viewport renders zero of them server-side. Declining the SSR assertion was right. Leaving the regression unguarded was not. It has a source-shape guard now, and I injected the hole to watch it go red before trusting it.

The toast probes racing the 4000ms auto-dismiss is the kind of thing that becomes a mystery months later: a vanished toast resolves to a null chain, a null chain is a hard failure by design rather than a skip, and this now runs unconditionally in the required e2e job.

On the fourth: the reviewer read a stale PR body and reported the test plan as almost entirely unchecked. It was updated before the review landed and now carries seven checked boxes, with only the manual screen-reader pass open. The half of that finding which does stand is Closes #1245, since the issue's acceptance criteria include the manual pass and posting the outcome, and neither is done. Switched to a plain reference so merging cannot auto-close it.

Comment thread packages/ui/packages/registry/components/sonner.ts
Comment thread packages/ui/test/components/browser/ui-a11y.test.js
Comment thread packages/ui/test/ssr-aria.test.js
Comment thread packages/ui/test/e2e/a11y-tree.e2e.mjs
A published post described the wiring this change removed: that the dialog
marks the panel role=dialog with aria-modal=true, and that an ordinary toast
carries role=status. Neither is true any more, so the repo was shipping a test
forbidding role=status on a toast alongside a live page documenting it.

The line about the alert helper's role=alert vs role=status is untouched: that
is the Tier-1 alert component, which this change does not affect.

@vivek7405 vivek7405 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went back over the fix commit and traced what it touched outward.

The three fixes themselves hold. { duration: 0 } really does disable the timer through makeToast and _add, so the probes no longer race dismissal. The new source-shape guard is not vacuous: its assertions fail against the pre-fix spelling, which is the property that matters for a regex-based test, and the comment-stripping pass does not eat any code because this file has no /* inside a string or regex literal. The JSDoc edits keep the A11y block above @example, so the extractDocHeader guard in the same file still holds.

One thing the blast radius turned up, and it is the kind of miss the doc-sync rule exists for. A published blog post still described the old wiring on two counts: that the dialog marks the panel role="dialog" with aria-modal="true", and that an ordinary toast carries role="status". So the repo was simultaneously shipping a test forbidding role="status" on a toast and a live page documenting it. I had updated packages/ui/AGENTS.md and the three JSDoc blocks and stopped there. Fixed in 6104f24a.

I swept the rest of the repo for the same two claims rather than assume that was the only one. The only other tracked hit is examples/blog/components/ui/dialog.ts, an app-owned copy that is 108 lines behind the registry across many unrelated changes. Deferred rather than half-synced, and called out for the owner.

Two review findings, both consequences of the doc correction.

examples/blog vendors its own copy of the dialog component and ui-demo imports
it, so example-blog.webjs.dev was serving the two-nested-dialog-nodes shape
this change removes, while the corrected blog post said it was gone. The
deferral I recorded was wrong: #1146 touched this same vendored copy in the
same commit as the registry, so the repo's precedent is that a registry dialog
fix propagates here. Applied as the same targeted change, not a full re-sync;
the copy predates the label wiring and has none to re-point.

The post's rewritten toast sentence also claimed an error toast is announced
assertively and that the old nesting made some readers announce a toast twice.
Both are announcement-layer claims, and this branch cannot verify either: the
tree measures live-root count and politeness, not utterances, and the manual
screen-reader pass has not run. Reworded to claim only what is measured. The
previous wording made no announcement claim, so the correction had introduced
them.

@vivek7405 vivek7405 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two findings, and both are consequences of the doc fix rather than of the original change, which is exactly what a delta round is for.

First, I was wrong to defer the vendored copy. examples/blog ships its own components/ui/dialog.ts and app/ui-demo/page.ts imports it, so the deployed blog was serving the two-nested-dialog-nodes shape while the post I had just corrected said it was gone. I had reasoned it was an app-owned frozen snapshot that an app updates on its own schedule. The precedent says otherwise: #1146 touched this same vendored copy in the same commit as the registry dialog and alert-dialog. So a registry dialog fix does propagate here, and my judgment lost to prior art. Applied as the same targeted change #1146 made rather than a full re-sync, since the copy predates the label wiring and has none to re-point.

Second, and this one is worse because I introduced it: the rewritten toast sentence claimed an error toast "is announced assertively" and that the old nesting "made some screen readers announce the toast twice". Both are announcement-layer claims, and this branch cannot support either. The tree measures which live root a toast resolves under and with what politeness, not what a reader utters, and the manual screen-reader pass has not run. The PR body says so in as many words. The previous wording made no announcement claim at all, so the correction was a net regression on accuracy. Reworded to claim only what is measured.

Fixed in 61b71df4. Blog e2e 94/94 and webjs check clean on examples/blog after the component change.

The previous commit patched the ARIA in examples/blog's vendored dialog copy,
on the reasoning that #1146 set a precedent for propagating a registry dialog
fix there. Two things make that wrong.

#1146's change to that file was 148 insertions against 18 deletions: it
propagated a whole mechanism, plausibly by re-copying. It is not precedent for
patching one attribute and leaving the rest of a snapshot 108 lines behind.

More concretely, the vendored copy has no accessible-name wiring at all, no
aria-labelledby, no aria-describedby, no label wiring of any kind, because it
predates that work. In the registry the role move is deliberately paired with
the naming wiring and its APG name floor, which exists so an unnamed modal is
impossible. Moving the role onto the native <dialog> there in isolation
produced a dialog exposing exactly one dialog node with NO name, which is a
worse state than the coherent-if-stale snapshot it started from.

The post documents the component as the kit ships it, not as one example app's
old copy renders it, so nothing in the docs depends on this. Bringing that copy
current is a re-sync across the eight components the blog vendored, with its
own testing, and it is not this change.

@vivek7405 vivek7405 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This round is about my own previous commit, and it was right to kill it.

I had patched the ARIA in examples/blog's vendored dialog copy, reasoning that #1146 set a precedent for propagating a registry dialog fix there. Two things undo that. #1146's change to that file was 148 insertions against 18 deletions, so it propagated a whole mechanism, plausibly by re-copying; it is not precedent for patching one attribute and leaving the rest of a snapshot 108 lines behind. And the copy has no accessible-name wiring at all, because it predates that work, so moving the role onto the native &lt;dialog&gt; there in isolation produced a dialog exposing exactly one dialog node with NO name. In the registry the role move is deliberately paired with the naming wiring and its APG name floor, which exists precisely so an unnamed modal is impossible. The partial sync was worse than the coherent-if-stale snapshot it started from, and it shipped with nothing asserting it at any layer.

Reverted in 2c011442. The body now records the reasoning rather than the earlier deferral text.

Worth naming the pattern, since it cost three rounds: correcting a doc surface pulled me into an example app's stale vendored copy, and each fix there implied another. The stopping rule I should have applied earlier is that this post documents the component as the kit ships it, not as one example app's old copy renders it, so no doc claim ever depended on that file.

@vivek7405 vivek7405 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conceding the main finding here, because it is right and it is about my own reasoning.

I justified reverting the vendored-copy sync by saying the synced state was worse: it exposed one dialog node with no accessible name. That is false, and I have withdrawn it from the body. The restored snapshot is equally nameless, with no aria-labelledby, no panel aria-label and no label wiring at all, so it exposes TWO unnamed dialog-family nodes where the synced state exposed ONE. Identical on naming, strictly worse on node count, which is the exact defect the registry half of this PR removes. The registry's one-node result reads as named only because of its APG name floor, which that copy predates. I reached for the most flattering reading of a decision I had already made instead of checking it.

The revert still stands, on the reasons that survive. It shipped with nothing asserting it at any layer. The precedent I leaned on was a whole-mechanism propagation, not a one-attribute patch. And this post documents the component as the kit ships it, so no doc claim depends on that file. What changes is the honesty of the record: the example app is left in a measurably worse state than one commit on this branch had it in, deliberately, for scope and coverage reasons, and the body now says that rather than implying the revert was an improvement.

Also fixed the Test plan bullet citing a blog e2e run as evidence, when the tree state it ran against no longer exists on this branch.

Neither correction touches the branch, so the diff is unchanged at 2c011442.

…panel

The dialog twin of this locator was re-pointed when the role moved onto the
native <dialog>, and this one was not. With the role no longer on the content
div, [role=alertdialog] resolves to the native element, so `inner` and
`native` became the same node and the assertion above was a weaker duplicate
of the one three lines below it, with a message naming an element that no
longer exists as a distinct thing. It passed silently, which is what made it
worth fixing.

Swept the rest of the ui suite for the same class. The only other role-based
dialog locators belong to hover-card, which owns role=dialog on its own panel
and is untouched here.

@vivek7405 vivek7405 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full pass over the finished diff. One finding, and it is a good catch precisely because it was passing.

When the role moved onto the native <dialog>, I re-pointed the dialog locator in ui-overlay.test.js and wrote a comment explaining that a locator conflating a styling hook with an accessibility contract breaks whenever either moves. I then left the alert-dialog twin of that exact locator alone. With the role gone from the content div, [role="alertdialog"] resolved to the native element, so inner and native were the same node, the assertion was a weaker duplicate of the one three lines below it, and its message named an element that no longer existed as a distinct thing. Nothing failed, which is the whole problem.

Fixed in a91dddd3, and I swept the suite for the class rather than fixing the one instance. The only other role-based dialog locators belong to hover-card, which owns role="dialog" on its own panel and is untouched here.

The lesson I am taking from this one: when a change makes a locator wrong in one place, the sweep for its siblings belongs in the same commit as the first fix, not two rounds later.

…owns them

Re-pointing the two locators away from [role=dialog] / [role=alertdialog]
removed this layer's only proof that either role exists. One of those tests is
even named for the role it stopped checking, and after the previous commit
nothing in the browser suite asserted the alertdialog role at all, so both
components could have lost their role entirely with every browser test still
green.

Asserted directly on the native <dialog> rather than through a role-based
lookup, so the check is on the element and cannot quietly degrade into a
selector that passes because it matched something else. Each is paired with an
assertion that the content panel carries no second dialog role, which is the
actual contract this change is about.

Verified discriminating: stripping the role from both native dialogs reds
exactly these two tests on all three engines.

@vivek7405 vivek7405 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked the locator fix and found that it had opened a hole rather than closed one.

Re-pointing the two locators away from [role="dialog"] and [role="alertdialog"] removed this layer's only proof that either role exists. Those lookups were doing double duty: naming an element AND, by resolving at all, proving the role was there. One of the tests is named for the role it stopped checking, and after that commit nothing in the browser suite asserted the alertdialog role anywhere, so both components could have lost their role entirely with every browser test still green.

Fixed in c59ccfdb by asserting the role directly on the native &lt;dialog&gt;, paired with an assertion that the content panel carries no second dialog role, which is the contract this change is actually about. Asserted on the element rather than through a role-based lookup, so it cannot degrade the same way twice. Verified discriminating: stripping the role from both native dialogs reds exactly these two tests on all three engines.

The general shape, since it has now happened twice on this branch: a locator that selects by the thing under test is load-bearing in a way that is invisible until you move it, and swapping it for a stabler selector silently deletes an assertion nobody wrote down.

One honest note on the suite. A full browser run showed a single Firefox failure that did not reproduce: the next full run was clean on all three engines, and the file I touched passes Firefox three times out of three. I could not identify which test it was before the output scrolled, so I am recording it as an unexplained flake elsewhere in the suite rather than claiming it was nothing.

The name said the content has role=alertdialog while the body now asserts the
content has no role at all, so it advertised the opposite of what it checks.

@vivek7405 vivek7405 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked the added role assertions. They hold: each targets the element that actually carries the role on this branch, each fails on a removed role or a missing element, and the paired negatives are true of the current templates, since neither content panel writes a role and neither label-wiring function adds one.

One thing left over, and it is cosmetic rather than structural: the alert-dialog test was still named for the content having role="alertdialog" while its body now asserts the content has no role at all, so the name advertised the opposite of the check. Renamed in 55f4f782. The only other test name referencing these roles belongs to hover-card, which does put role="dialog" on its own content and is untouched here, so that one is still accurate.

That ends the review cycle. This round found nothing that changes behaviour or weakens an assertion, which is the stopping condition.

@vivek7405
vivek7405 marked this pull request as ready for review August 14, 2026 17:59
Handles the one finding this PR had left deferred, properly this time.

examples/blog vendors its own dialog copy and ui-demo imports it, so the
deployed demo exposed the two-nested-dialog-nodes shape the kit no longer has.
An earlier commit on this branch moved the role there and was reverted, for two
good reasons: it shipped with nothing asserting it, and the copy has no
accessible-name wiring at all, so declaring a role in isolation produced a modal
that announces itself as a dialog with nothing to read. Measured: the name was
the empty string.

So both halves land together. The role moves onto the native <dialog>, and the
naming wiring comes with it, including the APG floor that makes an unnamed modal
impossible. The title lookup covers this app's <ui-dialog-title> and
<ui-dialog-description> markup, which the vendored copy does not register and
which therefore leaves no heading for the usual fallback to find.

Tested at both layers it is observable from. The role is in render(), so the
smoke test asserts it on the first paint. Naming happens at showModal(), so the
e2e opens the dialog and asserts the resolved name is non-empty. Each was seen
red: reverting the role move reds the smoke assertion, and removing the naming
wiring reds the e2e.
…h open

Three defects, two of them introduced by this PR.

The HTML Standard says the tabindex attribute must not be specified on dialog
elements. Moving role onto the native <dialog> brought tabindex="-1" along
with it, which put a conformance error into both kit components and the blog's
copy. It was never doing work either: showModal()'s focusing steps already fall
back to the dialog itself when there is no focus delegate, and a tabindex="-1"
descendant was never eligible to be that delegate, since a delegate has to be
tabbable. Removed from all three.

The naming wiring skipped when the panel already carried the attribute, but the
panel keeps its attributes between opens, so it could never re-resolve. Remove a
title node and re-open, and the stale aria-labelledby survives pointing at a
dead IDREF, resolving to no name, while also suppressing the floor that exists
to make an unnamed modal impossible. Both failures are silent. Each pass now
clears what the previous one wrote before resolving. This shape was already in
the kit and is fixed there too, in dialog and alert-dialog alike.

The e2e name assertion could not fail: a non-empty check is satisfied by the
generic floor alone, so the whole title lookup could be deleted and it would
stay green. It now asserts the resolved name and description, and that each
IDREF points at a live element.

@vivek7405 vivek7405 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three findings on the vendored-copy commit, and two of them turned out to be defects this PR had put into the KIT, not just the example app. Worth the round on its own for the first one.

tabindex on <dialog> is a conformance error. The HTML Standard says the attribute must not be specified on dialog elements, and moving the role onto the native element brought tabindex="-1" along with it, into both kit components and the blog copy. It was never doing work either: showModal()'s focusing steps already fall back to the dialog itself with no focus delegate, and a tabindex="-1" descendant could never have been that delegate, since a delegate has to be tabbable. Removed from all three. The tree e2e still reports one correctly named node per case afterwards, which is the check that it was inert.

The naming pass could never re-resolve. It skipped when the panel already carried the attribute, but the panel keeps its attributes between opens. So: open with a title, remove the title node, re-open, and the stale aria-labelledby survives pointing at a dead IDREF. That resolves to no name AND suppresses the floor whose entire job is making an unnamed modal impossible, and both failures are silent. Each pass now clears what the previous one wrote before resolving. Same shape was already in the kit's dialog and alert-dialog, so it is fixed there too. Covered by a new re-open test, seen red against the old guard.

The e2e name assertion could not fail. name.length > 0 is satisfied by the generic floor alone, so the entire title lookup could be deleted and it would stay green, and the description half was asserted nowhere at all. It now asserts the resolved name is the demo's actual title, the resolved description, and that each IDREF points at a live element.

Fixed in 21e63326. ui unit 219/219, browser 882/872/882 across three engines, tree e2e 16/16, blog e2e 95/95, webjs check clean on blog and website.

The pattern I keep hitting on this branch: moving an attribute is not a mechanical operation. The role move carried a tabindex that was illegal on its destination, and pointing the naming wiring at a longer-lived element exposed a stale-state bug that had been latent while it wrote to a shorter-lived one.

@vivek7405
vivek7405 merged commit 9b0af33 into main Aug 15, 2026
10 checks passed
@vivek7405
vivek7405 deleted the fix/screen-reader-a11y-pass branch August 15, 2026 04:34
vivek7405 added a commit that referenced this pull request Aug 15, 2026
Clears every package carrying unreleased user-facing work since 0.7.50.

core adds refreshPage, which re-renders the current url on the server and
applies it in place with no reload, no history entry and no scroll, and
prefetches a link that drives a frame in that frame's own dimension so the
click costs no round trip. The fix records a same-document history entry
before the swap rather than after, which is what made an iOS swipe-back
preview the page the user was already on.

server stops a dev edit to a page or layout reloading the browser, so hydrated
state and scroll survive a save, and marks a sliced frame response so the
client can tell a real subtree from a whole document. The fixes render an
error boundary inside its own layout chain, coalesce a burst of dev reload
signals so rapid edits stop leaving the page unstyled, and root the vendor
specifier scan in the module graph.

ui takes the a11y fix alone.

Three packages are deliberately absent. cli, mcp and intellisense carry no
user-facing change in the range. cli's only such commit was #1414, which #1417
reverted in full, so what remains is the doctor barrel split, an internal move
with an unchanged export surface. mcp picked up three doc-string edits from
the same refactor, naming the new sibling-directory paths its `source` tool can
read, and the paths it already documents still resolve because they are
barrels. intellisense picked up nothing at all.

Raises packages/server's declared @webjsdev/core range from ^0.7.50 to
^0.7.51. The dev refresh reads refreshPage through a runtime feature check
rather than a static import, so nothing breaks without it, but the release PR
is the only place that bump is legal and core carries the earliest date in the
batch so it publishes first.

core and server carry the generated notes verbatim. ui is edited twice, both
because the generator reads conventional-commit prefixes and cannot see
anything else. It wrote #1414 into the entry for work #1417 has since reverted,
which is removed along with the Features section it emptied; and its excerpt is
the first lines of a squashed message, which for #1412 opened on the CDP
measurement instrument rather than on the fix, so the fix sub-commit takes its
place.
vivek7405 added a commit that referenced this pull request Aug 15, 2026
Clears every package carrying unreleased user-facing work since 0.7.50.

core adds refreshPage, which re-renders the current url on the server and
applies it in place with no reload, no history entry and no scroll, and
prefetches a link that drives a frame in that frame's own dimension so the
click costs no round trip. The fix records a same-document history entry
before the swap rather than after, which is what made an iOS swipe-back
preview the page the user was already on.

server stops a dev edit to a page or layout reloading the browser, so hydrated
state and scroll survive a save, and marks a sliced frame response so the
client can tell a real subtree from a whole document. The fixes render an
error boundary inside its own layout chain, coalesce a burst of dev reload
signals so rapid edits stop leaving the page unstyled, and root the vendor
specifier scan in the module graph.

ui takes the a11y fix alone.

Three packages are deliberately absent. cli, mcp and intellisense carry no
user-facing change in the range. cli's only such commit was #1414, which #1417
reverted in full, so what remains is the doctor barrel split, an internal move
with an unchanged export surface. mcp picked up three doc-string edits from
the same refactor, naming the new sibling-directory paths its `source` tool can
read, and the paths it already documents still resolve because they are
barrels. intellisense picked up nothing at all.

Raises packages/server's declared @webjsdev/core range from ^0.7.50 to
^0.7.51. The dev refresh reads refreshPage through a runtime feature check
rather than a static import, so nothing breaks without it, but the release PR
is the only place that bump is legal and core carries the earliest date in the
batch so it publishes first.

core and server carry the generated notes verbatim. ui is edited twice, both
because the generator reads conventional-commit prefixes and cannot see
anything else. It wrote #1414 into the entry for work #1417 has since reverted,
which is removed along with the Features section it emptied; and its excerpt is
the first lines of a squashed message, which for #1412 opened on the CDP
measurement instrument rather than on the fix, so the fix sub-commit takes its
place.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Screen-reader pass for the two needs-runtime-check a11y items from #1080

1 participant