Repository navigation
Fixes #3045 - #3054
Merged
Merged
Fixes #3045#3054
Conversation
…3045) The AG rollup's three tiles built the number and its label as sibling divs with nothing tying them together, the defect #3038 fixed on the fleet page. The label now carries a stable id and the number describes itself with it. rollupTextId moves from fleet.js to util.js rather than being copied: the de-duplication rule it carries is subtle (a duplicate id resolves to the first match, mis-associating silently), AG's labels are all distinct today, and a local copy would be written without the guard and stay correct until a fourth tile repeated a label. The guard derives its population from the shipped modules — every rollup figure under wwwroot/js must carry a description, with a floor under the enumeration — because a positive-only check is what let this page survive #3031.
|
Reviewed. No correctness, security, or parity issues found.
The new No blocking issues; nothing else to flag. |
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
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.
Fixes #3045.
rollup()inwwwroot/js/pages/ag.jsbuilt each of its three tiles as a number div and a label div with nothing tying them together. The text was in the accessibility tree, in the correct reading order, and the meaning was carried by text rather than colour — nothing was unavailable. What was missing is the programmatic relationship, the same thing #3038 fixed on the fleet page and deliberately left here because the two pages share no code path.The label now carries a stable label-derived id (
rollup-groups-lbl,rollup-reporting-servers-lbl,rollup-views-lbl) and the number points at it witharia-describedby. Wired once in the tile closure, at no call site, so a tile cannot be added half-wired. Nothing is added to, removed from or reordered in the rendered text; no stylesheet in this wwwroot keys on an id or on anaria-*attribute (the only id selectors are the shell's six:app,sidebar,main,statusbar,server-list,view-list), so nothing moves visually and nogetElementByIdlookup is shadowed.The decision this carried: hoist, not duplicate
#3031's closing note said "the
util.jshoist is the shape", and #3045 corrects it — there was no hoist.rollupTextIdand the wired closure both lived infleet.js, andutil.jsexposed no tile or id helper, so creating the shared helper was the decision.rollupTextIdmoves toutil.jsverbatim (byte-identical function body; the removal fromfleet.jsand the insertion intoutil.jsmatched exactly), and both pages import it. The reason is the de-duplication rule it carries, which is the subtle part: two tiles sharing a label emit a duplicate id, andaria-describedbyresolves a duplicate to the first match — a silent mis-association rather than a visible break. AG's three labels are all distinct today, so theusedset is inert on this page — which is exactly why a local copy would have been written without it, and would have stayed correct right up until someone added a fourth tile that repeated a label. Eight lines with no page-specific knowledge is a poor trade for a second copy of a rule that fails quietly.The alternative — duplicating the derivation in
ag.js— is cheaper to review in isolation and leaves the pages free to diverge. Nothing here wants to diverge: the two rollups differ in what they qualify, not in how an id is spelled.What did not come across from the model
The sub-line machinery.
fleet.js's tile takessubandtitlebecause that rollup carries a coverage sub-line on one tile and a long coverage note; six of its seven tiles have no sub-line either. The AG rollup has neither, so a second id would point at an element that does not exist.subIdis absent, and its absence is pinned rather than left to the next reader's restraint.Why the label rides in
aria-describedby.numis a plaindiv. ARIA prohibits namingrole=generic, soaria-label/aria-labelledbyset there is invalid and may be dropped; a description is a supported property on it. Description is the carrier that works without inventing a widget role for a static figure. Page-independent, taken from #3038 rather than re-derived.The guard fails on an unwired tile, not merely passes on a wired one
Darling/Darling.Tests/RollupTileDescriptionTests.cs, six tests. A positive-only check is why this page survived #3031 — asserting that the fleet tiles carry a description would have stayed green through the entire life of this defect. So the population is derived from the shipped source: the modules under test are found by scanningwwwroot/jsfor rollup figures, every figure found must carry the relationship, and a page that grows a rollup tomorrow is swept in with no edit to the test. Every enumeration has a floor, because an empty match is the one way a check like this reads as clean while proving nothing.The figure regex is scoped to
el("div", …), socompose.js's numerictd.numgrid cell — a table cell whose meaning comes from its column header, not a sibling label — is not swept in.Nine mutations, each proven to have landed (the driver asserts its anchor is present, and refuses to run if not), each failing a different named assertion:
aria-describedbyremoved from the AG figureEveryRollupFigure_CarriesADescription,TheAgNumbers_DescribeThemselvesWithTheirLabelsIdEveryRollupPage_BuildsEveryTileFromOneClosureonly — the sweep passes it, which is why the one-closure pin existsTheSharedDerivation_DeDuplicatesWithinARenderEveryIdDerivationCall_IsHandedTheRendersUsedIdSetag.js(the rejected option)TheIdDerivation_ExistsExactlyOnce_AndEveryFigurePageImportsItfleet.js's sub-line machinery copied over by resemblanceTheAgNumbers_DescribeThemselvesWithTheirLabelsIdag.jsrestored to whatdevships — the defect exactly as reported.numrenamed on both pages, so the enumeration matches nothingexpected at least 2 rollup figures under wwwroot/js, found 0Mutations ran against a copy of the tree in a sandbox, after this commit, so a restore could not eat uncommitted work. The test source itself was never mutated.
Darling.Testscannot execute on macOS, so the six tests were run by compiling this exact file into a throwaway net10.0 xunit v3 host staged outside the tree:Total: 6, Errors: 0, Failed: 0, Skipped: 0, Not Run: 0. The realDarling/Darling.Tests/Darling.Tests.csprojbuilds clean with the file in place (-p:EnableWindowsTargeting=true, 0 errors, 0 warnings attributable to it), re-checked after mergingorigin/dev.Behaviour, under a DOM shim
The source pins cannot see what the module builds, so the shipped
ag.jsandutil.jswere run under a minimal DOM shim in node. The only change toag.jswasexporton the module-privatefunction rollup(d);util.jsis byte-identical, andpanels.jsis a stub (rollup()never touchesVIZ).Each of the three figures is described by its own label, every reference resolves to an element in the built tree, no duplicate ids, no name set on any generic div, no
titleanywhere on the tile, and the ids are identical across a re-render (the page re-renders on a 60s cadence).The duplicate-label hazard was demonstrated rather than asserted, with the control #3038 ran:
Groups→rollup-groups-lbl,Reporting servers→rollup-reporting-servers-lbl,Views→rollup-views-lblrollup-groups-lbl-2and resolves to its own label — greenrollup-groups-lbl; the third figure's description resolves to the first match, so the number reading9is described by the label belonging to the tile reading4— redNot verified
/api/agwas never called —rollup()was handed a payload object directly..numis a non-focusable generic element and screen readers vary in whether they announce a description on one at all. This establishes the programmatic relationship 1.3.1 asks for; it does not promise a changed announcement.ariaDescribedByElements; this run indexes the built tree first-match-wins in the shim. The resolution rule is modelled, not observed.distinct_ag_count,reporting_server_count,availability_group_count) are unchanged by this PR and were not re-verified againstDarlingAgReader.CHANGELOG entry, for whoever owns the
[Unreleased]blockCHANGELOG.mdis untouched here — every lane appends to the same block. Under### Fixed:Needs a link definition:
[#3045]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/3045