fix(a11y): composite alpha when measuring contrast ratios - #122
Conversation
|
@KhyFee is attempting to deploy a commit to the PhilaCon Valley Team on Vercel. A member of the Team first needs to authorize it. |
traksaw
left a comment
There was a problem hiding this comment.
The compositing fix itself is right: f.a * v + (1 - f.a) * b.rgb[i] is proper source-over, the ~2.62 expectation computes to 2.6179 (comfortably inside toBeCloseTo(2.62, 1)), and none of the three existing product assertions change — hero-cta, event-bar-cta, and the nav pill are all opaque text-brand-dark on bg-brand-pink/bg-accent-400, so the suite stays green on that front.
Requesting changes on one blocker: the branch fails npm run format:check, which .github/workflows/ci.yml:37 runs as a required step, so this lands red as-is.
Beyond that, the comments below are mostly about the helper reporting a passing ratio for a pairing that genuinely fails. I know the PR body defers semi-transparent backgrounds to a follow-up, and that's a fair scope call — my ask is only that the deferral be loud (throw) rather than silent (green check), so we don't bank confidence on measurements the helper can't actually make yet.
One more, outside the diff — effectiveColours, line 67
GitHub won't let me anchor an inline comment there since it's untouched by this PR, so it goes here.
isTransparent only rejects alpha === 0, so the walk-up stops at the first partially transparent ancestor and hands its raw RGB back as both the composite base and the background luminance. Concrete case: dark text over a bg-white/10 veil on the dark page measures 17.40:1 — passes AA comfortably — when the real composited ratio is 1.34:1.
This isn't hypothetical for the current DOM: src/components/Header.astro:168 uses bg-brand-dark/90.
Deferring full background compositing is fine, but as written the helper manufactures a passing number instead of declining to measure. Minimum viable version — keep climbing when the background isn't opaque, or throw:
const a = alpha && alpha.length === 4 ? Number(alpha[3]) : 1;
if (a > 0 && a < 1) {
throw new Error(`translucent background ${bg} on <${node.tagName}> — not yet supported`);
}|
Addressed review notes on
Ready for another look when you have a moment — thank you for the careful review. |
traksaw
left a comment
There was a problem hiding this comment.
Thanks for taking #117 — the core compositing fix is right. I ran the new helper standalone and both new assertions hold (6.396487 vs toBeCloseTo(6.3965, 3), and 2.61796 vs ~2.62), and the opaque path is numerically identical to the old parseRgb, so there's no regression on the existing tests.
Two things to fix before merge — one real, one latent. Details inline.
| const f = parseRgba(fg); | ||
| const b = parseRgba(bg); | ||
| const composited = f.rgb.map((v, i) => f.a * v + (1 - f.a) * b.rgb[i]); | ||
| const [hi, lo] = [relativeLuminance(composited), relativeLuminance(b.rgb)].sort((a, b) => b - a); |
There was a problem hiding this comment.
The background's alpha is parsed and then discarded, so a transparent background scores a false pass.
parseRgba now returns b.a, but the foreground is composited over b.rgb and the background is measured with relativeLuminance(b.rgb) — the raw channels, as if opaque. Two concrete failures:
- The nav-pill test below bypasses
effectiveColoursand feedsgetComputedStyle(pill).backgroundColorstraight in. The pill carriesx-cloakand its background is Alpine-driven; if it ever computes torgba(0, 0, 0, 0)(element swapped, class renamed, Alpine not initialized), this returns exactly 21:1 — the a11y gate passes against nothing, which is precisely the failure mode theeffectiveColoursdoc comment says it exists to prevent. - The inverse also happens: dark text on a translucent panel like
bg-brand-dark/5(rgba(26, 26, 26, 0.05)over white — real usage on the mobile menu button atsrc/components/Header.astro:135) measures 1.0:1 instead of ~15:1, a false failure.
Since the alpha is already in hand, either composite the background over an assumed page white before measuring it, or throw when b.a < 1 — anything but ignoring it.
There was a problem hiding this comment.
Thanks — translucent backgrounds now composite over white before luminance; a === 0 throws so the nav-pill path cannot false-pass.
| function parseRgb(value: string): number[] { | ||
| const nums = value.match(/[\d.]+/g); | ||
| function parseRgba(value: string): { rgb: number[]; a: number } { | ||
| const nums = value.match(/[\d.]+%?/g); |
There was a problem hiding this comment.
The new % support applies to the colour channels too, not just alpha, and misparses them silently.
parseRgba('rgb(100%, 40%, 66%)') returns rgb: [1, 0.4, 0.66], which relativeLuminance then divides by 255 — a bright pink is measured as near-black and the ratio is garbage, with no error raised. The same silent-nonsense path swallows any non-legacy serialization: color(srgb 1 0.4 0.66) → [1, 0.4, 0.66], oklch(0.7 0.15 20) → [0.7, 0.15, 20].
This is latent today (Tailwind 3.4 emits legacy rgb()/rgba() and Chromium serializes computed colours that way), but it's exactly the "modern syntax" the guard on line 37 claims to cover — and that guard never actually fires for 85%, since the map above already converted it to 0.85. Rejecting a % on any of the first three components, or a function name other than rgb/rgba, would make the failure loud instead of a wrong number.
There was a problem hiding this comment.
Thanks — latest commit rejects % on RGB channels and non-rgb/rgba functions, while still converting percentage alpha.
|
Thanks @traksaw — appreciate the careful follow-up on the helper. Addressed both open points in the latest commit:
Added unit coverage for those cases. Ready for another look when you have a moment. |
| if (b.a === 0) { | ||
| throw new Error(`transparent background "${bg}"`); | ||
| } | ||
| const bgRgb = b.a < 1 ? b.rgb.map((v) => b.a * v + (1 - b.a) * 255) : b.rgb; |
There was a problem hiding this comment.
contrastRatio always composites a translucent background over pure white (line 70), but effectiveColours's DOM walk-up (lines 81-94) only continues past a background when alpha is exactly 0. A background with alpha between 0 and 1, e.g. rgba(20,20,20,0.9), is accepted immediately and never resolved against its real ancestor backdrop. Example: a CTA with background: rgba(20,20,20,0.9) on a dark-navy section — effectiveColours returns that value right away, then contrastRatio blends it with white instead of navy, which can report a wrong AA pass or fail. The PR description flags this as deferred follow-up work, so this is a known scope cut, not an oversight — flagging in case you want it closed before merge.
There was a problem hiding this comment.
Thanks — latest commit composites translucent ancestor layers instead of treating 0 < a < 1 as opaque and then blending that over white.
|
Thanks @traksaw — good catch, and you’re right it was a known cut. Closed it in the latest commit: Happy to tweak the flatten if you want it to stop only at |
|
Composite alpha fix is in per last review — standing by if anything else should move to a follow-up PR vs this one. |
Summary
contrastRatiocomposites translucent foreground over background via alpha before luminance (fixes Contrast test helper drops the alpha channel, so translucent text is scored as opaque #117)text-white/85~2.62 caseTest plan
npx playwright test e2e/contrast.spec.ts