fix(pg-sveltekit): give the example's controls accessible names and state - #152
Conversation
…tate From dobby's review of #145. The file is byte-identical to the archived postguard-examples, so this is relocated debt rather than something the import introduced -- but it is the app docs.postguard.eu teaches from, in a repo whose stated bar is WCAG 2.2 AA, and it would otherwise stay buried in a 4000-line import. Six controls were placeholder-only: the citizen name and email, the organisation name and email, the API key, and the message textarea. A placeholder is not a label -- the accessible name disappears the moment the user types, and there is no programmatic field/label association at all. That fails 1.3.1, 3.3.2 and 4.1.2. Each control is now wrapped in a <label> with visible text, and the placeholders that carried a sample value keep it as a format hint rather than as the name. The delivery-mode toggle carried its selection only in `class:active`, so a screen-reader user could not tell which mode was active. Both buttons now expose `aria-pressed`, per the ARIA APG pattern for a toggle pair, and both got the explicit `type="button"` they were missing. ProgressBar was three bare divs: no role, no value, no accessible name (4.1.2), and no announcement as it advanced (4.1.3). The track now carries role="progressbar" with aria-valuenow/min/max, aria-valuetext matching the rounded percentage the visible text shows, and its name from the existing label via aria-labelledby -- falling back to aria-label when no label is passed, so the name is never absent. aria-live="polite" sits on the container rather than the track so the label is part of the announcement. The error box had no role="alert", so a failed send was silent to assistive technology. It has one now. One CSS consequence, since it would otherwise have been a latent warning: `section > input` matched the API key field as a direct child, and wrapping it in a label made that selector dead. Replaced with `.field input`, which covers every input now that they all sit inside one. svelte-check went from 1 warning back to 0 errors / 0 warnings. Verified at the built output rather than the source: role="progressbar", aria-valuenow, aria-pressed, aria-live and role="alert" all appear in .svelte-kit/output, and the new label text renders. Build and lint clean. Not covered here: FileDropzone, which has no aria attributes at all and wants its own look, since a drop target's keyboard and screen-reader story is a design question rather than an attribute fix. Refs #145
|
Thanks for opening this! I'm on it — reviewing #152 now for correctness and WCAG 2.2 AA compliance, and I'll follow up with a consolidated review shortly. |
There was a problem hiding this comment.
Net accessibility win, and the mechanism is right in most places: six real labels with programmatic association, aria-pressed on the toggle pair, a named progressbar with a value, role="alert" on the error box. All 20 checks are green, including Svelte Check, the Node examples lane (typecheck + build + lint over every example) and the Conventional Commit title check. Two things in ProgressBar.svelte need a change before merge; the other four inline notes are optional.
Blocking
id="progress-label"is a hardcoded literal in a reusablelib/componentsfile, so the accessible name of the second instance on a page resolves to the first instance's label, and the duplicate id is invalid HTML. Only one instance exists today (+page.svelte:269), so nothing is broken for a user right now, but this is the copy docs.postguard.eu readers lift. Svelte is 5.56.8, so$props.id()fixes it in two lines.- The new comment says
aria-livesits on the container "so the label is part of the announcement", butaria-atomicdefaults tofalse, so AT announces only the changed node, which is.progress-text. The code and its own comment disagree. Either setaria-atomic="true"or reword the comment to say the announcement is a bare percentage.
Not introduced here, and not for this PR
Three contrast values on this page still miss AA. I recomputed each with the sRGB formula rather than trusting a reported figure, and one number that reached me was wrong:
| pair | ratio | needs | criterion |
|---|---|---|---|
.hint / .mode-desc #888 on white |
3.54:1 | 4.5:1 | 1.4.3 |
input border #ccc on white |
1.61:1 | 3:1 | 1.4.11 |
progress fill #4a9 on track #eee |
2.43:1 | 3:1 | 1.4.11 |
The progress-fill figure was handed to me as 2.19:1; the correct value is 2.43:1. It still fails, so the conclusion holds.
Two more that sit on the buttons this PR touches, both from untouched declarations: .mode-btn.active .mode-desc is #6b7280 on #eff6ff = 4.44:1 at 0.8rem (12.8px is not large text, so 1.4.3 wants 4.5:1), and the button boundary is #e2e8f0 resting = 1.23:1 and #cbd5e1 on hover = 1.48:1 against the 3:1 that 1.4.11 asks of a control boundary. All five are colour-token changes rather than markup, so a separate PR is the right home, same call you already made for FileDropzone.
On the missing test
examples/pg-sveltekit has no test script, no test runner in its devDependencies and no test directory, and examples.yml only requires each example to declare typecheck. Standing up a harness to assert an aria attribute would be a larger change than the fix, so no regression test here is the right call. role="progressbar", aria-valuenow, aria-pressed, aria-live and role="alert" in .svelte-kit/output is a reasonable substitute, and I confirmed the .field input rewrite loses nothing: the textarea keeps its own rule at line 366 with the same padding, border, font-size and font-family, so wrapping it in a .field label did not strip its styling.
| <div class="progress-container" aria-live="polite"> | ||
| {#if label} | ||
| <div class="progress-label">{label}</div> | ||
| <div class="progress-label" id="progress-label">{label}</div> |
There was a problem hiding this comment.
Blocking. id="progress-label" is a hardcoded literal inside a reusable component, so every instance emits the same id. With two labelled <ProgressBar>s on one page the second one's aria-labelledby resolves to the first one's label, giving it the wrong accessible name, and duplicate ids are invalid HTML besides.
Only one instance exists today (+page.svelte:269), so no user hits it yet. It matters because this is the shared lib/components copy that docs readers lift, and because a wrong accessible name is the exact failure mode the rest of this PR fixes.
Svelte is 5.56.8 here, so $props.id() is available and this is two lines:
const labelId = $props.id();then id={labelId} on line 21 and aria-labelledby={label ? labelId : undefined} on line 31.
| updates silently. It sits on the container rather than the track so the label | ||
| is part of the announcement. | ||
| --> | ||
| <div class="progress-container" aria-live="polite"> |
There was a problem hiding this comment.
Blocking, though either resolution is a one-line change.
The comment above says aria-live sits on the container "so the label is part of the announcement". aria-atomic defaults to false, so assistive tech announces only the changed descendant, which is the .progress-text node on line 35, not the label div. What a user actually hears is 42%.
Getting the stated behaviour needs aria-atomic="true" on this element, and that has a real cost: packages/pg-js/src/crypto/encrypt.ts uploads in 5 MB chunks, so a 100 MB upload fires roughly 20 progress events, and each one would re-announce the whole string Encrypting & uploading... 42%.
A bare percentage every few seconds is arguably the better experience, so rewording the comment is a fine fix. What should not ship is a comment in a teaching example that describes behaviour the code does not have.
| <div | ||
| class="progress-track" | ||
| role="progressbar" | ||
| aria-valuenow={Math.round(percentage)} |
There was a problem hiding this comment.
Non-blocking. aria-valuenow is unclamped while .progress-fill on line 33 is, so <ProgressBar percentage={120} /> renders aria-valuenow="120" against aria-valuemax="100" while the bar visually stops at 100%. ARIA requires aria-valuenow to fall within the min/max range.
The app cannot reach it, since encrypt.ts already clamps with Math.min(100, Math.round(...)) before calling onProgress. But the component takes an arbitrary percentage prop, and the original author clamped the fill for exactly this reason. One clamped local reused by the fill, aria-valuenow and aria-valuetext covers all three.
| <input type="text" bind:value={citizenName} placeholder="Name" /> | ||
| <input type="email" bind:value={citizenEmail} placeholder="citizen@example.com" /> | ||
| <label class="field"> | ||
| <span class="field-label">Name</span> |
There was a problem hiding this comment.
Non-blocking, 2.4.6 Headings and Labels. The organisation fields got qualified names ("Organisation name", "Email address at the organisation") but the citizen ones stayed bare: "Name" and "Email address".
In forms mode or when browsing the form field by field, a screen-reader user gets the label without the surrounding <h2>2. Citizen recipient</h2>, so two of the four recipient fields are ambiguous against the two organisation fields directly below them. "Citizen name" and "Citizen email address" would match the qualification you already applied one section down.
| display: flex; | ||
| flex-direction: column; | ||
| gap: 0.25rem; | ||
| flex: 1; |
There was a problem hiding this comment.
Non-blocking. flex: 1 does nothing useful at either place .field lands.
Inside .field-group, which is flex-direction: column on line 351, flex: 1 1 0% does not put the two fields side by side. It makes them grow to equal height, which surfaces as dead space once one label wraps at a narrow width. At the two section-level usages (API key on line 240, message on line 254) .field is not a flex item at all, so the declaration is inert there.
Either drop it, or make .field-group a row above some breakpoint if side-by-side was the intent.
| <section> | ||
| <div class="mode-toggle"> | ||
| <button | ||
| type="button" |
There was a problem hiding this comment.
Non-blocking. type="button" went on the two mode buttons but not on the other three: "Start over" (line 168), "Cancel" (line 270) and the submit button (line 273).
Nothing breaks today, because no <form> wraps any of them, which is also why the two that got it were not broken to begin with. So either apply it to all five or leave it out. Half-applied is the one state that reads as an oversight to the next person.
|
Reviewed #152 and left a request-changes review with 6 inline comments. Nice fix overall — all 20 CI checks are green. Two blocking items in
4 optional notes: unclamped Also corrected one of my own numbers along the way: the progress-fill contrast is actually 2.43:1, not 2.19:1 — still fails 1.4.11's 3:1 minimum, so the finding stands. Found two more pre-existing AA contrast gaps on buttons this PR touches, but since they're separate color-token work I left them as a follow-up rather than blocking this PR on them (same call you made for |
…overclaiming
Two blocking review findings in ProgressBar, plus four optional ones taken.
The label id was a hardcoded literal in a reusable lib/components file, so
every instance emitted the same id: a second labelled <ProgressBar> on one page
would resolve its accessible name to the FIRST one's label, and duplicate ids
are invalid HTML. Only one instance exists today, so no user hits it -- but a
wrong accessible name is precisely the failure the rest of this branch fixes,
and this is the copy docs.postguard.eu readers lift. svelte 5.56.8 supports
$props.id(), so it is per-instance now.
The aria-live comment contradicted the code. It said the label is part of the
announcement, but aria-atomic defaults to false, so assistive tech announces
only the changed node -- the percentage text. Resolved by rewording rather than
setting aria-atomic="true", and the reason is in the comment: the SDK uploads
in 5 MB chunks, so a 100 MB upload fires around 20 progress events and
re-announcing "Encrypting & uploading… 42%" each time is worse than the bare
percentage. The label remains the track's accessible name via aria-labelledby,
so it is available on focus.
aria-valuenow is clamped now. The fill was already clamped because the prop is
arbitrary, but aria-valuenow was not, so <ProgressBar percentage={120} /> would
have emitted aria-valuenow="120" against aria-valuemax="100", which ARIA
forbids. One derived value feeds the fill, aria-valuenow, aria-valuetext and the
visible text.
The citizen fields are qualified, matching the organisation ones. "Name" and
"Email address" were ambiguous against "Organisation name" and "Email address
at the organisation" directly below when navigating field by field, where the
section heading is not read (2.4.6).
Dropped `flex: 1` from .field. Inside .field-group it equalised HEIGHT rather
than putting fields side by side, since that container is column-direction --
visible as dead space once a label wrapped -- and at the two section-level
usages .field is not a flex item at all.
type="button" now on all five buttons rather than the two this branch touched.
Nothing breaks either way, since no <form> wraps them, which is why the two
were not broken to begin with; half-applied is the state that reads as an
oversight.
Not fixed here, and not introduced here: five colour-contrast failures on this
page (.hint/.mode-desc #888, the #ccc input border, the #4a9 fill on #eee,
.mode-btn.active .mode-desc, and the button boundary). All are colour-token
changes rather than markup, so they belong with FileDropzone in their own pass.
svelte-check 0 errors / 0 warnings, build and lint clean, and verified in the
built bundle: no hardcoded id, aria-labelledby present, aria-atomic absent as
documented.
Refs #145
|
/dobby both blockers fixed in The hardcoded On Clamped Qualified the citizen labels ("Citizen name", "Citizen email address"). The field-by-field navigation argument is the one I had not thought through — the Dropped On the contrast table — thank you for recomputing rather than passing on the figure you were handed. One of those numbers reaching you wrong (2.19:1 against the correct 2.43:1) is a good argument for the habit, even though the conclusion held. Agreed all five are colour-token changes rather than markup and belong with Verified in the built bundle rather than the source: no hardcoded id, |
|
Thanks for the detailed rundown — I'm on it now, taking a look at |
There was a problem hiding this comment.
Approving. This clears the CHANGES_REQUESTED this bot left on 2026-07-29; nothing below blocks the merge.
Both blockers from the last round are genuinely fixed, and I checked the rendered output rather than only the source:
$props.id()emits a per-instance id, so the label association no longer depends on there being exactly one ProgressBar on the page.- The clamp holds at every boundary, and one derived value feeds the fill,
aria-valuenow,aria-valuetextand the visible text, so they cannot diverge. aria-atomicis absent from the component, so the comment describing a bare percentage announcement is accurate. The singlearia-atomic="true"in.svelte-kit/outputis SvelteKit's own route announcer inapp.*.js, not this component.
Checks I ran on 0519f4c after a fresh install: pnpm check reports 333 files, 0 errors, 0 warnings; pnpm lint and pnpm build both exit 0.
Two notes, neither blocking.
1. The description still carries the claim this commit removed from the file
The inline comment covers the sibling slip in the code comment. The description says:
aria-live="polite"sits on the container rather than the track, so the label is part of the announcement instead of a bare number.
That is the pre-fix reasoning. The comment you rewrote in this commit now says the opposite, correctly: with aria-atomic at its default of false, what gets announced is the changed node alone, a bare "42%", not the label and percentage together. You took the reword over aria-atomic="true" deliberately and gave the cost argument for it, so the code is right and the description is still describing the version that lost that argument.
It is a one-sentence body edit, so no branch push and no CI rerun. Something like "so the label stays the track's accessible name via aria-labelledby, while the announcement itself is the bare percentage" would match what shipped. Worth doing because the description is what a reader meets first, and this branch is specifically about not letting a description of accessible behaviour drift from the behaviour.
2. The success path has no announcement, at +page.svelte:105
Pre-existing and outside every diff hunk, so I cannot anchor it inline. The error path gained role="alert" here, but when sendState flips to done the top-level {#if} swaps the whole form out: the aria-live progress region unmounts, .success-box carries no role="status", and the focused submit button stops existing, so focus falls back to <body>. A screen reader user gets progress announcements and then silence, with the virtual cursor reset, while the failure case announces itself. role="status" on .success-box, or moving focus to its <h2>, would close it.
Reasonable to defer to the same pass as FileDropzone and the colour tokens, since it is page-level focus management rather than control naming.
Nothing else came up. The five contrast items stay where we left them, as agreed. I did recompute the one colour this diff actually adds: .field-label at #444 on white is 9.74:1, which passes AA with room.
| const labelId = $props.id(); | ||
|
|
||
| // aria-valuenow has to sit inside min/max per ARIA, and the fill was already | ||
| // clamped for the same reason — the prop is arbitrary even though the app's |
There was a problem hiding this comment.
The clamp is right and the reasoning behind it is right: the value arriving here is already clamped upstream, so the component's own clamp is belt and braces. Only the attribution is wrong.
The app's onProgress is (pct: number) => (progress = pct) at +page.svelte:52, a bare assignment that clamps nothing. The clamping is the SDK's, at packages/pg-js/src/crypto/encrypt.ts:112: Math.min(100, Math.round((uploaded / totalSize) * 100)).
It matters slightly more than a usual comment nit because this is the reusable lib/components copy a docs reader lifts, and someone who reads this and goes looking in +page.svelte for the clamp will not find one. "even though the SDK clamps before calling it" would be accurate.
The 5 MB chunk figure in the comment above checks out: DEFAULT_UPLOAD_CHUNK_SIZE = 5_000_000 at encrypt.ts:14.
|
Approved: #152 (review)
Two non-blocking notes left, one new: the PR description still argues the pre-fix reasoning for Everything else came back clean — dropped a couple of prose flags that were about my own writing, not yours. |
From dobby's review of #145.
+page.svelteis byte-identical to the archivedpostguard-examples, so this is relocated debt rather than something the import introduced — but it is the app docs.postguard.eu teaches from, in a repo whose stated bar is WCAG 2.2 AA, and it would otherwise stay buried in a 4000-line import.Six placeholder-only controls
The citizen name and email, the organisation name and email, the API key, and the message textarea had no label at all. A placeholder is not one: the accessible name disappears the moment the user types, and there is no programmatic field/label association. That fails 1.3.1, 3.3.2 and 4.1.2.
Each control is now wrapped in a
<label>with visible text. Placeholders that carried a sample value (citizen@example.com,PG-1a2b3c4d5e6f7g8h) keep it as a format hint rather than as the name.The delivery-mode toggle had no exposed state
Selection lived only in
class:active, so a screen-reader user could not tell which mode was active. Both buttons now exposearia-pressed, per the ARIA APG toggle-pair pattern, and both got the explicittype="button"they were missing.ProgressBar was three bare divs
No role, no value, no accessible name (4.1.2), and no announcement as it advanced (4.1.3). The track now carries
role="progressbar"witharia-valuenow/min/max, plusaria-valuetextmatching the rounded percentage the visible text shows rather than a long float.Its name comes from the existing label via
aria-labelledby, falling back toaria-labelwhen no label is passed — so the name is never absent either way.aria-live="polite"sits on the container rather than the track, so the label is part of the announcement instead of a bare number.The error box was silent
No
role="alert", so a failed send was invisible to assistive technology. It has one now.One CSS consequence, fixed rather than left latent
section > inputmatched the API key field as a direct child; wrapping it in a label made that selector dead, whichsvelte-checkflagged. Replaced with.field input, which covers every input now that they all sit inside one. Back to 0 errors / 0 warnings.Verified at the built output
Not just the source —
role="progressbar",aria-valuenow,aria-pressed,aria-liveandrole="alert"all appear in.svelte-kit/output, and the new label text renders. Build and lint clean.Not covered here
FileDropzone.sveltehas no aria attributes at all. A drop target's keyboard and screen-reader story is a design question rather than an attribute fix, so it wants its own change rather than being guessed at in this one.Refs #145.