Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions .changeset/10496-header-bar-root-dom-props.md
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
});
Original file line number Diff line number Diff line change
@@ -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<string, unknown>): { header: HTMLElement; container: HTMLElement } {
const { container } = render(
<SidebarProvider>
<SchemaRenderer schema={{ type: 'header-bar', ...schema } as never} />
</SidebarProvider>,
);
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'],
);
});
});
38 changes: 34 additions & 4 deletions packages/components/src/renderers/navigation/header-bar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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) ?? '';

Expand Down Expand Up @@ -90,9 +106,23 @@ function BreadcrumbLabel({ crumb, isLast }: { crumb: BreadcrumbItemType; isLast:
return <BreadcrumbLink href={crumb.href || '#'}>{label}</BreadcrumbLink>;
}

ComponentRegistry.register('header-bar',
({ schema }: { schema: HeaderBarSchema }) => (
<header className={cn(HEADER_BAR_CLASS, schema.className)}>
// 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) => (
<header
{...toDomProps(hostProps)}
className={cn(HEADER_BAR_CLASS, schema.className)}
style={style}
>
<SidebarTrigger />
<Separator orientation="vertical" className="mr-2 h-4" />
<Breadcrumb>
Expand Down
10 changes: 5 additions & 5 deletions packages/types/src/navigation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
*
Expand All @@ -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.
*
Expand All @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
Loading
Loading