Repository navigation
Fix/ppi widget accessibility - #508
Open
ReneeSleepy wants to merge 2076 commits into
Open
ReneeSleepy wants to merge 2076 commits into
ReneeSleepy wants to merge 2076 commits into
Conversation
Rebases onto the closure merge of PR #105 and fixes four things review found, plus three more the re-audit turned up. The audit was taken on efda579 at 09:24. The closure branch landed at 22:00 the same day and moved nine of the counts, so every Appendix B command was re-run and every number re-derived on the new base. The loose hex literals under src/app went 105 to 226, which matters because section 10 quotes that figure as the migration baseline for another card. Two files new to this base carry 94 of the 226 between them. What did not move is recorded too: the three palette files, all 15 status colours and every contrast ratio in sections 7, 8 and appendix A were re-checked and are unchanged, so the token tables stand as written. Section 15 claimed the theme is not sent to Google Analytics and cited src/index.html:6-14. There is no Google Analytics in this app. No GA, no tag manager, no analytics package in package.json, and those lines are the Google Fonts stylesheet links. Replaced with the greps that show the absence and with Sentry, which is real and is the reporter the bullet should have been about. Section 12 cited src/index.html:34 for the theme-color meta. It is at line 24; line 34 is a respond.js shim behind an IE conditional comment. Section 6.2 rule 3 left the case that decides the whole feature undefined: a valid local preference, no updatedAt key, and an account field that has never been written. That is not an edge case, it is day one of THM-B01 for every existing user, because phase one never writes the timestamp key. Read naively the rule discarded the local value and reset everyone to system. Now settled as a four-row table where presence beats recency and timestamps are only consulted when both sides hold a value, with the two intended consequences spelled out and a test for each row added to section 14. The Angular Material line citation for rejecting option B could not be checked because node_modules is not installed here, so the line numbers are dropped rather than left unverifiable. The argument does not depend on them. Section 2.3's palette reference is marked the same way, with the four flattened values shown to be reproducible without it. Three more from the re-audit. The burndown chart palette moved and its fifth slot changed from transparent to a real colour. The localStorage key convention moved and now has a second call site. And the focus counts read level at 11 against 11 where they were 11 against 7, which looks like an improvement and is not: only one of the ten files that suppress an outline declares a focus-visible rule, so nine still have no replacement ring. Appendix B now carries the commands for the section 2.4 palette split and remainder, the focus-set overlap, the custom-property count, the template file count and the analytics check, so every number in the document has a command beside it. Each one was run and matches.
(cherry picked from commit e1abb68)
Improved clarity in the user guide. (cherry picked from commit 67e732c)
Seven templates gated an action or an explanation on (mouseover) and (mouseout) alone, so a keyboard user could never reach what the pointer revealed. mouse-events-have-key-events moves from off to error, which catches the bare case where a hover handler has no keyboard counterpart at all. That rule is a floor and not a guarantee, so it is worth being precise about what it does and does not stop. Its selector is Element:has(BoundEvent[mouseover]):not(:has(BoundEvent[focus])), and :has matches descendants, so an element can keep (mouseover) and (mouseout) with no keyboard equivalent of its own and still lint clean off a nested control's (focus). It also never checks that whatever takes focus is reachable, so a div carrying (focus) and no tabindex passes. The rule stops the pattern being reintroduced bare. The tests added here are what hold these particular fixes in place. Where the reveal hid a control, it is done with opacity now rather than display or [hidden]. Both of those take the control out of the tab order, so :focus-within can never become true and the focus path is shut before it starts. staff-notes, tutor-notes and task-comments-viewer show their actions on :hover or :focus-within of the surrounding card, and those actions are real buttons instead of mat-icon elements carrying a click handler, so there is something in the tab order for :focus-within to answer to. The staff task inbox keeps its options button in the tab order, stacked over the pin indicator and revealed on row hover or on its own focus. Pointer and keyboard hold the row open independently, so tabbing off the button no longer fades it out from under a pointer still on the row. The row div is one of the cases described above: it keeps (mouseover) and (mouseout) and depends on that nested options button for its keyboard path, which is a real path rather than a lint technicality. The pin beside that button is no longer a button itself. It reports whether a task is pinned and has no action, so it was an unlabelled tab stop on every row and empty on the rows that were not pinned. Worse, it was hidden whenever the adjacent options button took focus, so a forward tab landed on it while a shift-tab skipped it and the row's tab order read differently in each direction. It is a div with role="img" and a label now, and it is out of the tab order in both directions. The planner's gantt bar carries role="button" and tabindex="0", so the (focus) binding on it can actually fire, and Enter and Space open the prerequisites modal the way a click does. Its focus outline is drawn in currentColor, which is already picked per bar to contrast with that bar's own background. The planner spec blanks the component template, so those attributes were previously unguarded by any test. It now holds the #bar TemplateRef and stamps that template directly, because ngx-gantt sizes the chart from real layout and draws no bars under jsdom. Deleting role, tabindex or either keydown binding fails those tests. tutor-discussion wrote task.hover from its mouse handlers and fed it to an [ngClass] with no matching .hover rule in that component's stylesheet, so the path was a no-op before and after. It is removed rather than left looking like it does something, and allowHover goes with it since nothing there ever set it false. hoveredCommentId in the engagement dialog was likewise only ever written and never read. Three rule ignores are left in the config, covering keyboard activation and focus for click handlers elsewhere in the app. Clearing them is A11Y-F03 and A11Y-F04.
Two internal inconsistencies found while re-verifying the audit against 11.0.x at 4034e7d. Appendix B said the closure merge moved "the four tree-wide counts" and "all three src/app figures". The total of nine is right but the split was backwards, and it contradicted the sentence directly after it, which correctly lists !important among the counts that did not move. Only three of the four tree-wide counts moved: the hex total 350 to 471, the rgba/hsla file count 20 to 25, and the stylesheet count 180 to 189. !important is 63 on both bases. All four src/app figures moved, not three. Re-derived by extracting efda579 and running each command against both trees. Section 3 option A described 233 as "loose component hexes". 233 is every literal outside the three palette files, which is 226 under src/app plus 7 in shared partials and styles.scss. Section 2.4 and section 10 both use 226 for the component figure, so calling 233 a component count read as a contradiction of the migration baseline. The number is unchanged, the label now says what it counts.
Pin the Node slim image and upgrade npm so bundled fixable vulnerabilities are removed.
Pin the Node builder and upgrade only the Alpine OpenSSL runtime packages to their fixed release.
Document that deploy.Dockerfile produces the shipped Nginx runtime and must be the release scan target.
Verify the pinned Node/npm versions and the narrow Alpine OpenSSL runtime upgrade.
The unit screens read their unit from a route snapshot taken the first time the component was created, so switching units from inside one of them left the header showing the new unit and the screen underneath showing the old one. Each screen now subscribes to the resolved unit and resets its own selection when the unit id changes. The task inbox and the staff task list also re-read their route parameters rather than keeping the first value, so a deep link or a Back press lands on the task it names. Audit ticket RTE-01.
The role whitelist guard existed and was applied to some staff routes and not others, so several screens were reachable by any signed in user and relied on the api refusing the data behind them. Every staff route now carries the guard, and the guard has a spec so a route added without one is caught. Audit ticket SEC-04.
…out asking A task created with Add Task exists only in selectedTaskDefinition until it is saved. It is not in the task definition cache, so it is not in the list on the left and nothing on screen marks it as pending. Three handlers cleared that field with no prompt: the search box on every keystroke, clicking another task, and a second click on Add Task. Any of them threw the whole thing away, with no undo and nothing saying it had happened. The predicate tests isNew first, and that is the part that matters. TaskDefinition.hasChanges returns false when there is no originalSaveData, and only selectTaskDefinition and a successful save ever set that, so a brand new task with every field filled in reports no changes at all. A guard built on hasChanges alone would protect the case that was already safe and leave this one untouched. The two deliberate exits ask before discarding. The search box does not: it is [(ngModel)] bound and the handler runs from ngModelChange, so the character is already typed by the time the handler sees it, and cancelling would mean writing the text back one keystroke at a time while the convenor answered a modal per letter. It keeps the editor open and filters the list underneath instead, which is the better behaviour regardless. Clicking the task that is already open still returns early, so nobody is asked to discard their work for clicking where they already are. Not done here, on purpose: putting the unsaved task into unit.taskDefinitionCache so it shows in the list. EntityCache stores under the entity key, which is the id, and that is undefined until the task is saved, so it would cache under undefined and then be added again under the real id after save. The unit would carry a ghost task definition for the rest of the session, including in guessTaskAbbreviation, which names the next task from the last element of that array. Showing pending state in the list needs a key strategy and its own ticket. Audit ticket STAFF-05.
Two reads of the same route parameter, both from a snapshot, in two components. The child task list already follows route.paramMap, so this closes the other two halves. The parent read the abbreviation once in its constructor and wrote the result into the shared subject. Angular reuses the component when only a route parameter changes, so that ran once for the life of the screen and fought the child's own reactive read. It is deleted rather than made reactive, because the child is the one component shared by all three routes that carry a :taskAbbreviation, and the project dashboard and the portfolio progress view render it with no such parent at all. The selection is also re-applied from ngOnChanges. The unit resolves progressively, so on a hard refresh of a deep link the parameter arrives while taskDefinitions is still empty, and nothing put it back once the list turned up. An empty list is now treated as not loaded yet rather than as a missing task, so the selection waits instead of being cleared. A loaded list that does not contain the abbreviation still clears it, which the existing spec covers. The comparison against the current value stays. Selecting a task navigates, that navigation makes paramMap emit, and the emission comes straight back in, so the comparison is the only thing stopping a click from looping. It also collapses the duplicate write from the second rendered instance of the child in the parent template. Audit ticket STAFF-07.
extractErrorMessage read error.error.error before checking what error.error was,
and Angular sets that to null whenever the response body is empty. So a 502 from
a proxy while the api restarts, or any 500 that returns nothing, threw a
TypeError inside the catchError callback. That TypeError became the value passed
to the subscriber, and subscribers hand it straight to a snackbar, so the person
was told "Cannot read properties of null" and nothing about the request that
failed.
The same branch also mishandled an unparseable body. Angular delivers that as
{error: SyntaxError, text: '<the raw body>'}, which made the property read
truthy and assigned a SyntaxError to a variable typed as a string. An HTML error
page produced a snackbar about an unexpected token. The nested value now has to
be a string before it is used as one, so that case falls back to the status text.
The Blob check moves above the property read. It only worked where it was
because a Blob happens to have no error property.
Sentry reporting is the second half. It threw an HttpRequestError inside a
startSpan callback, which rethrows, inside a then() with no catch, so every
failed request in the app produced an unhandled promise rejection that nothing in
the observable chain could see. It is now a captureException. That is what makes
the file testable at all, which is why it is here rather than in its own ticket.
diagnoseSdkConnectivity is dropped: it answers a start up question, not a per
request one, and its promise is what turned a report into a rejection.
The spec constructs the interceptor directly with two stubs, so it needs no
TestBed and no module mocking.
Not in scope and left alone: the refresh token flow above, and the call site in
campus-list.component.ts that reads response.error?.error as though this method
returned an HttpErrorResponse. Changing the return type to suit it would break
every other subscriber at once.
Audit ticket WEBUX-14.
…hem on sign out An unsent comment was written to localStorage under a key that named the task and nobody else, and sign out did not remove it. The app signs out by routing rather than reloading, so sessionStorage survived it too. On a shared machine in a lab or a staff room, the next person to sign in and open the same task was handed the previous person's unsent words, in the box, ready to send under their own name. The key now carries the user id, as uid<id>, and so does the submitted-task set. The marker matters: a legacy key is either task_comment_draft_<taskId> or task_comment_draft_<projectId>_<definitionId>, so a bare number would make task_comment_draft_5_7 mean both "user 5, task 7" and "project 5, definition 7". With the marker the two shapes cannot be confused, which is what lets sign out clear the old ones safely. The id and not the username or the email. Ids are stable, usernames are not, and an email in a storage key is personal data sitting in plain sight in dev tools on a shared machine. Nothing is read or written when there is no signed in user. currentUser is the anonymous user during sign out and the composer can still be torn down then. Sign out now clears the departing user's drafts, before currentUser is replaced, and sweeps the unscoped keys left by earlier versions, which belong to nobody and would otherwise sit in storage for good. Keys are collected and then deleted, because removeItem inside a forward loop over localStorage.key(i) shifts the indices along and skips every second match. Server side drafts were considered and deferred. They need an endpoint, a migration and a decision about how long an unsent draft is kept and who can read it. This closes the leak with what is already here. Audit ticket WEB-27.
…accordion-20260828 feat(cpd): add stacked phone dashboard
…uide-handover-11.0.x-20260827 docs(cpd): publish dashboard user guide and contributor handover
…e-vulnerability-remediation-11.0.x-20260827 fix(security): harden Web container images
…k-mobile-view-20260828 fix(dashboard): make task feedback usable on phones
…ract docs(theme): define the light, dark and system theme contract
fix(a11y): two empty states are a single aria-hidden icon with no text
…ailwind refactor: replace ng-flex-layout with Tailwind in task-viewer-state
First slice of THM-M03 (tutor/unit-chair/admin workflow theming), covering checklist item 2: the task inbox / marking queue tutors use to review and assess submissions (inbox.component, staff-task-list.component, moderation.component, inbox-dashboard.component). Migrates every hardcoded colour in these four components onto the THM-M01 token layer (--ot-color-*), following the exact pattern THM-M01 established in header.component.scss: var(--ot-color-X, <original-light-value>), so light mode is pixel-identical and only dark mode changes. One exception needed a real override rather than a token swap: staff-task-list's --background-gray (a hover/selected row tint) is an rgba(black) value that reads correctly in light but would be nearly invisible or wrong-direction on a dark surface, so it gets a :host-context([data-ot-theme='dark']) override to an rgba(white) tint instead. Also migrates the four Tailwind arbitrary-hex classes and one inline style in inbox-dashboard's empty states (the icons/text shown when no task is selected, or a task has no PDF/task sheet) onto the same tokens via Tailwind's var() arbitrary-value syntax. Adds a focused spec for InboxDashboardComponent (previously untested) that asserts the empty-state icon and message carry the token class rather than a bare hex, verified to fail against the pre-migration classes and pass after. Checklist items 1, 3-10 (route confirmation beyond this surface, student lists, unit/task editors, staff reports, dense-table/nav/narrow-layout checks, and the remaining marking-queue sub-surfaces - task-claim, confirm-moderation-modal, batch-feedback-workflow-dialog) are follow-up work, not covered by this PR. No business, permission, or marking-transition logic changed - colour/class values only. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…M-F02) Adds a three-option, keyboard- and screen-reader-accessible mat-radio-group to the edit-profile form, next to notification and peer-progress settings. Reads and writes through the existing THM-F01 ThemeService, which already owns persistence, account sync, and storage-failure fallback, so this component only renders the current preference and forwards selections. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
DX-W11 - web CONTRIBUTING is a stub that sends contributors to a finished migration guide, with four broken links
…matrix FILE-A01: Safe upload policy and compatibility matrix
FILE-T01: Test and harden clipboard image paste
fix(a11y): the three main search fields have placeholder-only labels
…1-validation docs(a11y): add Phase 1 independent validation notes
…in-workflows feat(theme): dark mode for the task inbox marking queue (THM-M03, 1/N)
…e-control feat(profile): add the Light, Dark, and System appearance control (TH…
…display-name feat(users): migrate priority surfaces to shared display name
The empty state was reachable, so removing it left users with no units on a blank home screen. Put it back behind a loaded flag instead. Both lists start as empty arrays before the first load finishes, which is what made the message flash for every user on sign in. - notEnrolled is true only once GlobalStateService.onLoad has fired and both the student projects and unit roles are empty. - Spec covers loading, no units, a student project and a staff unit role.
fix(web): the 'not enrolled' message on the home screen can never appear
ReneeSleepy
force-pushed
the
fix/ppi-widget-accessibility
branch
from
September 20, 2026 04:18
c8d2e9e to
e78e2fe
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR applies targeted accessibility improvements to the task-sheet PPI widget.
The changes:
Add meaningful accessible text for peer submission progress.
Ensure the comparison is not communicated by colour alone.
Hide decorative icons from screen readers.
Keep the changes focused on the existing PPI widget.
Fixes # (issue)
Type of change
How Has This Been Tested?
I ran the Angular test suite using:
npm.cmd test -- --watch=falseThe results were:
The Angular application also compiled successfully.
Manual browser verification could not be completed because the local OnTrack login page currently shows "Temporarily unavailable".
Testing Checklist:
Checklist: