diff --git a/.changeset/dashboard-grid-dom-prop-whitelist-4432.md b/.changeset/dashboard-grid-dom-prop-whitelist-4432.md new file mode 100644 index 000000000..e74c50115 --- /dev/null +++ b/.changeset/dashboard-grid-dom-prop-whitelist-4432.md @@ -0,0 +1,17 @@ +--- +'@object-ui/plugin-dashboard': patch +--- + +`DashboardRenderer`'s widget grid now passes only whitelisted DOM props to its container (objectui#4432) + +`view:dashboard` resolves to this component, so `SchemaRenderer` handed it the dashboard node's own keys, the contents of the node's `props` container, the ARIA it resolved and the host's trailing props — and every key the component did not destructure was spread raw onto the grid container. React writes unknown lowercase attributes through in silence and stringifies object values, so the failure was invisible. Measured through the real SDUI path: **13 non-DOM attributes**, including `events="[object Object]"`, `props="[object Object]"` and a camelCase `arialabel` sitting next to the resolved `aria-label`, so the element carried each ARIA value twice under two spellings — one of them meaningless to assistive technology. + +The container is now consume-or-whitelist per objectui#4425 phase 2: only `toDomProps`' output reaches the element, and it is spread FIRST so the component's own computed attributes stay authoritative. The resolved `aria-label` / `aria-describedby`, `role`, `id`, `tabIndex`, `className` and the `data-*` family still arrive — dropping them would have been an accessibility regression dressed as a leak fix, so the new pin asserts the delivered set exactly, not just the absent one. Both layout branches are covered: the responsive desktop grid and the mobile stack spread the same props onto the same host element. + +Three behaviours move with the spread, all of them consequences of a trailing spread that used to override the component's own computed props: + +- **`onClick` now has one carrier.** It is a declared DOM pass-through key AND this container computes a design-mode background handler, and the old spread let the incoming handler replace the computed one — so a host that passed `onClick` silently lost background deselection. Both run now, container affordance first. An authored non-function `onClick` (SDUI spells click behaviour `events: { onClick }`, which is data and is dropped) is ignored instead of handed to React, which used to throw on it. +- **An authored `style` no longer replaces the computed grid layout.** `style` is not in the SDUI pass-through set, and this container computes its own `gridTemplateColumns` / `gridAutoRows` / `gap`; an authored `style` used to overwrite all of it and collapse the grid. +- **An authored `data-user-actions` no longer overrides the value computed from the `userActions` prop.** The `data-*` family still passes the whitelist; only this one collision with a computed attribute resolves the other way now. + +The injected `disabled` verdict is also dropped rather than forwarded. Nothing in this component ever read it: it only became a `disabled` attribute on a container element that has no such attribute, which is the leak, not a behaviour. diff --git a/packages/app-shell/src/__tests__/widget-dom-leak-sweep.test.tsx b/packages/app-shell/src/__tests__/widget-dom-leak-sweep.test.tsx index be881eb7d..6cbe3235d 100644 --- a/packages/app-shell/src/__tests__/widget-dom-leak-sweep.test.tsx +++ b/packages/app-shell/src/__tests__/widget-dom-leak-sweep.test.tsx @@ -14,30 +14,45 @@ * * ## What this is, and what it deliberately is NOT * - * This is phase 1 of objectui#4425: **measure before converging.** It renders - * every target through the real SDUI path with planted canaries and checks every - * attribute of every rendered element against what HTML actually defines. It - * changes **no widget contract and no widget source** — leaks it finds are - * RECORDED in {@link LEAK_LEDGER} with a reason and an owning issue, never - * silently tolerated and never fixed here. - * - * Phase 2 — promoting `toDomProps`' whitelist to the SDUI widget contract, i.e. - * option 1 on objectui#4425 — is the maintainer's decision and is explicitly out - * of scope. It is now to be decided on THIS GATE'S READING rather than on an - * assumption about how wide the leak is. The reading, measured here: + * This file landed as phase 1 of objectui#4425: **measure before converging.** + * It renders every target through the real SDUI path with planted canaries and + * checks every attribute of every rendered element against what HTML actually + * defines. It changes **no widget contract and no widget source** — leaks it + * finds are RECORDED in {@link LEAK_LEDGER} with a reason and an owning issue, + * never silently tolerated and never fixed here. + * + * Phase 2 is now RULED (objectui#4425, comment 5270759246): option 1 — + * `toDomProps`' whitelist is promoted to the SDUI widget contract, and the + * migration runs as per-package cards that each delete their own ledger rows. + * So this gate's job changed without its mechanism changing: it was the + * measurement the ruling was waiting for, and it is now the ratchet that + * migration is graded against. The reading, kept at current truth as each card + * lands: * * | package | targets | targets leaking | leaked attributes | * |------------------|---------|-----------------|-------------------| * | plugin-charts | 9 | 0 | 0 | * | plugin-calendar | 3 | 0 | 0 | - * | plugin-chatbot | 3 | 2 | 14 each | - * | plugin-dashboard | 8 | 3 | 7 / 9 / 13 | - * - * 5 of 23 targets leak. The two clean packages are clean for opposite reasons - * worth keeping straight: `plugin-charts` never spreads the node onto its - * container at all, while `plugin-calendar`'s components take a declared prop + * | plugin-chatbot | 3 | 0 | 0 | + * | plugin-dashboard | 8 | 2 | 7 / 9 | + * + * **2 of 23 targets leak**, and both are in {@link LEAK_LEDGER} below: + * `plugin-dashboard:metric` and `plugin-dashboard:metric-card`, the open tail + * objectui#4425 owns directly. Two migration steps have closed their rows since + * the phase-1 measurement: + * + * - `plugin-chatbot:chatbot` / `chatbot-enhanced` — 14 attributes each, + * objectui#4431 / PR #4485, which also lifted `toDomProps` to + * `@object-ui/core` so later cards consume one executor. + * - `view:dashboard` — `DashboardRenderer`'s widget-grid container, 13 + * attributes, objectui#4432 / this file's most recent edit. + * + * The three packages now reading 0 are NOT clean for the same reason, and the + * difference is worth keeping straight: `plugin-charts` never spreads the node + * onto its container at all; `plugin-calendar`'s components take a declared prop * list and drop what they do not name, so the node's keys never reach an - * element. + * element; `plugin-chatbot` and `DashboardRenderer`'s grid reach zero by + * FILTERING — they still spread, through `toDomProps`. * * `calendar-view` was originally swept with the `events` canary WITHHELD, because * authoring it crashed the component outright (objectui#4433) — a worse failure @@ -45,19 +60,20 @@ * clean pass. That is fixed, so the omission is gone and the target is swept * with the full canary set; section 5 below carries what is left of it. * - * ## The divergence this repo is living with, recorded deliberately + * ## The divergence that decided phase 2, and what is left of it * - * objectui#4425's option 3 was "record the divergence rather than converge", and - * it happens here whichever way phase 2 goes, because both answers are live in - * the tree at once and this gate observes both: + * objectui#4425's option 3 was "record the divergence rather than converge". + * Both answers were live in the tree at once and this gate observed both, which + * is how the ruling got its evidence: * - * - `packages/fields` uses a **whitelist** (`widgets/toDomProps.ts`, #3291): - * keep the declared DOM pass-through keys, drop everything else. Its own - * gate (`fields/src/__tests__/widget-dom-leak-e2e.test.tsx`) is untouched by - * this file and stays the reference implementation of the technique. - * - `packages/plugin-dashboard` uses a **deny-list** (`src/schemaHostProps.ts`, - * #4357/PR #4428): destructure seven measured non-DOM props out, spread the - * rest. + * - a **whitelist** (`toDomProps`, #3291): keep the declared DOM pass-through + * keys, drop everything else. Born in `packages/fields`, whose own gate + * (`fields/src/__tests__/widget-dom-leak-e2e.test.tsx`) is untouched by this + * file and stays the reference implementation of the technique; the + * MECHANISM now lives in `@object-ui/core` (`utils/dom-props.ts`, #4431) so + * every plugin package can reach it. + * - a **deny-list** (`plugin-dashboard/src/schemaHostProps.ts`, #4357/PR + * #4428): destructure seven measured non-DOM props out, spread the rest. * * The deny-list is correct for the seven props it enumerates — this sweep * confirms all seven are gone from `metric` and `metric-card`. What it cannot @@ -67,8 +83,15 @@ * `metric-card`, where `zzcanary` / `reference_to` / an authored * `props: { colorVariant }` all land as attributes while every one of the seven * named keys is correctly stripped. A deny-list bounded by enumeration cannot be - * finished; a whitelist bounded by declaration can. That contrast is the - * measurement phase 2 was waiting for. + * finished; a whitelist bounded by declaration can. That contrast IS the + * measurement phase 2 waited for, and the ruling went to the whitelist on it. + * + * So the divergence is no longer a standing state of the repo — it is a + * migration in progress, and the two rows still below are the last of it inside + * these four packages. `plugin-dashboard` is now MIXED by design: its + * `DashboardRenderer` grid container filters through `toDomProps` (#4432) while + * the two KPI components still run the deny-list, and the two surviving rows are + * precisely that difference, measured. * * ## Why this file lives in `packages/app-shell` * @@ -542,23 +565,6 @@ const LEAK_LEDGER: Readonly> = { 'the DOM as an attribute instead of rendering it.', issue: 'objectui#4425', }, - - /* ── plugin-dashboard: the container the KPI cards sit in ────────────────── */ - // - // Same package as the two rows above, different element and NOT in #4357's - // scope: `DashboardRenderer`'s widget grid still spreads. `datasource` is - // absent here because this component consumes the adapter. - 'view:dashboard': { - attributes: [ - 'ariadescribedby', 'arialabel', 'bind', 'colorvariant', 'events', 'name', - 'props', 'reference_to', 'zzcanary', 'zzcanarycamel', 'zzcanarynum', - 'zzcanaryobj', 'zzcanaryprop', - ], - reason: - "DashboardRenderer's widget-grid container ends in a bare `{...props}` " + - 'spread, so the dashboard node\'s own keys land on it.', - issue: 'objectui#4432', - }, }; /* ════════════════════════════════════════════════════════════════════════════ diff --git a/packages/plugin-dashboard/src/DashboardRenderer.tsx b/packages/plugin-dashboard/src/DashboardRenderer.tsx index 78f9c9283..256e71dd0 100644 --- a/packages/plugin-dashboard/src/DashboardRenderer.tsx +++ b/packages/plugin-dashboard/src/DashboardRenderer.tsx @@ -15,6 +15,7 @@ import { dashboardFilterVariableDefs, buildWidgetScopedFilter, mergeFilters, + toDomProps, } from '@object-ui/core'; import { cn, Card, CardHeader, CardTitle, CardContent, Button, getLazyIcon } from '@object-ui/components'; import { forwardRef, useState, useEffect, useCallback, useMemo, useRef, Fragment } from 'react'; @@ -401,6 +402,35 @@ const DashboardRendererInner = forwardRef { + handleBackgroundClick(e); + if (typeof props.onClick === 'function') props.onClick(e); + }; + // --- Drag-and-drop reordering (design mode only) --------------------- // Powered by @dnd-kit. Because each widget renders with // `gridColumn: span W` (no explicit x/y), array order *is* visual order, @@ -865,6 +895,35 @@ const DashboardRendererInner = forwardRef` below, and React writes + * unknown lowercase attributes through in silence while stringifying object + * values. Measured through the real SDUI path: **13 non-DOM attributes**, + * including `events="[object Object]"`, `props="[object Object]"` and a + * camelCase `arialabel` sitting next to the resolved `aria-label` — the same + * value twice, one spelling of it meaningless to assistive technology. + * + * Per objectui#4425 phase 2 the answer is the whitelist, not another + * deny-list: `toDomProps` keeps `id` / `className` / `role` / `tabIndex` / + * `aria-*` / `data-*` and drops the rest. It is spread FIRST so this + * component's own computed attributes (`className`, `style`, + * `data-user-actions`, the click channel above) stay authoritative — the + * old trailing spread let an incoming key overwrite each of them. + * + * `datasource` is absent from the 13 because this component destructures + * the adapter and hands it to its own `SchemaRenderer` calls; that is the + * one key objectui#4428 shipped a pass without, and it never reached this + * spread. + */ + const hostDomProps = toDomProps(props); + const refreshButton = onRefresh && (
{recordCountBadge} @@ -902,7 +961,7 @@ const DashboardRendererInner = forwardRef w.type !== 'metric') || []; const mobileBody = ( -
+
{headerSection} {filterBar} {refreshButton} @@ -937,6 +996,7 @@ const DashboardRendererInner = forwardRef {headerSection} {filterBar} diff --git a/packages/plugin-dashboard/src/__tests__/DashboardRenderer.domProps.test.tsx b/packages/plugin-dashboard/src/__tests__/DashboardRenderer.domProps.test.tsx new file mode 100644 index 000000000..8797faa78 --- /dev/null +++ b/packages/plugin-dashboard/src/__tests__/DashboardRenderer.domProps.test.tsx @@ -0,0 +1,276 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * The DOM pass-through contract of `DashboardRenderer`'s widget-grid container + * (objectui#4432, migration step 2 of objectui#4425 phase 2). + * + * ## What these pin that the sweep gate cannot + * + * `packages/app-shell/src/__tests__/widget-dom-leak-sweep.test.tsx` is the + * cross-package ratchet: it proves nothing UNEXPLAINED arrives, and its + * `view:dashboard` row expired with this fix. These cases are the other half, + * in this package, next to the code: + * + * - the exact attribute SET on the host element — so a key that stops being + * DELIVERED (`role`, `aria-label`, `data-obj-id`, `tabindex`) is as red as a + * key that leaks. The sweep structurally cannot see that direction: it looks + * for attributes that arrive, not for ones that go missing. Removing a spread + * can regress accessibility just as quietly as leaving it in place leaks. + * - the two items objectui#4432 named, each asserted as a PAIR so neither half + * can drift: the camelCase `arialabel` / `ariadescribedby` gone while the + * RESOLVED `aria-label` / `aria-describedby` — the spellings assistive + * technology actually reads — stay with their values. + * - the click channel, which is the one place the removed spread was + * OVERRIDING a computed prop rather than merely adding attributes. + * + * ## Why the adapter is attached + * + * `dataSource` is not a schema key: `SchemaRenderer` strips the node's own + * `dataSource` BINDING by name (objectstack#5576), and the one that reaches a + * widget is the injected ADAPTER a host hands the renderer. It therefore only + * appears on a deployment that really loads data — which is why PR #4428 shipped + * a six-key first pass measured against a schema-only fixture and missed exactly + * that key. `DashboardRenderer` happens to destructure the adapter (so it was + * never among this container's 13 leaked attributes), but a fixture that cannot + * SEE the key cannot pin its absence either, so every render here goes through + * `SchemaRendererProvider` WITH an adapter. + */ + +import * as React from 'react'; +import { describe, it, expect, afterEach } from 'vitest'; +import { render, waitFor, cleanup, fireEvent } from '@testing-library/react'; +import { SchemaRenderer, SchemaRendererProvider } from '@object-ui/react'; +// Side-effect import: the package barrel is what registers `view:dashboard`. +// At MODULE scope (never inside a case) per AGENTS.md's flaky-test rule — the +// cost lands in the import phase, unbounded by any timeout. +import { DashboardRenderer } from '../index'; + +afterEach(cleanup); + +/** The injected adapter. Answers empty so nothing lands in an error state. */ +const ADAPTER = { + find: async () => [], + findOne: async () => null, + aggregate: async () => [], + count: async () => 0, + getObject: async () => null, + queryDataset: async () => ({ rows: [] }), +}; + +/** + * One dashboard node carrying every family the renderer hands a widget: + * authored SDUI metadata, the camelCase ARIA the resolver converts, the DOM + * identity keys that must survive, an open tail of keys no component declares, + * and a `props` container whose CONTENTS the renderer spreads separately. + * + * Deliberately the same canary vocabulary the sweep gate plants, so the two + * gates report one vocabulary rather than two. + */ +const CANARY_NODE = { + type: 'view:dashboard', + id: 'canary-node', + name: 'canary_node', + className: 'zz-authored-class', + role: 'region', + tabIndex: 0, + bind: 'data.revenue', + events: { onClick: [{ action: 'navigate', params: { url: '/x' } }] }, + ariaLabel: 'Canary label', + ariaDescribedBy: 'canary-desc', + zzcanary: 'CANARY-STR', + zzcanaryobj: { nested: true }, + zzcanarynum: 42, + zzcanaryCamel: 'CANARY-CAMEL', + reference_to: 'contacts', + props: { colorVariant: 'success', zzcanaryprop: 'CANARY-PROP' }, +} as const; + +/** Renders the dashboard through the real SDUI host and returns the grid. */ +async function renderCanaryDashboard(): Promise { + render( + + + , + ); + + // Non-vacuity: the REAL grid must exist before anything is scanned. The + // responsive flow layout is the branch this node takes (no `columns`), and + // `auto-rows-min` is what identifies it. + await waitFor(() => { + if (!document.body.querySelector('.grid.auto-rows-min')) { + throw new Error( + 'the widget grid never rendered, so scanning it would measure nothing. ' + + `Body was:\n${document.body.innerHTML.slice(0, 600)}`, + ); + } + }); + + const grid = document.body.querySelector('[data-obj-id="canary-node"]'); + if (!grid) { + throw new Error( + 'no element carries data-obj-id — either the container stopped forwarding ' + + "the renderer's own identity attributes, or it never rendered.", + ); + } + return grid; +} + +function attributeNames(element: Element): string[] { + return Array.from(element.attributes) + .map((attribute) => attribute.name) + .sort(); +} + +describe("DashboardRenderer's widget grid — only whitelisted DOM props (objectui#4432)", () => { + it('carries EXACTLY the whitelisted DOM attributes — no more, no fewer', async () => { + const grid = await renderCanaryDashboard(); + + // Exact set equality, the sweep ledger's own discipline applied locally: a + // new leak fails, and so does a DOM key that silently stops being + // delivered. `class` and `style` are the container's OWN computed grid + // layout, not pass-through. + expect(attributeNames(grid)).toEqual([ + 'aria-describedby', + 'aria-label', + 'class', + 'data-obj-id', + 'data-obj-type', + 'id', + 'role', + 'style', + 'tabindex', + ]); + + // The whitelisted keys carry their real values — an empty pass-through + // would satisfy the set above only by also losing these. + expect(grid).toHaveAttribute('id', 'canary-node'); + expect(grid).toHaveAttribute('data-obj-type', 'view:dashboard'); + expect(grid).toHaveAttribute('role', 'region'); + expect(grid).toHaveAttribute('tabindex', '0'); + expect(grid.className).toContain('zz-authored-class'); + // The component's own grid layout survived the reordered spread. + expect(grid.className).toContain('auto-rows-min'); + expect(grid.getAttribute('style')).toContain('gap'); + }); + + it('drops the SDUI metadata, the `props` container and the open tail', async () => { + const grid = await renderCanaryDashboard(); + + // The 13 attributes objectui#4432 measured, by name. + for (const attribute of [ + 'ariadescribedby', + 'arialabel', + 'bind', + 'colorvariant', + 'events', + 'name', + 'props', + 'reference_to', + 'zzcanary', + 'zzcanarycamel', + 'zzcanarynum', + 'zzcanaryobj', + 'zzcanaryprop', + ]) { + expect(grid, `the grid leaked ${attribute}`).not.toHaveAttribute(attribute); + } + + // `schema` and `datasource` were never among the 13 — the renderer injects + // the first and this component destructures the second — so pin them too, + // or the whitelist could stop covering them without any case going red. + expect(grid).not.toHaveAttribute('schema'); + expect(grid).not.toHaveAttribute('datasource'); + + // The mechanism half: an object that reaches an attribute is `String()`-ed, + // so a stringified object anywhere on the element means something non-DOM + // got through under some other name. + const stringified = Array.from(grid.attributes) + .filter((attribute) => attribute.value.includes('[object Object]')) + .map((attribute) => `${attribute.name}="${attribute.value}"`); + expect(stringified).toEqual([]); + }); + + it('drops the camelCase ARIA and keeps the resolved spellings', async () => { + const grid = await renderCanaryDashboard(); + + // Meaningless to assistive technology — these are the AUTHORED keys, and + // the resolver already emitted the hyphenated forms from them. + expect(grid).not.toHaveAttribute('arialabel'); + expect(grid).not.toHaveAttribute('ariadescribedby'); + // The half that MUST survive: dropping these would be an accessibility + // regression dressed as a leak fix. + expect(grid).toHaveAttribute('aria-label', 'Canary label'); + expect(grid).toHaveAttribute('aria-describedby', 'canary-desc'); + }); +}); + +/** + * The one place the removed spread was OVERRIDING a computed prop rather than + * only adding attributes: `onClick`. + * + * `onClick` is a declared DOM pass-through key of the SDUI widget contract AND + * this container computes its own design-mode background handler. The old + * trailing `{...props}` spread resolved that collision by letting the incoming + * handler REPLACE the computed one, so a host that passed `onClick` silently + * lost background deselection. Dropping the incoming handler would be the + * mirror failure — a whitelisted key that never arrives. Both run now, and both + * directions are asserted here so neither can be quietly restored. + */ +describe("the grid's click channel has one carrier (objectui#4432)", () => { + const DASHBOARD = { + type: 'dashboard', + name: 'sales', + widgets: [], + } as never; + + it('a host onClick and the design-mode background handler BOTH fire', () => { + const hostClicks: string[] = []; + const selections: Array = []; + + const { container } = render( + selections.push(id)} + onClick={() => hostClicks.push('host')} + />, + ); + + const grid = container.querySelector('.grid'); + if (!grid) throw new Error('the widget grid never rendered'); + fireEvent.click(grid); + + // The container affordance: a background click deselects. + expect(selections).toEqual([null]); + // The whitelisted key really arrives — this is the half the old spread got + // right and a naive `toDomProps` migration would have dropped. + expect(hostClicks).toEqual(['host']); + }); + + it('a non-function onClick is ignored instead of handed to React', () => { + // SDUI spells click behaviour `events: { onClick: [ActionDef] }`, which is + // DATA and is dropped by the whitelist. An authored literal `onClick` + // string used to reach React through the spread and throw. + const selections: Array = []; + + const { container } = render( + selections.push(id)} + onClick={'navigate' as never} + />, + ); + + const grid = container.querySelector('.grid'); + if (!grid) throw new Error('the widget grid never rendered'); + expect(() => fireEvent.click(grid)).not.toThrow(); + expect(selections).toEqual([null]); + }); +});