Associate FieldErrors with its input for assistive technology #74
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/field-errors-aria"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Implements task 157.
FieldErrorsgains a requiredIdparameter, rendered on its<ul>; every call site pairs it with the input's ownaria-describedby, the same explicit shape<label for>already uses. Where an input already had a hint (Register's password field), the error id is added to the existing token list rather than replacing it.Live announcement stays on submit-appearance (
role="alert"plus an explicitaria-live="assertive", matching ADR-0063): every consumer's client-side validation runs only on submit, never per keystroke, so this can't become a keystroke-repeating live region, and a visitor who submits and doesn't immediately tab back still hears why.aria-describedbyis what makes the same message available on demand afterwards.Covers all five current usages —
Login,Register,GroupForm,PlaceForm, andGroupMembersPanel(merged in #73 after this branch started; picked up on rebase). Each has a bUnit test reading the input'saria-describedbyback against the id actually rendered on the error list, not just that both exist — verified to redden under mutation on both sides before pushing.Reasoning, and the alternatives rejected, in ADR-0066 (next free number, confirmed against
mainand the open-PR list — none open at push time).What bUnit cannot show (task 156, as ever): no browser or screen reader, so nothing here proves the live region announces once per screen reader's own behaviour, or that a real AT reads the
aria-describedbyassociation back. What's proved is that the attributes are present and correctly paired.Verdict: mergeable
Verified independently, not just re-checked the PR's own claims:
FieldErrorscall sites myself (grep, not the PR body's list): 12 usages acrossLogin,Register,GroupForm,PlaceForm,GroupMembersPanel— matches.GroupMemberRow'saria-describedbyis a confirm-button/description pairing, unrelated to this ticket, correctly left untouched.FieldErrors.Idand separately an input'saria-describedbyand confirmed the matching bUnit test reddens (Login,Register,GroupMembersPanelchecked directly;GroupForm/PlaceFormuse the identical assertion shape). Reverted each mutation; fullPlaceMark.WebUI.Testssuite is green (285 passed, 4 pre-existing skips) withdotnet 10.0.100.Register's password hint: mutated the input'saria-describedbyto dropregister-password-hint, confirmedRegisterTestsreddens on the missing token. The hint survives alongside the error id as claimed.place-form-latitude-errorsvsplace-form-longitude-errors, etc.) — nothing duplicated, though this remains a review-only guarantee sinceIdisn't derived or checked for collision, which ADR-0066 acknowledges.[Parameter, EditorRequired] Idgenuinely fails the build (RZ2012) when omitted, by temporarily stripping one call site'sIdand rebuilding.ContractValidation.Validate/_fieldErrorsassignment happens only insideSubmitAsync/InviteAsync, never from a change/input/blur handler.@bind:event="oninput"only updates the bound field, nothing else. Also confirmed_generalError(ErrorNotification, assertive) and_fieldErrors(FieldErrors, assertive) are set in mutually exclusive branches on every submit path (GroupForm,PlaceFormbothif (exception.Errors.Count > 0) … else …), so the two assertive regions never both fire for one submission.One thing worth writing down that isn't: ADR-0066's safety argument for
aria-live="assertive"is entirely conditional on "validation only happens in a submit handler," which today is true by inspection but is enforced nowhere — no test, no architecture rule, not even a comment atContractValidation.Validate's call sites. A future consumer wiring it toonbluroronchangewould silently reintroduce the exact keystroke/interaction-repeating live region ADR-0063 warns against, and nothing here would catch it. ADR-0063 itself only had to hold this invariant for one component's own diffing; this ADR extends the same assumption to every futureFieldErrorscaller with no seam to keep it true. Not blocking — same review-only posture the codebase already accepts elsewhere (ADR-0012, ADR-0027) — but worth a line in ADR-0066's Consequences or a comment next to the<ul aria-live="assertive">naming the invariant, since right now it's only implicit in the Decision's prose.role="alert"+ explicitaria-live="assertive"is not a double-announcement risk: they express the same live-region properties on one element (role="alert" implies aria-live="assertive"/aria-atomic="true" by default), not two separate regions — same shape ADR-0063 already established forErrorNotification, applied consistently here.What is and isn't verifiable, bounded honestly: bUnit and this review can confirm the markup — the id genuinely matches, the hint token survives, the attributes are correctly paired, RZ2012 genuinely fires. Nothing here (or in the test suite) can confirm that a real screen reader announces the live region once rather than per-AT quirk, or reads the
aria-describedbyassociation back at all — ADR-0066's own Consequences section says this plainly and doesn't overclaim.Verdict: changes needed
Delta re-reviewed at
472021f(docs/adr/0066…,FieldErrors.razor.csdoc comment, and the newFieldErrorsValidatesOnSubmitOnlyTestsonly — confirmed nothing else moved viagit diff 59d5fea..472021f --stat).Reproduced the disclosed
@onblurhazard onLogin: it fails, and the message names the file, the@on<event>="Method"attribute, and the code-behind path — actionable. Confirmed the fullPlaceMark.Architecture.Testssuite (17 tests, including this one) is green on the unmodified tree, and confirmed all three disclosed evasions behave exactly as documented (inline lambda evades; indirection via a second method evades; renaming_fieldErrorsalone does not evade, becauseContractValidation.Validate(is still matched directly — the ADR's own caveat about needing to "wrap that call too" is accurate).Two things the disclosed-limits list misses:
A fourth, undisclosed evasion:
@onblur="@ValidateEmailOnBlur"(leading@inside the attribute string — legal, older-style Razor event-binding syntax, compiles and behaves identically to the bare form) is not matched byNamedEventHandlerAttribute()'s regex, because"(\w+)"requires the quoted value to start immediately with a word character. Built and confirmed this compiles and the architecture test passes it silently. This is the exact "look like protection, checks nothing" gap the task brief warns about, and it's a one-line fix ("@?(\w+)") rather than something that needs disclosing-and-living-with — recommend fixing the regex over adding a fourth bullet to the limits list.A false positive from source text including comments:
MethodBodyextracts raw text by brace-matching, it doesn't strip comments. A non-submit handler that deliberately does not validate, but says so in a comment mentioning the literal textContractValidation.Validate((e.g. "// deliberately does not call ContractValidation.Validate( here — see ADR-0066"), trips the check and fails, naming a handler that doesn't actually validate. Reproduced directly. This is a real risk in this specific codebase's own commenting style — every file in this PR carries exactly this kind of explanatory inline reasoning — and it's the "checked and non-validating" case, not "not checked," making it worse than a documented gap: it will happen to a future author trying to be careful. Worth at least disclosing alongside the other three if not fixed (e.g. strip//line comments before theRunsFieldValidationmatch).Both are within scope of this PR's own new file, not pre-existing debt, so raising them here rather than as follow-up.
Consistent, no drift: ADR-0066's new paragraph, the
FieldErrorsdoc comment, and the test class's own XML doc state the same condition (validate on submit only) in the same terms. The doc comment doesn't restate the three limits — it delegates to the test's own doc comment for those — which is a reasonable way to avoid a second copy drifting, not an inconsistency.Verdict: changes needed
Delta re-reviewed at
c898f24(docs/adr/0066…andFieldErrorsValidatesOnSubmitOnlyTests.csonly, pergit diff 472021f..c898f24 --stat;FieldErrors.razor.csuntouched).Both prior findings hold:
@onblur="@HandleEmailBlur"reddens (message correctly resolves toHandleEmailBlur), the bare form still reddens, and a comment merely mentioningContractValidation.Validate(in a genuinely non-validating handler no longer false-positives — confirmed directly onLogin, reverted after each. Also confirmed the fix doesn't overcorrect: a real call sitting next to that same kind of comment still reddens.Attacked the new four-item list the same way as the old three-item one and it's still short:
A fifth, undisclosed evasion: single-quoted attribute values.
@onblur='HandleEmailBlur'is legal Razor (HTML permits single-quoted attribute values and Blazor's compiler accepts it) and compiles unchanged.NamedEventHandlerAttribute()'s pattern hard-codes"@?(\w+)"— double quotes only — so this passes silently. Built and confirmed it compiles and the test doesn't catch it. None of the four disclosed items cover quote-character choice; item 4 only names casing/spelling of the event name, not the attribute's quoting. One-line fix (["']@?(\w+)["'], checking both delimiters match) rather than a fifth bullet.A sixth gap, already written down but not counted among the "four".
WithoutCommentsOrStringLiterals's own doc comment discloses that an interpolation hole is stripped along with its enclosing string, so "real validation logic hidden inside a string interpolation hole would...evade this reader too" — and that's true:_generalError = $"{ContractValidation.Validate(request).Count} problems"in a non-submit handler compiles and passes silently, confirmed directly. This is a real, already-documented gap, but it isn't rolled into the class-level "four gaps remain" list or into ADR-0066's mirroring prose, both of which now undercount by at least one relative to the file's own documentation. Either fold it into the headline list as a fifth item, or the "four" claim in both places is wrong on its own terms.Checked casing/spelling of the event name itself (disclosed item 4's specific worry):
@onBlur="HandleEmailBlur"compiles and is caught (the pattern's\w+and theOrdinalIgnoreCasecomparison against"submit"handle case variation fine) — so item 4 as stated is more cautious than the implementation actually warrants, not itself a false claim, just conservative.Verified the false-positive fix doesn't swallow real code via the naive quote-pairing in
WithoutCommentsOrStringLiterals: traced and probed a nested-quote-inside-interpolation-hole case ($"Value: {Foo("x")}"followed by a realContractValidation.Validate(call) — the mis-paired scan still resynchronises correctly at the true end of the string cluster because valid C# always has a balanced quote count, so no false negative there. Not blocking, just recording that the check held.ADR-0066's new prose and the test's XML doc state the same four items in the same terms — no drift between those two — but see the sixth-gap finding above: both are consistent with each other while being simultaneously incomplete relative to the same file's own
WithoutCommentsOrStringLiteralsdoc comment.Verdict: mergeable
Delta re-reviewed at
70803d3(docs/adr/0066…,FieldErrors.razor.csremark,FieldErrorsValidatesOnSubmitOnlyTests.cs).Single-quote fix:
@onblur='HandleEmailBlur'reddens;@onblur='@HandleEmailBlur'(single-quote +@-prefix combined) reddens with the method name correctly resolved without the@; the bare double-quoted form still reddens. All three verified on a real consumer (Login), not just the isolated regex theory cases. The backreference ((?<quote>["'])…\k<quote>) does its job — no cross-contamination between quote styles observed.Mixed-case regression test genuinely pins the behaviour: reverted the fix to
[a-z]+(case-sensitive) and confirmed only the mixed-case theory case (@onBlur=...) reddens while the other four stay green — it isn't a tautological test that would pass regardless of what the pattern does.The wording holds up. No residual completeness claim survives — "best-effort tripwire," "unbounded evasion surface," "not an exhaustive account," "no obvious accident found, nothing stronger" are all present and consistent across the three locations, and nothing elsewhere in the ADR or doc comments quietly asserts the check is reliable against a determined evasion. It doesn't underclaim either: it says plainly what class of mistake it does catch (the accidental
@onblur="Validate"shape) rather than leaving a reader to assume the whole mechanism is worthless once "unbounded" is said. Good calibration.Not blocking, noted per your framing:
@bind:after="Handler"— Blazor's own idiomatic hook for running code after a two-way-bound value changes, not a contrived adversarial spelling — also compiles and evades silently (confirmed). It's arguably a sharper example of the accidental case than the three currently listed (a developer reaching for@bind:afterto validate live is a natural thing to write, not an edge case), but per your instruction this is recorded as further confirmation of the unbounded surface, not a defect to fix.Three statements consistent, no drift: ADR-0066's Consequences (confirmed under that heading), the test class's XML doc, and the
FieldErrorsdoc comment (which continues to delegate to the test's remarks rather than restating them, and its own line now matches the new framing) all say the same thing in the same terms.