diff --git a/.changeset/10496-header-bar-root-dom-props.md b/.changeset/10496-header-bar-root-dom-props.md new file mode 100644 index 0000000000..8e5afa7b71 --- /dev/null +++ b/.changeset/10496-header-bar-root-dom-props.md @@ -0,0 +1,34 @@ +--- +'@object-ui/components': patch +'@object-ui/types': patch +--- + +fix(components): `header-bar` puts its `BaseSchema` DOM channels on the root `header` (objectui#10496) + +The `header-bar` renderer was a function of the node alone, so the props every +component receives beside it never reached the element. An `ariaLabel` the author +declared ("Rendered as aria-label attribute") never became the banner landmark's +accessible name, and `style`, `id`, `testId` and the `data-obj-id` / +`data-obj-type` markers rendered exactly as if they had not been written. +objectui#10397 had made the root honour `className`, and nothing else arrived. + +The root now takes the same route as the other converged renderers: what it is +handed goes through `toDomProps`, the shared DOM whitelist in `@object-ui/core`, +and `style` is forwarded by name. So `ariaLabel` names the landmark, and `style`, +`id`, `data-testid` and `data-obj-*` land on the `header`. The keys the renderer +consumes (`crumbs`, `search`, `actions`, `rightContent`) and any key an author +invents stay off the DOM. `className` is unchanged: it is still merged after the +header's own classes. + +What an author sees change: + +- a `header-bar` with `ariaLabel` is announced by that name; +- `style`, `id` and `testId` now apply to the header; +- every rendered header carries `data-obj-type="header-bar"`, like other nodes. + +`@object-ui/types`: the refusal messages and doc comments for the retired +`header-bar` keys (`title`, `logo`, `nav`, `left`, `center`, `right`, `sticky`, +`height`, `variant`) said the renderer "takes no spread props". They now say what +it does: it forwards to its root only what the shared DOM whitelist admits, plus +`style`. None of those keys is on that whitelist, so none of them reaches the DOM, +and each is still refused by name. diff --git a/packages/components/src/__tests__/header-bar-classname-10397.test.tsx b/packages/components/src/__tests__/header-bar-classname-10397.test.tsx index f5d09bdc16..d790a6d11b 100644 --- a/packages/components/src/__tests__/header-bar-classname-10397.test.tsx +++ b/packages/components/src/__tests__/header-bar-classname-10397.test.tsx @@ -112,9 +112,18 @@ describe('ui:header-bar merges the authored className onto its root (objectui#10 expect(classes).toContain(scopeClassFor('hb-10397')); }); - it('control: without className the root is exactly what it was before the repair', () => { + it('control: without className the root class is exactly what it was before the repair, beside `data-obj-type` only', () => { const header = headerFor({}); expect(header.getAttribute('class')).toBe(PRE_REPAIR_ROOT_CLASS); - expect(header.getAttributeNames()).toEqual(['class']); + // Was exactly `['class']` until objectui#10496 routed the root's props + // through `toDomProps`. `SchemaRenderer` hands every component + // `data-obj-type` unconditionally, and the whitelist forwards the open + // `data-*` family, so it is the one attribute that joins on an unauthored + // node. The other channels it now forwards (`id`, `data-obj-id`, + // `aria-label`, `data-testid`, `style`) arrive only when authored, and are + // pinned in `header-bar-root-dom-props-10496.test.tsx`. Sorted, because + // the order React sets attributes in is not the contract. + expect([...header.getAttributeNames()].sort()).toEqual(['class', 'data-obj-type']); + expect(header.getAttribute('data-obj-type')).toBe('header-bar'); }); }); diff --git a/packages/components/src/__tests__/header-bar-root-dom-props-10496.test.tsx b/packages/components/src/__tests__/header-bar-root-dom-props-10496.test.tsx new file mode 100644 index 0000000000..738a76e5a5 --- /dev/null +++ b/packages/components/src/__tests__/header-bar-root-dom-props-10496.test.tsx @@ -0,0 +1,145 @@ +/** + * 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. + */ + +/** + * `ui:header-bar` delivers the `BaseSchema` DOM channels to its root `header` + * (objectui#10496). + * + * `SchemaRenderer` hands every registered component `aria-label` (resolved from + * `ariaLabel`), `data-obj-id` / `data-obj-type`, a conditional `data-testid` + * (from `testId`), `style` and `id` as props. The renderer was registered as a + * function of `schema` alone, so all of them were dropped: an accessible name + * the author declared never reached the landmark, and `style` / `id` rendered + * byte-identical to their absence. objectui#10397 had already made the root + * honour `className`; nothing else arrived. + * + * The root now takes the converged route every other converged renderer in + * this package takes: `toDomProps` (the SDUI whitelist in `@object-ui/core`) + * over the props it is handed, plus `style` forwarded by name. So the rows + * below come in two halves, and both are needed: + * + * - the DECLARED channels reach the root, one key per row; + * - an UNDECLARED key, and the keys the renderer consumes off `schema`, still + * do not reach the DOM. This is what tells the whitelist apart from a bare + * `{...props}` spread, which would turn every row in the first half green + * as well. + * + * Every row renders through the real `SchemaRenderer` and the real registry, + * the path an authored JSON node takes, inside the `SidebarProvider` host + * `SidebarTrigger` needs (see `header-bar-classname-10397.test.tsx`). + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { render, cleanup, screen } from '@testing-library/react'; +import { SchemaRenderer } from '@object-ui/react'; +// Registered at module scope, NOT in a `beforeAll`: there the cold transform is +// billed to `hookTimeout`, which is narrower than the timeout it replaces +// (objectui#3010 / #3021, and `object-ui/no-dynamic-import-in-test-hook`). +import '../renderers'; +import { SidebarProvider } from '../ui'; + +afterEach(() => cleanup()); + +const LABEL = 'Workspace header 10496'; +const ID = 'hb-10496'; +const TEST_ID = 'hb-testid-10496'; +/** Not a colour: AGENTS.md keeps colour off inline styles, and any property proves the channel. */ +const STYLE = { minWidth: '12rem' }; +/** A key no surface declares, with a value no other part of the tree prints. */ +const UNDECLARED_KEY = 'zzUndeclared10496'; +const UNDECLARED_VALUE = 'zz-leak-10496'; + +function mount(schema: Record): { header: HTMLElement; container: HTMLElement } { + const { container } = render( + + + , + ); + const header = container.querySelector('header'); + if (!header) throw new Error('no header element: the harness did not mount the real renderer'); + return { header, container }; +} + +describe('ui:header-bar delivers the BaseSchema DOM channels to its root (objectui#10496)', () => { + it('harness: the real header mounts, with its sidebar trigger inside it', () => { + const { header } = mount({}); + expect(header.querySelector('[data-sidebar="trigger"]')).not.toBeNull(); + }); + + it('`ariaLabel` becomes the banner landmark\'s accessible name', () => { + const { header } = mount({ ariaLabel: LABEL }); + expect(screen.getByRole('banner', { name: LABEL })).toBe(header); + expect(header.getAttribute('aria-label')).toBe(LABEL); + }); + + it('`style` reaches the root', () => { + const { header } = mount({ style: STYLE }); + expect(header.style.minWidth).toBe(STYLE.minWidth); + }); + + it('`id` reaches the root', () => { + const { header } = mount({ id: ID }); + expect(header.id).toBe(ID); + }); + + it('`testId` reaches the root as `data-testid`', () => { + const { header } = mount({ testId: TEST_ID }); + expect(header.getAttribute('data-testid')).toBe(TEST_ID); + }); + + it('`data-obj-id` / `data-obj-type` reach the root', () => { + const { header } = mount({ id: ID }); + expect(header.getAttribute('data-obj-id')).toBe(ID); + expect(header.getAttribute('data-obj-type')).toBe('header-bar'); + }); + + it('each declared channel lands ONCE, on the root and on no other element', () => { + const { header, container } = mount({ id: ID, ariaLabel: LABEL, testId: TEST_ID }); + for (const selector of [`[aria-label="${LABEL}"]`, `[id="${ID}"]`, `[data-testid="${TEST_ID}"]`]) { + const hits = container.querySelectorAll(selector); + expect(hits, selector).toHaveLength(1); + expect(hits[0], selector).toBe(header); + } + }); + + it('control: an undeclared key and the consumed keys do not reach the DOM', () => { + // Absence only, so the row does not depend on the wiring the rows above + // pin: with the renderer reading `schema` alone it is green too. What it + // tells apart is the whitelist from a bare `{...props}` spread, under which + // every one of these keys lands on the root and this row goes red. + const { header, container } = mount({ + ariaLabel: LABEL, + [UNDECLARED_KEY]: UNDECLARED_VALUE, + crumbs: [{ label: 'Home', href: '#' }, { label: 'Here' }], + search: { enabled: true, placeholder: 'Find' }, + }); + const names = header.getAttributeNames(); + expect(names).not.toContain(UNDECLARED_KEY.toLowerCase()); + // Consumed off `schema`, never forwarded; and the camelCase `ariaLabel` the + // author wrote is not an attribute, only the resolved `aria-label` is. + for (const consumed of ['crumbs', 'search', 'type', 'arialabel', 'testid']) { + expect(names, consumed).not.toContain(consumed); + } + expect(container.innerHTML).not.toContain(UNDECLARED_VALUE); + }); + + it('with every channel authored, the root carries exactly these attributes', () => { + const { header } = mount({ + id: ID, + ariaLabel: LABEL, + testId: TEST_ID, + style: STYLE, + className: 'zz-header-bar-10496', + [UNDECLARED_KEY]: UNDECLARED_VALUE, + }); + // Sorted: which order React sets attributes in is not the contract. + expect([...header.getAttributeNames()].sort()).toEqual( + ['aria-label', 'class', 'data-obj-id', 'data-obj-type', 'data-testid', 'id', 'style'], + ); + }); +}); diff --git a/packages/components/src/renderers/navigation/header-bar.tsx b/packages/components/src/renderers/navigation/header-bar.tsx index 032e4a3245..ce3850854a 100644 --- a/packages/components/src/renderers/navigation/header-bar.tsx +++ b/packages/components/src/renderers/navigation/header-bar.tsx @@ -7,7 +7,7 @@ */ import React from 'react'; -import { ComponentRegistry } from '@object-ui/core'; +import { ComponentRegistry, toDomProps } from '@object-ui/core'; import type { HeaderBarSchema, BreadcrumbItem as BreadcrumbItemType } from '@object-ui/types'; import { resolveKeyedI18nLabel, SchemaRenderer, toRenderableSchema } from '@object-ui/react'; import { @@ -57,8 +57,24 @@ import { cn } from '../../lib/utils'; // with the same value (its `responsiveStyles` scope class included), so that a // renderer honours ONE of them. tailwind-merge does not collapse a repeated // non-Tailwind class, so reading both would print every custom class twice. +// The prop is therefore taken off the pass-through below by name and left +// unread: `className` is on `toDomProps`' whitelist, so it would otherwise +// ride the spread as a second copy of the same channel. const HEADER_BAR_CLASS = 'flex h-14 sm:h-16 shrink-0 items-center gap-2 border-b px-3 sm:px-4'; +/** + * What `SchemaRenderer` hands the registered component: the node on `schema`, + * and beside it the node's own keys, the resolved ARIA (`aria-label` from + * `ariaLabel`), `data-obj-id` / `data-obj-type`, a conditional `data-testid` + * (from `testId`), `style` and `id` as props. + */ +type HeaderBarRendererProps = { + schema: HeaderBarSchema; + className?: string; + style?: React.CSSProperties; + [key: string]: unknown; +}; + function BreadcrumbLabel({ crumb, isLast }: { crumb: BreadcrumbItemType; isLast: boolean }) { const label = resolveKeyedI18nLabel(crumb.label) ?? ''; @@ -90,9 +106,23 @@ function BreadcrumbLabel({ crumb, isLast }: { crumb: BreadcrumbItemType; isLast: return {label}; } -ComponentRegistry.register('header-bar', - ({ schema }: { schema: HeaderBarSchema }) => ( -
+// The root's DOM channels take the converged route (objectui#10496): the +// props `SchemaRenderer` hands this renderer go through `toDomProps`, the SDUI +// whitelist in `@object-ui/core` (`id`, `role`, the open `aria-*` / `data-*` +// families, …), and `style` is forwarded BY NAME, as `grid.tsx`, `box.tsx` and +// the other converged renderers do (the objectui#4435 route). Until then this +// was a function of `schema` alone, so an authored `ariaLabel` never named the +// banner landmark and `style`, `id`, `testId` and `data-obj-*` never reached +// the element. The keys this renderer consumes off `schema` (`crumbs`, +// `search`, `actions`, `rightContent`) and any key an author invents stay off +// the DOM, because the whitelist drops them. +ComponentRegistry.register('header-bar', + ({ schema, className: _classNameProp, style, ...hostProps }: HeaderBarRendererProps) => ( +
diff --git a/packages/types/src/navigation.ts b/packages/types/src/navigation.ts index 4d53b7fb50..2e25d1061d 100644 --- a/packages/types/src/navigation.ts +++ b/packages/types/src/navigation.ts @@ -62,7 +62,7 @@ export interface HeaderBarSchema extends BaseSchema { * Not in `@objectstack/spec`, so the objectui#7759 ruling makes the read * site the truth, and there is none: the renderer's one function reads * `actions`, `crumbs`, `rightContent`, `search` and the inherited - * `className` off `schema` and takes no spread props. Through the real `SchemaRenderer` an authored title drew the + * `className` off `schema`, and forwards to its root only what the shared DOM whitelist (`toDomProps`) admits plus `style` (objectui#10496). Through the real `SchemaRenderer` an authored title drew the * header byte-identical to its absence. For the current page name, use the * last entry of `crumbs`. * @@ -86,7 +86,7 @@ export interface HeaderBarSchema extends BaseSchema { * Not in `@objectstack/spec`, so the objectui#7759 ruling makes the read * site the truth, and there is none: the renderer's one function reads * `actions`, `crumbs`, `rightContent`, `search` and the inherited - * `className` off `schema` and takes no spread props. Through the real `SchemaRenderer` an authored link list drew + * `className` off `schema`, and forwards to its root only what the shared DOM whitelist (`toDomProps`) admits plus `style` (objectui#10496). Through the real `SchemaRenderer` an authored link list drew * the header byte-identical to its absence. No in-tree document authored it. * For links, use `crumbs` here, or a `navigation-menu` / `sidebar` node. * @@ -113,8 +113,8 @@ export interface HeaderBarSchema extends BaseSchema { * RETIRED (objectui#10387, ADR-0049) — `header-bar` has no left slot. * * Not in `@objectstack/spec`; the renderer reads only `actions`, `crumbs`, - * `rightContent`, `search` and the inherited `className`, and takes no spread - * props, so an authored node rendered nothing. No in-tree document authored it. For custom content use + * `rightContent`, `search` and the inherited `className`, and forwards to its root only what the shared DOM whitelist (`toDomProps`) admits plus `style` (objectui#10496), + * so an authored node rendered nothing. No in-tree document authored it. For custom content use * `rightContent` or `actions`. * * @deprecated Nothing renders it; the zod mirror refuses it by name. @@ -166,7 +166,7 @@ export interface HeaderBarSchema extends BaseSchema { * The key is not in `@objectstack/spec`, so the objectui#7759 ruling makes * the read site the truth, and the read site has none: the renderer's one * function reads `actions`, `crumbs`, `rightContent`, `search` and the - * inherited `className` off `schema` and takes no spread props, so every + * inherited `className` off `schema`, and forwards to its root only what the shared DOM whitelist (`toDomProps`) admits plus `style` (objectui#10496), so every * spelling rendered the same header. The two faces had also drifted apart on the way there — this * declaration offered `floating`, the zod mirror `transparent` — and * neither word had ever reached a class name. diff --git a/packages/types/src/zod/navigation.zod.ts b/packages/types/src/zod/navigation.zod.ts index 94e15fd1e8..724271770e 100644 --- a/packages/types/src/zod/navigation.zod.ts +++ b/packages/types/src/zod/navigation.zod.ts @@ -65,21 +65,21 @@ export const HeaderBarSchema = BaseSchema.extend({ title: retirementTombstone( 'REFUSED (objectui#10387, ADR-0049) — `header-bar` reads no `title`: the key is not in ' + '`@objectstack/spec`, and its renderer reads only `actions`, `crumbs`, `rightContent`, `search` and ' - + 'the inherited `className` off the node and takes no spread props, so an authored title rendered nothing — ' + + 'the inherited `className` off the node, and forwards to its root only what the shared DOM whitelist admits plus `style`, so an authored title rendered nothing — ' + 'no error, no warning, no element. What it renders instead: `actions`, `crumbs`, ' + '`rightContent`, `search`. For the current page name use the last entry of `crumbs`.', ), logo: retirementTombstone( 'REFUSED (objectui#10387, ADR-0049) — `header-bar` reads no `logo`: the key is not in ' + '`@objectstack/spec`, and its renderer reads only `actions`, `crumbs`, `rightContent`, `search` and ' - + 'the inherited `className` off the node and takes no spread props, so an authored logo (URL or node) rendered nothing — ' + + 'the inherited `className` off the node, and forwards to its root only what the shared DOM whitelist admits plus `style`, so an authored logo (URL or node) rendered nothing — ' + 'no error, no warning, no element. What it renders instead: `actions`, `crumbs`, ' + '`rightContent`, `search`. For brand content use `rightContent` or `actions`.', ), nav: retirementTombstone( 'REFUSED (objectui#10387, ADR-0049) — `header-bar` reads no `nav`: the key is not in ' + '`@objectstack/spec`, and its renderer reads only `actions`, `crumbs`, `rightContent`, `search` and ' - + 'the inherited `className` off the node and takes no spread props, so an authored link list rendered nothing — ' + + 'the inherited `className` off the node, and forwards to its root only what the shared DOM whitelist admits plus `style`, so an authored link list rendered nothing — ' + 'no error, no warning, no element. What it renders instead: `actions`, `crumbs`, ' + '`rightContent`, `search`. For links use `crumbs`, or a `navigation-menu` / `sidebar` node.', ), @@ -94,35 +94,35 @@ export const HeaderBarSchema = BaseSchema.extend({ left: retirementTombstone( 'REFUSED (objectui#10387, ADR-0049) — `header-bar` reads no `left`: the key is not in ' + '`@objectstack/spec`, and its renderer reads only `actions`, `crumbs`, `rightContent`, `search` and ' - + 'the inherited `className` off the node and takes no spread props, so an authored node rendered nothing — ' + + 'the inherited `className` off the node, and forwards to its root only what the shared DOM whitelist admits plus `style`, so an authored node rendered nothing — ' + 'no error, no warning, no element. What it renders instead: `actions`, `crumbs`, ' + '`rightContent`, `search`. For custom content use `rightContent` or `actions`.', ), center: retirementTombstone( 'REFUSED (objectui#10387, ADR-0049) — `header-bar` reads no `center`: the key is not in ' + '`@objectstack/spec`, and its renderer reads only `actions`, `crumbs`, `rightContent`, `search` and ' - + 'the inherited `className` off the node and takes no spread props, so an authored node rendered nothing — ' + + 'the inherited `className` off the node, and forwards to its root only what the shared DOM whitelist admits plus `style`, so an authored node rendered nothing — ' + 'no error, no warning, no element. What it renders instead: `actions`, `crumbs`, ' + '`rightContent`, `search`. For custom content use `rightContent` or `actions`.', ), right: retirementTombstone( 'REFUSED (objectui#10387, ADR-0049) — `header-bar` reads no `right`: the key is not in ' + '`@objectstack/spec`, and its renderer reads only `actions`, `crumbs`, `rightContent`, `search` and ' - + 'the inherited `className` off the node and takes no spread props, so an authored node rendered nothing — ' + + 'the inherited `className` off the node, and forwards to its root only what the shared DOM whitelist admits plus `style`, so an authored node rendered nothing — ' + 'no error, no warning, no element. What it renders instead: `actions`, `crumbs`, ' + '`rightContent`, `search`. The right side is `actions` (a node list) and `rightContent` (one node).', ), sticky: retirementTombstone( 'REFUSED (objectui#10387, ADR-0049) — `header-bar` reads no `sticky`: the key is not in ' + '`@objectstack/spec`, and its renderer reads only `actions`, `crumbs`, `rightContent`, `search` and ' - + 'the inherited `className` off the node and takes no spread props, so true and false drew the same non-sticky header — ' + + 'the inherited `className` off the node, and forwards to its root only what the shared DOM whitelist admits plus `style`, so true and false drew the same non-sticky header — ' + 'no error, no warning, no element. What it renders instead: `actions`, `crumbs`, ' + '`rightContent`, `search`. To pin the header, make its parent layout sticky.', ), height: retirementTombstone( 'REFUSED (objectui#10387, ADR-0049) — `header-bar` reads no `height`: the key is not in ' + '`@objectstack/spec`, and its renderer reads only `actions`, `crumbs`, `rightContent`, `search` and ' - + 'the inherited `className` off the node and takes no spread props, so every value drew the same default-height header — ' + + 'the inherited `className` off the node, and forwards to its root only what the shared DOM whitelist admits plus `style`, so every value drew the same default-height header — ' + 'no error, no warning, no element. What it renders instead: `actions`, `crumbs`, ' + '`rightContent`, `search`. To change the height, author `className` (e.g. `h-20 sm:h-20`); ' + 'it is merged after the default `h-14` / `sm:h-16` the renderer sets.', @@ -130,7 +130,7 @@ export const HeaderBarSchema = BaseSchema.extend({ variant: retirementTombstone( 'REFUSED (objectui#10286, ADR-0049) — `header-bar` reads no `variant`: the key is not in ' + '`@objectstack/spec`, and its renderer reads only `actions`, `crumbs`, `rightContent`, `search` and ' - + 'the inherited `className` off the node and takes no spread props, so every variant rendered the same header — ' + + 'the inherited `className` off the node, and forwards to its root only what the shared DOM whitelist admits plus `style`, so every variant rendered the same header — ' + 'no error, no warning, no class. What it renders instead: `actions`, `crumbs`, `rightContent`, ' + '`search`.', ),