diff --git a/.changeset/7396-scatter-extreme-marks-stay-inside-the-plot.md b/.changeset/7396-scatter-extreme-marks-stay-inside-the-plot.md new file mode 100644 index 0000000000..59398c2986 --- /dev/null +++ b/.changeset/7396-scatter-extreme-marks-stay-inside-the-plot.md @@ -0,0 +1,36 @@ +--- +'@object-ui/plugin-charts': patch +--- + +Reserve a margin at both ends of a scatter's numeric axes, so an extreme mark is drawn +wholly inside the plot area (objectui#7396). + +Both scatter axes are numeric and carry no explicit domain, so recharts fits the domain +to `[dataMin, dataMax]` and maps it across the whole plot box. A row at either extreme +is therefore **centred on the boundary**, and since a mark has a radius, about half of +each extreme symbol paints outside the plot area — the half-dots hugging both edges of +the Chart Gallery scatter. + +Measured on that scatter ("Estimate vs Progress") in real Chromium — viewport 1440, +widget svg 510x350, plot area x 53..505 / y 5..296: + +- before: marks at cx 53, 256.4, 301.6, 414.6, 459.8, 505 with the y-max row at cy 5, + radius 4.514px. The first and last sit exactly on the x boundary and the y-max one on + the top boundary, each overhanging its edge by a full radius. +- after: cx 65, 257.6, 300.4, 407.4, 450.2, 493 with the y-max row at cy 17. The worst + case now clears its nearest edge by 7.486px, on both axes. + +The card reported the x axis; the y axis clipped the same way and is fixed with it. + +**The domain is not touched.** The margin is reserved as recharts' axis `padding`, which +insets the pixel range the scale maps into and leaves the domain alone, so every tick +**value** is unchanged — the axes still read 0/25/50/75/100 and 0/15/30/45/60 — and only +the mapping moves. Padding the domain instead would invent unround tick endpoints, and it +would write the same recharts prop a spec-declared `min`/`max` needs (objectui#9675), where +whichever landed second would shadow the other. + +The margin is sized to the largest radius the scatter's declared symbol-area envelope +admits, not to the radius drawn today, so neither a change in recharts' own default mark +size nor a future variable-size mark can reopen it. + +Every scatter's marks shift inward by that margin; nothing else about the chart changes. diff --git a/packages/plugin-charts/src/AdvancedChartImpl.scatterEdgeClipping-7396.test.tsx b/packages/plugin-charts/src/AdvancedChartImpl.scatterEdgeClipping-7396.test.tsx new file mode 100644 index 0000000000..8f3888c51f --- /dev/null +++ b/packages/plugin-charts/src/AdvancedChartImpl.scatterEdgeClipping-7396.test.tsx @@ -0,0 +1,196 @@ +/** + * objectui#7396 — a scatter's extreme marks were half-clipped by the plot edge. + * + * ## The defect + * + * Both scatter axes are numeric and carry no explicit domain, so recharts fits + * the domain to `[dataMin, dataMax]` and maps it across the WHOLE plot box. A + * row at either extreme is therefore CENTRED on the boundary, and since a mark + * has a radius, about half of it paints outside the plot area. + * + * Measured on the Chart Gallery scatter ("Estimate vs Progress") in real + * Chromium — viewport 1440, widget svg 510x350, plot area x 53..505 / y 5..296: + * + * before: cx 53, 256.4, 301.6, 414.6, 459.8, 505 and the y-max row at cy 5, + * r 4.514 — first and last ON the x boundary, y-max ON the top one, + * each overhanging its edge by 4.514px. + * after: cx 65, 257.6, 300.4, 407.4, 450.2, 493, y-max at cy 17 — worst + * clearance 7.486px INSIDE the plot area, on both axes. + * + * The card reported the x axis only; the y axis clips the same way and is + * covered here too. + * + * ## Why this is measurable in this environment, when chart geometry usually is not + * + * The usual objection holds — happy-dom reports `clientWidth` 0, so a + * container-size effect never fires and `ResponsiveContainer` would measure + * nothing. That is exactly what the mock below removes: given an explicit + * width/height, recharts computes the plot rect, every `cx`/`cy` and the symbol + * radius ARITHMETICALLY, with no DOM measurement in the path. Verified against + * the real-Chromium run above: the x coordinates agree to the digit. + * + * What is NOT portable is any ABSOLUTE coordinate that depends on text metrics + * — the legend is shorter here, so the plot rect is taller than Chromium's. So + * nothing below pins a coordinate: every assertion compares a mark against the + * plot rect READ FROM THE SAME RENDER. + */ +import { describe, it, expect, vi, afterEach } from 'vitest'; +import React from 'react'; +import { render, cleanup } from '@testing-library/react'; + +vi.mock('recharts', async () => { + const actual = await vi.importActual('recharts'); + return { + ...actual, + // The widget's real box on the showcase Chart Gallery. + ResponsiveContainer: ({ children }: any) => + React.cloneElement(children, { width: 510, height: 350 }), + }; +}); + +import AdvancedChartImpl from './AdvancedChartImpl'; + +afterEach(cleanup); + +type Row = Record; + +/** The six aggregate rows the Chart Gallery scatter plots: progress buckets x + * avg estimate_hours. Its x extremes (0 and 100) are the clipped pair. */ +const GALLERY: Row[] = [ + { progress: 0, avg_estimate: 16.5 }, + { progress: 45, avg_estimate: 40 }, + { progress: 55, avg_estimate: 60 }, + { progress: 80, avg_estimate: 24 }, + { progress: 90, avg_estimate: 30 }, + { progress: 100, avg_estimate: 12 }, +]; + +/** Puts a row on each of the four edges at once: x min/max and y min/max. The + * y floor needs a zero because recharts anchors an all-positive y domain at 0, + * so the lowest POSITIVE row never reaches the bottom. */ +const FOUR_EDGES: Row[] = [ + { progress: 0, avg_estimate: 0 }, + { progress: 50, avg_estimate: 30 }, + { progress: 100, avg_estimate: 60 }, +]; + +const CONFIG = { progress: { label: 'Progress' }, avg_estimate: { label: 'Avg Estimate' } }; + +const renderScatter = (data: Row[]) => + render( + , + ); + +/** The plot area, exactly: recharts emits the chart offset rect as the clip path + * every cartesian layer is drawn through. */ +const plotAreaOf = (c: HTMLElement) => { + const rect = c.querySelector('defs clipPath rect'); + expect(rect, 'recharts drew no plot-area clip rect').not.toBeNull(); + const num = (a: string) => Number(rect!.getAttribute(a)); + const x = num('x'); + const y = num('y'); + const w = num('width'); + const h = num('height'); + expect([x, y, w, h].every(Number.isFinite), 'plot rect is not numeric').toBe(true); + expect(w, 'plot area has no width').toBeGreaterThan(0); + expect(h, 'plot area has no height').toBeGreaterThan(0); + return { left: x, right: x + w, top: y, bottom: y + h }; +}; + +/** Each mark's painted centre and radius, straight off the symbol path: recharts + * writes the symbol's bounding box onto it, so `width / 2` IS the drawn radius + * rather than a value assumed from the ZAxis range. */ +const marksOf = (c: HTMLElement) => + [...c.querySelectorAll('path.recharts-symbols')].map((p) => { + const cx = Number(p.getAttribute('cx')); + const cy = Number(p.getAttribute('cy')); + const r = Number(p.getAttribute('width')) / 2; + expect([cx, cy, r].every(Number.isFinite), 'symbol carries no geometry').toBe(true); + expect(r, 'symbol has no radius, so this measures nothing').toBeGreaterThan(0); + return { cx, cy, r }; + }); + +/** Per mark, how far `centre ± r` pokes PAST each plot edge. Negative is inside; + * zero is the defect (centred on the boundary, half outside). */ +const overhangs = (c: HTMLElement) => { + const plot = plotAreaOf(c); + const marks = marksOf(c); + expect(marks.length, 'no marks were drawn').toBeGreaterThan(0); + return marks.map((m) => ({ + mark: m, + left: plot.left - (m.cx - m.r), + right: m.cx + m.r - plot.right, + top: plot.top - (m.cy - m.r), + bottom: m.cy + m.r - plot.bottom, + })); +}; + +const worstOverhang = (c: HTMLElement) => + Math.max(...overhangs(c).flatMap((o) => [o.left, o.right, o.top, o.bottom])); + +describe('objectui#7396 — every scatter mark is drawn wholly inside the plot area', () => { + it('keeps the gallery scatter clear of both x edges', () => { + // The card's own fixture. Its x extremes sat exactly ON the boundary, so + // every overhang here was +r before the margin was reserved. + const { container } = renderScatter(GALLERY); + for (const o of overhangs(container)) { + expect(o.left, `mark at cx ${o.mark.cx} pokes past the left edge`).toBeLessThan(0); + expect(o.right, `mark at cx ${o.mark.cx} pokes past the right edge`).toBeLessThan(0); + } + }); + + it('keeps the gallery scatter clear of the y edges too', () => { + // Not in the card, found while reproducing it: the y-max row was centred on + // the plot's top edge in the same real-Chromium run. + const { container } = renderScatter(GALLERY); + for (const o of overhangs(container)) { + expect(o.top, `mark at cy ${o.mark.cy} pokes past the top edge`).toBeLessThan(0); + expect(o.bottom, `mark at cy ${o.mark.cy} pokes past the bottom edge`).toBeLessThan(0); + } + }); + + it('holds when one row sits on each of the four edges at once', () => { + // The invariant, not the instance: x min, x max, y min and y max all + // occupied. The margin has to be reserved on both ends of both axes. + const { container } = renderScatter(FOUR_EDGES); + expect(worstOverhang(container), 'a mark is painted outside the plot area').toBeLessThan(0); + }); + + it('reserves at least a full mark radius, so the WHOLE symbol clears the edge', () => { + // Stated as the reader sees it — "half a dot is showing" is the complaint, + // and clearing the edge by a hair would still leave it half-clipped once a + // symbol grows. The margin is sized to the declared symbol envelope, so + // every clearance is a full radius or better. + const { container } = renderScatter(FOUR_EDGES); + for (const o of overhangs(container)) { + for (const [edge, value] of Object.entries({ left: o.left, right: o.right, top: o.top, bottom: o.bottom })) { + expect(value, `clearance at the ${edge} edge is under one mark radius`).toBeLessThanOrEqual(-o.mark.r); + } + } + }); + + it('reserves the margin WITHOUT moving the domain — the tick values are untouched', () => { + // Why this is one assertion with the containment above rather than its own + // pin: on its own it passes on the defect too, so it would measure nothing. + // Together they state the whole fix — the marks came inside AND the domain + // did not move to bring them there. Moving the domain instead would invent + // unround tick endpoints and would write the very prop the scatter's + // missing spec-axis spread is about (objectui#9675). + const { container } = renderScatter(GALLERY); + expect(worstOverhang(container), 'a mark is painted outside the plot area').toBeLessThan(0); + const ticks = (orientation: string) => + [...container.querySelectorAll(`text.recharts-cartesian-axis-tick-value[orientation="${orientation}"]`)] + .map((t) => (t.textContent ?? '').trim()); + // Round endpoints on both axes: the domains are still the data's own + // `[0, 100]` and `[0, 60]`, laid over a plot box that is merely inset. + expect(ticks('bottom'), 'the x domain moved').toEqual(['0', '25', '50', '75', '100']); + expect(ticks('left'), 'the y domain moved').toEqual(['0', '15', '30', '45', '60']); + }); +}); diff --git a/packages/plugin-charts/src/AdvancedChartImpl.tsx b/packages/plugin-charts/src/AdvancedChartImpl.tsx index b91c82abfd..34f262cb6d 100644 --- a/packages/plugin-charts/src/AdvancedChartImpl.tsx +++ b/packages/plugin-charts/src/AdvancedChartImpl.tsx @@ -394,6 +394,69 @@ const X_AXIS_ALL_LABELS_MAX_BUCKETS = 5; */ const ROTATED_X_LABEL_MAX_CHARS = 12; +/** + * Symbol AREA envelope the scatter branch declares for its marks, in px². + * + * Kept as a named number because the padding below is derived from it rather + * than guessed: the scatter's `` + * declares that envelope, recharts paints a scatter mark as a circle of + * `sqrt(area / PI)`, so this is the upper bound of the radius it can draw. + */ +const SCATTER_SYMBOL_MAX_AREA = 400; + +/** + * Margin, in px, that each scatter numeric axis reserves at BOTH ends so an + * extreme mark is drawn WHOLLY inside the plot area (objectui#7396). + * + * ## What it fixes + * + * Both scatter axes are numeric with no explicit domain, so recharts fits the + * domain to `[dataMin, dataMax]` and maps it across the WHOLE plot box. A row + * at either extreme is therefore CENTRED on the boundary and half its symbol + * paints outside the plot area. Measured on the Chart Gallery scatter + * ("Estimate vs Progress") in real Chromium — viewport 1440, widget svg + * 510x350, plot area x 53..505 / y 5..296 — marks landed at cx 53, 256.4, + * 301.6, 414.6, 459.8, 505 and the y-max row at cy 5: the first and last on the + * x boundary, the y-max one on the top boundary, each overhanging by its own + * radius. It is BOTH axes, not just the x the card measured. + * + * ## Why `padding` and not `domain` + * + * Recharts' axis `padding` insets the pixel RANGE the scale maps into — the + * axis range becomes `[left + padding.left, right - padding.right]` — and + * leaves the domain alone, so every tick VALUE is unchanged and only the + * mapping moves. Moving the domain instead would both invent unround tick + * endpoints and write the very prop the scatter's missing `yAxisSpecProps` + * spread is about (objectui#9675): a spec-derived domain and a + * padding domain are one recharts prop, and whichever landed second would + * shadow the other. Reserving the margin keeps that prop free. + * + * ## Why this size + * + * `sqrt(SCATTER_SYMBOL_MAX_AREA / PI)`, rounded up — the largest radius the + * declared envelope admits. It is deliberately the envelope's upper bound and + * not the radius observed today: with no `dataKey` on that ZAxis recharts + * ignores the declaration entirely and paints every mark at its own implicit + * default area (measured: r = 4.514px, i.e. area 64), so sizing this to what is + * painted would tie the fix to a third-party default AND reopen the defect the + * day a `dataKey` makes the declared envelope live. + * + * ## Why these are module constants and not inline objects + * + * Recharts memoises each axis's settings object on its props and dispatches a + * `replaceXAxis` / `replaceYAxis` when that object changes; a fresh literal per + * render would defeat the memo and re-register the axis on every render. + */ +const SCATTER_AXIS_EDGE_PADDING = Math.ceil(Math.sqrt(SCATTER_SYMBOL_MAX_AREA / Math.PI)); +const SCATTER_X_AXIS_PADDING = { + left: SCATTER_AXIS_EDGE_PADDING, + right: SCATTER_AXIS_EDGE_PADDING, +} as const; +const SCATTER_Y_AXIS_PADDING = { + top: SCATTER_AXIS_EDGE_PADDING, + bottom: SCATTER_AXIS_EDGE_PADDING, +} as const; + /** * Treemap leaf cell — paints each leaf rect with its palette fill + label. * Hoisted to module scope so it is a stable component reference rather than one @@ -1835,6 +1898,7 @@ function AdvancedChartImplInner({ tickLine={false} axisLine={false} minTickGap={isMobile ? 32 : 48} + padding={SCATTER_X_AXIS_PADDING} /> } />