|
| 1 | +/** |
| 2 | + * ObjectUI |
| 3 | + * Copyright (c) 2024-present ObjectStack Inc. |
| 4 | + * |
| 5 | + * This source code is licensed under the MIT license found in the |
| 6 | + * LICENSE file in the root directory of this source tree. |
| 7 | + */ |
| 8 | + |
| 9 | +/** |
| 10 | + * `ui:grid` forwards to the DOM by WHITELIST (objectui#4787). |
| 11 | + * |
| 12 | + * ## What is being observed, and why the obvious test is worthless here |
| 13 | + * |
| 14 | + * The observable is the RENDERED DOM, not the React tree. `SchemaRenderer` spreads |
| 15 | + * the authored node onto the registered component, and `grid.tsx` used to end in a |
| 16 | + * bare `{...gridProps}` spread onto its `<div>`, so the node's own vocabulary landed |
| 17 | + * as invalid HTML attributes. Measured on `origin/main` with the canary node below: |
| 18 | + * |
| 19 | + * ``` |
| 20 | + * <div class="grid grid-cols-4 …" columns="4" gap="4" mdcolumns="2" smcolumns="2" |
| 21 | + * id="grid-node" name="grid_node" props="[object Object]" zzcanary="leak" |
| 22 | + * data-testid="g" aria-label="A grid" role="region" colorvariant="x" |
| 23 | + * data-obj-id="grid-node" data-obj-type="grid"></div> |
| 24 | + * ``` |
| 25 | + * |
| 26 | + * Every one of those renders. "The grid renders" is GREEN against the broken code — |
| 27 | + * every catalog grid example rendered today, wrong attributes and all. So the |
| 28 | + * assertion has to be that the offending attributes are ABSENT from the element. |
| 29 | + * |
| 30 | + * ## Why case 1 sweeps every attribute instead of naming the bad ones |
| 31 | + * |
| 32 | + * Naming `columns` / `gap` / `mdcolumns` would pass just as well against a renderer |
| 33 | + * that strips those four and keeps leaking the rest, and it would re-rot the moment |
| 34 | + * `GridSchema` grows a key — the exact staleness this defect IS. It also could never |
| 35 | + * reach the OPEN TAIL: `zzcanary` and the flattened `props` container are |
| 36 | + * author-supplied, so no finite list of schema keys names them. Case 1 therefore |
| 37 | + * asserts the whole attribute set against what the contract DECLARES as DOM-safe, so |
| 38 | + * a key added to `GridSchema` tomorrow is covered without editing this file. |
| 39 | + * |
| 40 | + * ## The positive control is load-bearing |
| 41 | + * |
| 42 | + * A renderer that stripped EVERYTHING would satisfy the absence assertions on its |
| 43 | + * own. Case 2 pins what must still arrive — the computed grid classes, the authored |
| 44 | + * `className` merged with them, `id`, `role`, the `data-*` / `aria-*` families |
| 45 | + * (including the designer's `data-obj-id` / `data-obj-type`), the forwarded `style`, |
| 46 | + * and the children — so "strips everything" fails here even while case 1 passes. |
| 47 | + */ |
| 48 | + |
| 49 | +import { describe, it, expect, vi } from 'vitest'; |
| 50 | +import { render } from '@testing-library/react'; |
| 51 | +import { SchemaRenderer } from '@object-ui/react'; |
| 52 | +// Registered at module scope, NOT in a `beforeAll` — there the cold transform is |
| 53 | +// billed to the narrower `hookTimeout` (AGENTS.md 测试纪律, objectui#3010/#3021). |
| 54 | +import '../renderers'; |
| 55 | + |
| 56 | +/** |
| 57 | + * Attribute names legal on the `<div>` this renderer produces: the exact keys |
| 58 | + * `@object-ui/core`'s SDUI pass-through set declares (lowercased, as the DOM stores |
| 59 | + * them — `className` becomes `class`, `tabIndex` becomes `tabindex`), plus `style`, |
| 60 | + * which `grid.tsx` forwards by name as its designer sizing channel. |
| 61 | + */ |
| 62 | +const ALLOWED_EXACT = new Set(['class', 'id', 'role', 'tabindex', 'autofocus', 'style']); |
| 63 | + |
| 64 | +/** `data-*` and `aria-*` are open families in HTML and in the widget contract. */ |
| 65 | +const isAllowedAttribute = (name: string): boolean => |
| 66 | + ALLOWED_EXACT.has(name) || name.startsWith('data-') || name.startsWith('aria-'); |
| 67 | + |
| 68 | +/** |
| 69 | + * A grid node carrying, deliberately, one of every category the leak drew from: |
| 70 | + * the renderer's own vocabulary in both `columns` spellings, SDUI node metadata, |
| 71 | + * the `props` container, an unknown authored key, and the legitimately forwarded |
| 72 | + * DOM channels. |
| 73 | + */ |
| 74 | +const CANARY_NODE = { |
| 75 | + type: 'grid', |
| 76 | + // Renderer vocabulary — CONSUMED off `schema`, never forwarded. |
| 77 | + columns: 4, |
| 78 | + gap: 4, |
| 79 | + smColumns: 2, |
| 80 | + mdColumns: 2, |
| 81 | + lgColumns: 3, |
| 82 | + xlColumns: 6, |
| 83 | + // SDUI node metadata `SchemaRenderer` hands down. |
| 84 | + name: 'grid_node', |
| 85 | + // The `props` container, whose contents get flattened onto the component. |
| 86 | + props: { colorVariant: 'x' }, |
| 87 | + // The open tail: an authored key nothing declares. |
| 88 | + zzcanary: 'leak', |
| 89 | + // Legitimate DOM channels — the positive control. |
| 90 | + id: 'grid-node', |
| 91 | + className: 'authored-class', |
| 92 | + role: 'region', |
| 93 | + 'aria-label': 'A grid', |
| 94 | + 'data-testid': 'grid-el', |
| 95 | + children: [], |
| 96 | +} as const; |
| 97 | + |
| 98 | +function renderCanary(overrides: Record<string, unknown> = {}) { |
| 99 | + const { container } = render( |
| 100 | + <SchemaRenderer schema={{ ...CANARY_NODE, ...overrides } as never} />, |
| 101 | + ); |
| 102 | + const el = container.firstElementChild as HTMLElement; |
| 103 | + expect(el, 'the canary grid must render at all').not.toBeNull(); |
| 104 | + return el; |
| 105 | +} |
| 106 | + |
| 107 | +const attributeNames = (el: HTMLElement) => Array.from(el.attributes).map((a) => a.name); |
| 108 | + |
| 109 | +describe('ui:grid — schema keys never reach the DOM (objectui#4787)', () => { |
| 110 | + it('case 1: no attribute outside the declared DOM-safe set survives', () => { |
| 111 | + const el = renderCanary(); |
| 112 | + |
| 113 | + // The whole set, judged against the contract rather than against a list of |
| 114 | + // today's bad keys. The failure message carries the leaked names. |
| 115 | + expect(attributeNames(el).filter((n) => !isAllowedAttribute(n))).toEqual([]); |
| 116 | + |
| 117 | + // The named regressions from the card, spelled out so a future failure reads as |
| 118 | + // this defect rather than as an anonymous set difference. `mdColumns` reaches the |
| 119 | + // DOM lowercased (HTML attribute names are case-insensitive), which is why the |
| 120 | + // card records it as `mdcolumns`. |
| 121 | + for (const leaked of [ |
| 122 | + 'columns', 'gap', 'smcolumns', 'mdcolumns', 'lgcolumns', 'xlcolumns', |
| 123 | + 'name', 'props', 'zzcanary', 'colorvariant', |
| 124 | + ]) { |
| 125 | + expect(el.hasAttribute(leaked), `"${leaked}" must not reach the DOM`).toBe(false); |
| 126 | + } |
| 127 | + |
| 128 | + // The headline symptom: an object stringified onto an attribute. Assert on the |
| 129 | + // serialized element so this cannot pass by the attribute merely being renamed. |
| 130 | + expect(el.outerHTML).not.toContain('[object Object]'); |
| 131 | + }); |
| 132 | + |
| 133 | + it('case 1b: a responsive `columns` OBJECT does not become columns="[object Object]"', () => { |
| 134 | + // The flat `smColumns`/`mdColumns`/… keys deliberately OVERRIDE the object form |
| 135 | + // in this renderer, so they are cleared here to exercise the object path itself. |
| 136 | + const el = renderCanary({ |
| 137 | + columns: { xs: 1, md: 3 }, |
| 138 | + smColumns: undefined, mdColumns: undefined, lgColumns: undefined, xlColumns: undefined, |
| 139 | + }); |
| 140 | + |
| 141 | + expect(el.hasAttribute('columns')).toBe(false); |
| 142 | + expect(el.outerHTML).not.toContain('[object Object]'); |
| 143 | + expect(attributeNames(el).filter((n) => !isAllowedAttribute(n))).toEqual([]); |
| 144 | + // Still CONSUMED: the object form drives the classes it always did. |
| 145 | + expect(el.className).toContain('grid-cols-1'); |
| 146 | + expect(el.className).toContain('md:grid-cols-3'); |
| 147 | + }); |
| 148 | + |
| 149 | + it('case 2: positive control — everything legitimately forwarded still arrives', () => { |
| 150 | + const el = renderCanary({ |
| 151 | + style: { minHeight: '10px' }, |
| 152 | + children: [{ type: 'text', content: 'child-marker' }], |
| 153 | + }); |
| 154 | + |
| 155 | + // Computed layout classes — the renderer's real output, merged with the |
| 156 | + // authored className. A filter that dropped `className` would fail here. |
| 157 | + expect(el.className).toContain('grid'); |
| 158 | + expect(el.className).toContain('sm:grid-cols-2'); |
| 159 | + expect(el.className).toContain('md:grid-cols-2'); |
| 160 | + expect(el.className).toContain('gap-4'); |
| 161 | + expect(el.className).toContain('authored-class'); |
| 162 | + |
| 163 | + // Declared global attributes. |
| 164 | + expect(el.getAttribute('id')).toBe('grid-node'); |
| 165 | + expect(el.getAttribute('role')).toBe('region'); |
| 166 | + |
| 167 | + // The two open families, including the designer channel that used to be |
| 168 | + // hand-forwarded and now arrives through the `data-*` prefix rule. |
| 169 | + expect(el.getAttribute('aria-label')).toBe('A grid'); |
| 170 | + expect(el.getAttribute('data-testid')).toBe('grid-el'); |
| 171 | + expect(el.getAttribute('data-obj-id')).toBe('grid-node'); |
| 172 | + expect(el.getAttribute('data-obj-type')).toBe('grid'); |
| 173 | + |
| 174 | + // `style`, forwarded by name. |
| 175 | + expect(el.style.minHeight).toBe('10px'); |
| 176 | + |
| 177 | + // And the container still contains things. |
| 178 | + expect(el.textContent).toContain('child-marker'); |
| 179 | + }); |
| 180 | + |
| 181 | + /** |
| 182 | + * The card's side-question, pinned as far as it can honestly be pinned. |
| 183 | + * |
| 184 | + * React DOES warn here — but only for the CAMEL-CASE keys (`mdColumns`, |
| 185 | + * `smColumns`, `colorVariant`): "React does not recognize the `%s` prop on a DOM |
| 186 | + * element. If you intentionally want it to appear in the DOM as a custom |
| 187 | + * attribute, spell it as lowercase `%s` instead." Measured on `origin/main`: eight |
| 188 | + * attributes leaked, three `console.error` calls. The all-lowercase ones — |
| 189 | + * `columns`, `gap`, `name`, `props`, `zzcanary`, and both attributes the card's |
| 190 | + * title names — produce NO warning at all, because React passes unknown lowercase |
| 191 | + * attributes through by design and stringifies object values silently. |
| 192 | + * |
| 193 | + * So this case is the WEAKER half and is kept only as a noise-floor ratchet on this |
| 194 | + * renderer: it can catch a camelCase key being reopened, and it is blind to the |
| 195 | + * majority of the very defect it accompanies. Case 1 is what actually closes the |
| 196 | + * class, because it reads the DOM instead of the console. Do not let this case's |
| 197 | + * green be read as coverage. |
| 198 | + * |
| 199 | + * Note it must SPY rather than rely on the run's output: Vitest 4's reporter |
| 200 | + * defaults to `silent: 'passed-only'`, so console output from a passing test is |
| 201 | + * discarded — which is the third reason this warning could never have turned a |
| 202 | + * test red on its own. See the PR body for the full answer. |
| 203 | + */ |
| 204 | + it('case 3: React emits no unknown-prop warning for this renderer any more', () => { |
| 205 | + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); |
| 206 | + try { |
| 207 | + renderCanary(); |
| 208 | + const unknownPropWarnings = errSpy.mock.calls.filter((args) => |
| 209 | + /does not recognize the .* prop on a DOM element/.test(String(args[0])), |
| 210 | + ); |
| 211 | + expect(unknownPropWarnings).toEqual([]); |
| 212 | + } finally { |
| 213 | + errSpy.mockRestore(); |
| 214 | + } |
| 215 | + }); |
| 216 | +}); |
0 commit comments