Skip to content

Commit 2f93666

Browse files
Merge pull request #13 from refactorfirst/make-chart-legend-text-work-with-dark-mode
Make chart legend labels gray
2 parents 5d34658 + b20bf1a commit 2f93666

5 files changed

Lines changed: 594 additions & 4 deletions

File tree

‎AGENTS.md‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,11 @@ in `out/` is served by any static host (GitHub Pages, GitLab Pages, Bitbucket).
1818
of the breadcrumbs' left-edge alignment) — palettes selected in
1919
`app/globals.css` via `:root:has(#rf-theme-…:checked)` +
2020
`prefers-color-scheme`; an inline pre-paint script in `app/layout.jsx`
21-
only restores/persists the choice (CSP-hashed by the build). Guards:
21+
only restores/persists the choice (CSP-hashed by the build). Chart
22+
legends are canvas-drawn, so they can't follow the CSS switch:
23+
`resolveLegendTextColor`/`bindThemeChartRedraw` in `lib/report-view.js`
24+
resolve a per-theme legend color (>= 4.5:1 on each `--bg-color`) and
25+
redraw live charts on radio/`prefers-color-scheme` changes. Guards:
2226
`tests/unit/theme-css.test.js` (the two dark blocks must stay
2327
byte-identical; all hex colors live in the palette blocks; dark palette
2428
passes the same WCAG AA math as `tests/unit/css-a11y.test.js`)

