Repository navigation
Conversation
Axis titles had no fill and drew black on dark cards. Move chart chrome colours to a global partial keyed to the theme tokens, re-resolve burndown series colours when the theme flips, and skip chart animation under prefers-reduced-motion.
…l border token Light success, warning, error, info and urgency text sat at 4.3:1 on the page. Dark success, error, urgency and chart axis text fell under 4.5:1 on raised cards. Text fields, checkboxes and radios now use a control border that clears 3:1 on every surface.
The effect needed an injection context that the component spec does not provide. Compare the resolved theme in ngDoCheck instead.
Fails if a text token drops under 4.5:1 or a control, focus or status mark token drops under 3:1 on the page and card surfaces.
White on the dark success green was 2.3:1 and gray-600 on the dark dialog was 1.5:1.
Admin list add, save, cancel and delete buttons, header menu and jobs buttons, the home logo link, the Gravatar and student dashboard avatar links, the test attempt menu and the create portfolio link had no name for screen readers.
|
Reviewed the main behavioural parts of this accessibility slice, particularly the theme contrast guard, token changes, global ngx-charts theming, burndown theme refresh and reduced-motion handling. The contrast spec reads the actual light/dark token files and fails on missing or unparseable values, which gives useful regression protection instead of only checking hardcoded examples. The chart chrome is moved onto theme tokens and the burndown series colours are re-resolved when the active theme changes. The accessible-name additions are also consistent with the controls they describe. Node/Test/Lint CI are green. I did not find a blocking issue in the areas reviewed. The stacked dependency and the separate menuitemcheckbox redesign noted in the PR should still be respected. |
|
Closing: this work is now on |
2 participants
What this does
Raises a set of
--ot-*theme tokens past the WCAG 2.2 AA contrast thresholds, adds a control-border token for input, checkbox and radio outlines, gives accessible names to icon-only controls in twelve files, sizes the comment actions to a 24px target, and moves ngx-charts axis, tick, gridline, legend and gauge colours onto the theme tokens so they follow the dark theme.Stacked PR
Based on
theme/status-and-globals. The diff shown here is only this slice's incremental change against that base, and it must not be merged before its base does.The stack is a graph, not a chain.
theme/status-and-globalscarries commitcc4a834e6("Restyle extension request card and add full-screen comments to the task inbox") directly on top of its head. Two branches fork fromcc4a834e6: this one andui/panel-layout. Both are already merged in belowui/notification-links, which contains3c19fb377("Merge branch 'ui/a11y-audit' into ui/inbox-extension-card") and7972ea62a("Merge branch 'ui/panel-layout' into ui/inbox-extension-card"), andui/inbox-extension-cardsits aboveui/notification-links. Soui/panel-layoutis a sibling of this branch at the fork point only. Downstream it is an ancestor of bothui/notification-linksandui/inbox-extension-card.One thing worth flagging. Because this diff is taken against
theme/status-and-globalsrather than againstcc4a834e6, the six files that commit touched (extension-comment.*,inbox.component.*) show up here too. 25 of the 31 files come from the accessibility commits and 6 come fromcc4a834e6.ui/panel-layoutforks from the same commit, so the same six files appear in its diff against the same base. Whichever of the two merges first absorbs that commit and the other's diff against the new base drops it. Nothing to do about it, just do not review the same extension-card and full-screen-chat change twice.What changed and why
Colour contrast (WCAG 1.4.3 / 1.4.11)
Light
--ot-color-success,-warning,-error,-info,--ot-urgency-overdueand--ot-urgency-soonsat between 4.28:1 and 4.31:1 against--ot-color-page(#f3f4f6). The new values sit between 4.61:1 and 4.65:1. Dark--ot-color-success(4.38:1),--ot-color-error(4.41:1),--ot-urgency-overdue(3.32:1),--ot-urgency-later(3.61:1) and--ot-chart-axis(3.61:1) were all under 4.5:1 against--ot-color-surface-raised(#353c47) and now clear it.--ot-chart-gridis lightened in dark as well, as a gridline rather than as text.A new
--ot-color-control-bordertoken (#8a8a8alight,#7b8899dark) is added to both token files and pointed at--mat-form-field-outlined-outline-color,--mat-checkbox-unselected-icon-colorand--mat-radio-unselected-icon-colorinstyles.scss. Material's default for those is 38% black, which is 2.68:1 on a white card. The new token clears 3:1 on page, surface and raised surface in both themes.One dark status mark token moves with them.
--ot-status-not-started-graphicgoes from #7d8590 to #848c97, which takes it from 2.98:1 to 3.27:1 against the dark raised card, so it clears the 3:1 non-text minimum. The other graphic tokens are unchanged in both themes.Contrast guard
theme-contrast.spec.tsis new. It reads_light.scssand_dark.scssoff disk at run time, parses the--ot-*declarations, resolves single-levelvar()references, and builds one case per token pair. The cases are: eleven named text tokens against the three surface tokens at 4.5:1,--ot-color-control-borderand--ot-color-focusagainst the same three surfaces at 3:1, every--ot-status-*-ontoken against its own fill at 4.5:1, every--ot-status-*-graphictoken against--ot-color-surfaceand--ot-color-surface-raisedat 3:1, and six hand-listed pairs such as--ot-color-on-erroron--ot-color-error. That is 180 cases across the two themes. A token edit that drops any of those pairs under its threshold fails the spec.Accessible names on icon-only controls (WCAG 4.1.2)
Eleven files gain
aria-labels on buttons and links that carried only amat-icon: the admin add, save, cancel and delete actions (activity types, campuses, overseer images, teaching breaks), the overseer script editor's save button, the Gravatar link, the feedback template CSV upload and download buttons, the header's background-jobs button and administration menu, the student dashboard avatar link inuser-badge, the test attempt menu inscorm-comment, and the create-portfolio link inunit-task-list. A twelfth file,task-status-card, gives the task statusmat-selectaria-label="Task status", since it had no name at all.The header logo link also moves its name off the icon. The
mat-iconhadaria-label="Formatif logo"and the enclosing<a routerLink="/home">had none, so the name sat on a child of the thing a screen reader user activates. Now the link carriesaria-label="OnTrack home"and the icon carries nothing.24px targets for comment actions (WCAG 2.5.8)
.bubble-actionincomment-bubble-action.component.scssgetsdisplay: inline-flex, centred alignment and a 24px minimum width and height, so the existing 16px glyph sits centred in a 24px square. Themargin-left: 0.3emthat used to space the icons is dropped.Dark-mode chart theming
ngx-charts has no inputs for axis, tick, gridline or legend colour, and draws axis titles and tick labels as SVG
<text>with no fill, which falls back to black on a dark card.src/styles/common/charts.scssis a new global partial, imported fromstyles.scss, that points axis text, ticks, the domain line, gridlines, reference lines, pie labels, legend text and gauge text and arcs at the theme tokens. It replaces two per-component::ng-deepblocks: the one inprogress-burndown-chart.component.scssis removed, andtask-status-pie-chart.component.scssis deleted outright with itsstyleUrlsentry.The burndown chart's line-series colours are resolved in JS from tokens and do not repaint on their own when the theme flips. The component now compares the resolved theme in
ngDoCheckand re-applies, because the effect-based approach needed an injection context the component spec does not provide. The same component also setsanimationsfrom aprefers-reduced-motionmedia query and binds it on the chart, since ngx-charts animates in JS where the global reduced-motion CSS cannot reach it. No other chart component is touched for animation.Two style-token swaps folded in
The download-filter dialog's hint text moves off
text-gray-600ontotext-ot-muted. The knowledge check card headers move offbg-ot-success/text-whiteontobg-(--ot-status-complete)/text-(--ot-status-complete-on).Deliberately not changed
charts.scsscovers chart chrome only, which is axis, ticks, gridlines, reference lines, pie labels, legend and gauge. Series colours stay resolved in each chart component's own TypeScript, per the comment at the top of the file. Only the burndown chart's re-resolution is changed here. The other chart components were not audited for the same staleness in this slice.-graphictokens keep their existing values. They already clear 3:1 against both card surfaces, so only the darknot-startedmark needed raising.Testing
This slice adds one spec file,
src/app/common/theme/theme-contrast.spec.ts, and touches no other spec. It has a static sanity test (contrastRatioagainst black on white and #767676 on white, plus an assertion that more than 100 cases were generated) and a parametrizedit.eachblock that runs one assertion per case.inbox.component.spec.tsandprogress-burndown-chart.component.spec.tsalready exist in the repo and are not extended here, so the full-screen chat toggle that arrives withcc4a834e6and thengDoChecktheme re-resolution have no new dedicated coverage in this slice. Thearia-labeladditions, the 24px target rule and the two token swaps have no spec coverage either.Review guidance
Worth the most attention:
src/app/common/theme/theme-contrast.spec.tsand the two token files. Check the spec'svar()resolution and its token lists against the real cascade, not just the hex values. ThebodyTextlist is hand-written, so a text token missing from it is unguarded.--ot-chart-1through--ot-chart-6and--ot-chart-gridare not checked by design.progress-burndown-chart.component.ts. ThengDoCheckcomparison and the reduced-motion flag are the only behavioural change in the slice.ngDoCheckruns on every change detection pass, so the cost ofthis.themeColor?.resolved?.()is worth a look.styles.scssandcharts.scss. Both rely on:rootspecificity to win over Material and ngx-charts library styles that load later.inbox.component.ts,inbox.component.htmlandextension-comment.*. These arrive viacc4a834e6, not from the accessibility work. Ifui/panel-layoutis reviewed first, this half of the diff does not need reviewing twice.The rest, meaning the
aria-labeladditions across the eleven admin, header and task files, the task status select label, and the two Tailwind class swaps, is markup and CSS variable substitution with no logic change.Merge order
The base branch is
theme/status-and-globals, open as #192. Nothing here merges before it.After #192, this PR and the shared panel layout PR are independent of each other and can land in either order. Both must land before the notification-links PR, which already contains the merge commits for both.
One overlap to know about: commit
cc4a834e6(the extension request card restyle and the first full-screen comments toggle) is the fork point this branch shares with the panel layout branch, and it is not ontheme/status-and-globals. It is 6 files, +247/-68. Whichever of the two PRs merges first carries it, and the second will show it already applied. It only needs reviewing once.