‎components/report-view.jsx‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,8 @@ import {
2020
enhanceReport,
2121
bindPopupHandlers,
2222
stashStatefulDom,
23-
graftStatefulDom
23+
graftStatefulDom,
24+
destroyBubbleCharts
2425
} from '../lib/report-view';
2526
import { enhanceTables } from '../lib/table-enhancer';
2627
import { renderErrorPage, logError } from '../lib/error-handler';
@@ -101,6 +102,12 @@ export default function ReportView({
101102
const controller = new AbortController();
102103
const container = containerRef.current;
103104

105+
// Every fetch cycle replaces the report markup — loading placeholder,
106+
// then the fresh render — so release the previous cycle's Chart.js
107+
// instances before any of that happens; enhanceReport builds new charts
108+
// for the new canvases once the payload arrives.
109+
destroyBubbleCharts();
110+
104111
/** Fetches the trusted template and the selected repository report. */
105112
async function run() {
106113
setPayload(null);
@@ -141,7 +148,13 @@ export default function ReportView({
141148
}
142149

143150
run();
144-
return () => controller.abort();
151+
return () => {
152+
controller.abort();
153+
// Unmounting (or a dependency change) throws the report's canvases
154+
// away with the markup; release their charts alongside the aborted
155+
// request instead of leaking live instances on dead canvases.
156+
destroyBubbleCharts();
157+
};
145158
}, [username, repository, branch, environmentProp, platformBaseUrlProp, attempt]);
146159

147160
// Render effect: re-renders the report whenever the payload arrives or the

‎lib/report-view.js‎

Lines changed: 138 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -367,13 +367,137 @@ export function bindSectionNavLinks(root) {
367367
// Chart.js bubble charts per disharmony.
368368
// ---------------------------------------------------------------------------
369369

370+
// Legend text colors mirroring --muted-color of each palette in
371+
// app/globals.css. The chart canvas is transparent, so legend text sits on
372+
// --bg-color (#ffffff light / #10161d dark); each value keeps >= 4.5:1
373+
// contrast against its own background (WCAG 1.4.3), which no single color
374+
// can achieve on both.
375+
const CHART_LEGEND_TEXT = Object.freeze({
376+
light: '#5c6b7a', // 5.5:1 on #ffffff
377+
dark: '#9fb0c0' // 8.2:1 on #10161d
378+
});
379+
380+
/**
381+
* Returns 'light' or 'dark' for the active palette, mirroring the pure-CSS
382+
* selection in app/globals.css: the explicit radios win, the system radio
383+
* defers to prefers-color-scheme, and anything else is light.
384+
*
385+
* @returns {'light'|'dark'} Active palette name.
386+
*/
387+
export function activeChartTheme() {
388+
const doc = typeof document === 'undefined' ? null : document;
389+
if (doc) {
390+
const dark = doc.getElementById('rf-theme-dark');
391+
const light = doc.getElementById('rf-theme-light');
392+
if (dark && dark.checked) return 'dark';
393+
if (light && light.checked) return 'light';
394+
}
395+
if (typeof window !== 'undefined' && typeof window.matchMedia === 'function') {
396+
try {
397+
if (window.matchMedia('(prefers-color-scheme: dark)').matches) return 'dark';
398+
} catch {
399+
// Media query evaluation unavailable: fall through to light.
400+
}
401+
}
402+
return 'light';
403+
}
404+
405+
/**
406+
* Legend text color for the currently active theme (>= 4.5:1 against the
407+
* theme's --bg-color). Resolved at draw time so a redraw picks up theme
408+
* switches.
409+
*
410+
* @returns {string} Hex color for legend item text.
411+
*/
412+
export function resolveLegendTextColor() {
413+
return CHART_LEGEND_TEXT[activeChartTheme()] || CHART_LEGEND_TEXT.light;
414+
}
415+
416+
// Live Chart.js instances by canvas, so theme changes can redraw the
417+
// canvas-drawn legend text (the palette switch itself is pure CSS).
418+
const bubbleCharts = new Map();
419+
420+
/**
421+
* Redraws every live bubble chart so legend text re-resolves against the
422+
* newly active theme. Entries whose canvas left the DOM (report re-rendered
423+
* or torn down) are destroyed — releasing their Chart.js resources — and
424+
* pruned instead of redrawn.
425+
*
426+
* @returns {void}
427+
*/
428+
function redrawChartsForTheme() {
429+
for (const [canvas, chart] of bubbleCharts) {
430+
if (!canvas.isConnected) {
431+
if (chart && typeof chart.destroy === 'function') chart.destroy();
432+
bubbleCharts.delete(canvas);
433+
continue;
434+
}
435+
if (chart && typeof chart.update === 'function') chart.update();
436+
}
437+
}
438+
439+
/**
440+
* Destroys every live bubble chart and clears the registry. The report
441+
* component calls this before a payload change replaces the report markup
442+
* (container.innerHTML), so no Chart.js instance outlives its removed
443+
* canvas; enhanceReport then builds fresh charts for the new canvases.
444+
*
445+
* @returns {void}
446+
*/
447+
export function destroyBubbleCharts() {
448+
for (const chart of bubbleCharts.values()) {
449+
if (chart && typeof chart.destroy === 'function') chart.destroy();
450+
}
451+
bubbleCharts.clear();
452+
}
453+
454+
// The MediaQueryList for prefers-color-scheme, retained so the listener
455+
// (and the redraw callback it holds) stays reachable for the page lifetime.
456+
let systemThemeMediaQuery = null;
457+
458+
/**
459+
* Binds one delegated listener redrawing charts on theme changes: the
460+
* rf-theme radios (components/theme-toggle.jsx) for explicit switches and
461+
* prefers-color-scheme for the system radio. Idempotent per document.
462+
*
463+
* @returns {void}
464+
*/
465+
export function bindThemeChartRedraw() {
466+
const doc = typeof document === 'undefined' ? null : document;
467+
if (!doc || doc.documentElement.hasAttribute('data-rf-theme-redraw-bound')) return;
468+
doc.documentElement.setAttribute('data-rf-theme-redraw-bound', '');
469+
doc.addEventListener('change', event => {
470+
const target = event.target;
471+
if (target && target.name === 'rf-theme') redrawChartsForTheme();
472+
});
473+
if (typeof window !== 'undefined' && typeof window.matchMedia === 'function') {
474+
try {
475+
// Retain the MediaQueryList and subscribe with the best listener API
476+
// it exposes: addEventListener, or the deprecated addListener on
477+
// engines that never grew the modern one.
478+
systemThemeMediaQuery = window.matchMedia('(prefers-color-scheme: dark)');
479+
if (systemThemeMediaQuery && typeof systemThemeMediaQuery.addEventListener === 'function') {
480+
systemThemeMediaQuery.addEventListener('change', redrawChartsForTheme);
481+
} else if (systemThemeMediaQuery && typeof systemThemeMediaQuery.addListener === 'function') {
482+
systemThemeMediaQuery.addListener(redrawChartsForTheme);
483+
}
484+
} catch {
485+
// Older engines without MQL listeners: explicit radios still redraw.
486+
}
487+
}
488+
}
489+
370490
export function initBubbleChart(canvas, title, chartData) {
371491
const ChartLib = window.Chart;
372492
if (!ChartLib) {
373493
console.warn('Chart.js not loaded');
374494
return;
375495
}
376-
new ChartLib(canvas.getContext('2d'), {
496+
// Chart.js refuses a canvas that still backs a live instance; drop the
497+
// previous chart (and its registry entry) before re-initializing.
498+
const previous = bubbleCharts.get(canvas);
499+
if (previous && typeof previous.destroy === 'function') previous.destroy();
500+
const chart = new ChartLib(canvas.getContext('2d'), {
377501
type: 'bubble',
378502
data: {
379503
datasets: [{
@@ -396,11 +520,20 @@ export function initBubbleChart(canvas, title, chartData) {
396520
padding: 15,
397521
boxWidth: 20,
398522
boxHeight: 20,
523+
// Chart.js renders each legend item's text from
524+
// item.fontColor (plugin.legend.js); labels.color only
525+
// feeds the DEFAULT generateLabels, which this chart
526+
// overrides below. The color is resolved per draw so
527+
// the theme-change redraw (bindThemeChartRedraw)
528+
// re-syncs it; each palette's value keeps >= 4.5:1
529+
// against its --bg-color (#ffffff light / #10161d
530+
// dark), which no single color can do for both.
399531
generateLabels: () =>
400532
['High Priority (1)', 'Medium Priority', 'Low Priority (max)'].map((label, i) => ({
401533
text: label,
402534
fillStyle: ['rgb(235, 64, 52)', 'rgb(137, 119, 74)', 'rgb(39, 174, 96)'][i],
403535
strokeStyle: ['rgb(235, 64, 52)', 'rgb(137, 119, 74)', 'rgb(39, 174, 96)'][i],
536+
fontColor: resolveLegendTextColor(),
404537
lineWidth: 1,
405538
hidden: false,
406539
index: i
@@ -433,6 +566,7 @@ export function initBubbleChart(canvas, title, chartData) {
433566
}
434567
}
435568
});
569+
bubbleCharts.set(canvas, chart);
436570
}
437571

438572
export function initDisharmonyCharts(disharmonies) {
@@ -587,6 +721,9 @@ export async function enhanceReport(root, data) {
587721
if (event.key === 'Escape') hidePopup();
588722
});
589723
}
724+
// Theme switches are pure CSS, but chart legends are canvas-drawn: bind
725+
// their redraw once so the legend color keeps >= 4.5:1 in every palette.
726+
bindThemeChartRedraw();
590727
bindPopupHandlers(root);
591728

592729
// Menu links: explicit scrolling because the report renders too late for

‎tests/integration/report-view.test.jsx‎

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -693,6 +693,122 @@ describe('enhanced report tables: stateful widgets survive interactions', () =>
693693
expect(document.getElementById('overlay').style.display).toBe('block');
694694
hideAllPopups();
695695
});
696+
697+
test('a payload change destroys the previous charts before the loading markup and re-creates them', async () => {
698+
// Every Chart.js construction/teardown, in order, so the test can pin
699+
// that the old instances are released before the fetch effect clears
700+
// the payload (loading placeholder) and before the replacement render
701+
// builds fresh charts on the new canvases.
702+
const events = [];
703+
const instances = [];
704+
window.Chart = function () {
705+
const chart = {
706+
destroyed: false,
707+
update() {},
708+
destroy() {
709+
chart.destroyed = true;
710+
const canvas = document.querySelector('canvas#chart_GOD');
711+
events.push(canvas && canvas.isConnected ? 'destroy:attached' : 'destroy:detached');
712+
}
713+
};
714+
instances.push(chart);
715+
events.push('create');
716+
return chart;
717+
};
718+
markWidgetReady('chart');
719+
respondJsonFor(sampleJson);
720+
const utils = await renderReport();
721+
const firstCanvas = utils.container.querySelector('canvas#chart_GOD');
722+
const firstRenderCreates = instances.length;
723+
expect(firstRenderCreates).toBeGreaterThan(0);
724+
725+
// Switching branches re-runs the fetch effect: it must release the old
726+
// Chart.js instances (they would otherwise outlive their removed
727+
// canvases) before the loading placeholder replaces the report markup,
728+
// and the fresh render then builds new charts for the new canvases.
729+
utils.rerender(_jsx(ReportView, {
730+
username: 'junit-team',
731+
repository: 'junit4',
732+
branch: 'develop',
733+
widgetSettleMs: 25
734+
}));
735+
// The rerender passes through loading/graft phases that reuse or drop
736+
// the old canvas node; the destroys happen up front at the fetch
737+
// cycle's start, so wait on them and then on the fresh render.
738+
await waitFor(() => {
739+
expect(events.filter(e => e !== 'create').length).toBe(firstRenderCreates);
740+
});
741+
await waitFor(() => {
742+
expect(instances.length).toBe(2 * firstRenderCreates);
743+
});
744+
expect(utils.container.querySelector('canvas#chart_GOD')).not.toBe(firstCanvas);
745+
746+
expect(instances.slice(0, firstRenderCreates).every(c => c.destroyed)).toBe(true);
747+
// Destroyed while the canvases were still attached: before the loading
748+
// markup swap, not after the report markup was thrown away.
749+
const destroys = events.filter(e => e !== 'create');
750+
expect(destroys.every(e => e === 'destroy:attached')).toBe(true);
751+
expect(events).not.toContain('destroy:detached');
752+
// ...and all destroys precede every replacement construction.
753+
expect(events.findIndex(e => e !== 'create')).toBeLessThan(events.lastIndexOf('create'));
754+
// enhanceReport still builds charts for the replacement canvases.
755+
expect(instances.length).toBe(2 * firstRenderCreates);
756+
}, SLOW_TEST_MS);
757+
758+
test('unmounting the report destroys its live charts', async () => {
759+
const instances = [];
760+
window.Chart = function () {
761+
const chart = {
762+
destroyed: false,
763+
update() {},
764+
destroy() { chart.destroyed = true; }
765+
};
766+
instances.push(chart);
767+
return chart;
768+
};
769+
markWidgetReady('chart');
770+
respondJsonFor(sampleJson);
771+
const utils = await renderReport();
772+
expect(instances.length).toBeGreaterThan(0);
773+
expect(instances.every(c => c.destroyed)).toBe(false);
774+
775+
// The fetch effect's cleanup aborts the in-flight request; it must
776+
// release the report's Chart.js instances as well, since the canvases
777+
// go away with the unmounted container.
778+
utils.unmount();
779+
expect(instances.every(c => c.destroyed)).toBe(true);
780+
});
781+
782+
test('a refetch destroys the previous charts up front, even if it never delivers a payload', async () => {
783+
const instances = [];
784+
window.Chart = function () {
785+
const chart = {
786+
destroyed: false,
787+
update() {},
788+
destroy() { chart.destroyed = true; }
789+
};
790+
instances.push(chart);
791+
return chart;
792+
};
793+
markWidgetReady('chart');
794+
respondJsonFor(sampleJson);
795+
const utils = await renderReport();
796+
expect(instances.length).toBeGreaterThan(0);
797+
expect(instances.every(c => c.destroyed)).toBe(false);
798+
799+
// A branch switch whose fetch never resolves: the old Chart.js
800+
// instances must be released when the fetch cycle starts — before the
801+
// payload is cleared and the loading markup replaces the report — not
802+
// wait for a replacement payload that may never arrive.
803+
mockFetch.mockImplementation(() => new Promise(() => {}));
804+
utils.rerender(_jsx(ReportView, {
805+
username: 'junit-team',
806+
repository: 'junit4',
807+
branch: 'develop',
808+
widgetSettleMs: 25
809+
}));
810+
expect(instances.every(c => c.destroyed)).toBe(true);
811+
});
696812
});
697813

698814
async function hideAllPopups() {

0 commit comments

Comments
 (0)