diff --git a/.changeset/page-single-h1-3434.md b/.changeset/page-single-h1-3434.md new file mode 100644 index 0000000000..febd5e4405 --- /dev/null +++ b/.changeset/page-single-h1-3434.md @@ -0,0 +1,9 @@ +--- +'@object-ui/components': patch +--- + +`PageRenderer` no longer renders its own `

` when the page authors a titled `page:header`, so a page has exactly one level-1 heading. Every non-record page used to render the page `title`/`label` as an `h1` *and* let the `page:header` block render a second one — on the showcase master-detail page both said "New Project + Tasks", producing a broken document outline, a page title a screen reader announces twice, and the same string printed twice on screen. Record pages already delegated the whole title block to `page:header`; that rule now holds for `app` / `home` / `utility` pages too, and it is what the live e2e was reporting as a Playwright strict-mode violation (`getByRole('heading', { name })` resolving to 2 elements, objectui#3434). + +Delegation is deliberately conservative: only a `page:header` whose title renders literal text takes the heading over. A header with no title — or one whose title interpolates to nothing (e.g. `title: '{name}'` with no record in scope) — renders no heading of its own, so the page keeps its implicit `h1` rather than ending up with none. The page-level `description` is unaffected; it is the page's own prose, not a duplicate of the header `subtitle`. + +Author-visible effect: on a page carrying both a `label` and a titled `page:header`, only the header's title is shown (e.g. app-crm's welcome page shows "Welcome to the CRM", not also "CRM Welcome"). Pages without a `page:header` are unchanged. diff --git a/packages/components/src/__tests__/page-single-h1.test.tsx b/packages/components/src/__tests__/page-single-h1.test.tsx new file mode 100644 index 0000000000..39cc18eedd --- /dev/null +++ b/packages/components/src/__tests__/page-single-h1.test.tsx @@ -0,0 +1,150 @@ +/** + * 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. + * + * "One page, one h1" — accessibility contract for PageRenderer (objectui#3434). + * + * Two components can render the page's title: PageRenderer's own implicit + * heading (from `title`/`label`) and the authored `page:header` block. Before + * this guard they BOTH rendered on every non-record page, so the showcase + * master-detail page shipped two `heading level 1` nodes with the identical + * accessible name — a broken document outline, a title a screen reader + * announces twice, and the same string printed twice on screen. The live e2e + * caught it as a Playwright strict-mode violation: + * `getByRole('heading', { name: 'New Project + Tasks' })` resolved to 2 + * elements, taking `e2e/live/master-detail.spec.ts` down in `beforeEach`. + * + * The assertions below are written against the ACCESSIBLE TREE (`getAllByRole` + * with an explicit level), not against class names, so they pin the fact the + * e2e locator actually depends on. The negative controls matter as much as the + * positive one: delegating the heading must never leave a page with ZERO `h1`. + */ + +import { describe, it, expect } from 'vitest'; +import { render, screen } from '@testing-library/react'; +import { ActionProvider } from '@object-ui/react'; +import { SchemaRenderer } from '@object-ui/react'; +// Module scope, not a hook: registers PageRenderer (`app`/`home`/`record`/…) +// and `page:header`. A cold `await import()` inside a hook is billed to +// `hookTimeout` and races the assertions (AGENTS.md §测试纪律, objectui#3010). +import '../renderers'; + +/** The showcase master-detail page, reduced to the parts that render headings. */ +function masterDetailPage(headerProps: Record | null, extra?: Record) { + return { + type: 'app', + pageType: 'app', + name: 'showcase_project_workspace', + // Spec pages carry `label`; PageRenderer dual-reads it as the page title. + label: 'New Project + Tasks', + template: 'default', + regions: [ + ...(headerProps + ? [{ name: 'header', width: 'full', components: [{ type: 'page:header', properties: headerProps }] }] + : []), + { + name: 'main', + width: 'large', + components: [{ type: 'element:text', properties: { text: 'body' } }], + }, + ], + ...extra, + } as any; +} + +function renderPage(schema: any) { + return render( + + + , + ); +} + +describe('PageRenderer — one page, one h1 (objectui#3434)', () => { + it('renders exactly ONE h1 when a titled page:header carries the same name', () => { + renderPage( + masterDetailPage({ + title: 'New Project + Tasks', + subtitle: 'Master-detail entry — fill the project, add its tasks inline, and save them together.', + }), + ); + + // The exact locator the live e2e uses. Before the fix this threw + // "found multiple elements"; Playwright reported the same as a strict-mode + // violation resolving to 2 elements. + expect(screen.getByRole('heading', { name: 'New Project + Tasks' })).toBeTruthy(); + expect(screen.getAllByRole('heading', { level: 1 })).toHaveLength(1); + // The surviving h1 is the authored header's, and the header keeps its subtitle. + expect(screen.getByRole('heading', { level: 1 }).closest('header')).not.toBeNull(); + expect(screen.getByText(/Master-detail entry/)).toBeTruthy(); + }); + + it('drops the implicit title even when the page:header names the page differently', () => { + // app-crm welcome.page.ts: label 'CRM Welcome' + header 'Welcome to the CRM'. + renderPage({ ...masterDetailPage({ title: 'Welcome to the CRM' }), label: 'CRM Welcome' }); + + expect(screen.getAllByRole('heading', { level: 1 })).toHaveLength(1); + expect(screen.getByRole('heading', { level: 1 }).textContent).toBe('Welcome to the CRM'); + expect(screen.queryByText('CRM Welcome')).toBeNull(); + }); + + it('still renders the implicit h1 when the page authors NO page:header', () => { + renderPage(masterDetailPage(null)); + + const h1s = screen.getAllByRole('heading', { level: 1 }); + expect(h1s).toHaveLength(1); + expect(h1s[0].textContent).toBe('New Project + Tasks'); + }); + + it('still renders the implicit h1 when the page:header has no title of its own', () => { + // Negative control against a zero-h1 page: PageHeaderRenderer's bare branch + // renders `{explicitTitle &&

}`, so an untitled header contributes no + // heading at all and the page's own title must stay. + renderPage(masterDetailPage({ subtitle: 'Just a subtitle' })); + + const h1s = screen.getAllByRole('heading', { level: 1 }); + expect(h1s).toHaveLength(1); + expect(h1s[0].textContent).toBe('New Project + Tasks'); + }); + + it('still renders the implicit h1 when the header title interpolates to nothing', () => { + // `title: '{name}'` with no record in scope → `interpolate()` blanks it → + // no header heading. Suppressing ours here would leave the page headingless. + renderPage(masterDetailPage({ title: '{name}' })); + + const h1s = screen.getAllByRole('heading', { level: 1 }); + expect(h1s).toHaveLength(1); + expect(h1s[0].textContent).toBe('New Project + Tasks'); + }); + + it('honours an inline-translation map as a real header title', () => { + renderPage(masterDetailPage({ title: { en: 'Get in touch', 'zh-CN': '联系我们' } })); + + expect(screen.getAllByRole('heading', { level: 1 })).toHaveLength(1); + expect(screen.getByRole('heading', { level: 1 }).textContent).toBe('Get in touch'); + }); + + it('keeps the page description when the header takes over the heading', () => { + // `description` is the page's own prose, not a duplicate of the header + // `subtitle` — delegating the h1 must not delete unrelated content. + renderPage(masterDetailPage({ title: 'New Project + Tasks' }, { description: 'Page-level prose.' })); + + expect(screen.getAllByRole('heading', { level: 1 })).toHaveLength(1); + expect(screen.getByText('Page-level prose.')).toBeTruthy(); + }); + + it('record pages keep delegating the whole title block (unchanged)', () => { + renderPage({ + ...masterDetailPage({ title: 'New Project + Tasks' }), + type: 'record', + pageType: 'record', + description: 'record prose', + }); + + expect(screen.getAllByRole('heading', { level: 1 })).toHaveLength(1); + expect(screen.queryByText('record prose')).toBeNull(); + }); +}); diff --git a/packages/components/src/renderers/layout/page.tsx b/packages/components/src/renderers/layout/page.tsx index 731b7e9a1e..1e26664a96 100644 --- a/packages/components/src/renderers/layout/page.tsx +++ b/packages/components/src/renderers/layout/page.tsx @@ -83,6 +83,85 @@ function getRemainingRegions(regions: PageNodeRegion[] | undefined, exclude: str return regions.filter((r) => !lowerSet.has(r.name?.toLowerCase() ?? '')); } +// --------------------------------------------------------------------------- +// "One page, one h1" — who owns the page heading (objectui#3434) +// --------------------------------------------------------------------------- + +/** + * `page:header` is registered `skipFallback: true`, so this is the ONLY node + * type that resolves to PageHeaderRenderer — no bare `header` alias to match. + */ +const PAGE_HEADER_TYPE = 'page:header'; + +/** Text a header title contributes once `{token}` interpolation is stripped. */ +function literalTitleText(value: unknown): string { + if (value == null) return ''; + if (typeof value === 'string') { + // `interpolate()` (containers.tsx) replaces `{field}` with the record value + // and blanks it when there is none, so only the literal remainder is text + // we can promise will be on screen. + return value.replace(/\{[a-zA-Z0-9_.]+\}/g, '').replace(/\s+/g, ' ').trim(); + } + if (typeof value === 'object') { + // Inline translation map (`pickLocalized`): any non-empty translation + // means the header will render a heading in some language. + return Object.values(value as Record) + .map((v) => literalTitleText(v)) + .find((s) => s !== '') ?? ''; + } + return String(value); +} + +/** Does this node render a `page:header` heading of its own? */ +function isTitledPageHeader(node: any): boolean { + if (node?.type !== PAGE_HEADER_TYPE) return false; + // Spec bridge may inline `properties.*` onto the node or preserve the bag — + // PageHeaderRenderer reads both, so this must too. + return literalTitleText(node?.title ?? node?.properties?.title) !== ''; +} + +/** Depth-bounded walk over the component shapes a page can nest. */ +function containsTitledPageHeader(nodes: unknown, depth = 0): boolean { + if (!Array.isArray(nodes) || depth > 6) return false; + return nodes.some( + (n: any) => + !!n && + typeof n === 'object' && + (isTitledPageHeader(n) || + containsTitledPageHeader(n.components, depth + 1) || + containsTitledPageHeader(n.children, depth + 1) || + containsTitledPageHeader(n.body, depth + 1)), + ); +} + +/** + * Does the page delegate its `

` to an authored `page:header`? + * + * A document has exactly ONE `h1`. When an author drops a `page:header` into a + * region, THAT component is the page's title renderer — the record chip on + * record pages, a bare `

` everywhere else — so PageRenderer must not emit a + * second one. It used to, for every non-record page type: the showcase + * master-detail page rendered its `label` as an `h1` AND its `page:header` + * title as another `h1` with the same name, which is a broken document outline, + * a title a screen reader announces twice, and a visible duplicate on screen + * (objectui#3434 — a live e2e `getByRole('heading', { name })` resolved to 2 + * elements). Record pages already delegated the whole title block; this is the + * same rule stated for every page type. + * + * Deliberately conservative — only a header whose title renders literal text + * counts. `page:header` drops an empty title (and one that interpolates to + * nothing, e.g. `title: '{name}'` with no record in scope), so suppressing ours + * against a header that renders no heading would leave the page with NO `h1`. + */ +function pageHeaderOwnsTitle(schema: PageNodeSchema): boolean { + const regionNodes = (schema.regions ?? []).flatMap((r: any) => r?.components ?? []); + return ( + containsTitledPageHeader(regionNodes) || + containsTitledPageHeader((schema as any).body) || + containsTitledPageHeader((schema as any).children) + ); +} + // --------------------------------------------------------------------------- // RegionContent — renders all components inside a single region // --------------------------------------------------------------------------- @@ -513,6 +592,18 @@ export const PageRenderer: React.FC<{ ); const maxWidthClass = fullBleed ? 'max-w-none' : getPageMaxWidth(pageType); + // Who renders the page's single `

` (objectui#3434). Record pages always + // delegate to `page:header`; every other page type delegates too as soon as + // the author put a titled `page:header` in a region. + const headerOwnsTitle = React.useMemo( + () => pageType === 'record' || pageHeaderOwnsTitle(schema), + [schema, pageType], + ); + const showPageTitle = !!pageTitle && !headerOwnsTitle; + // The description is the page's own prose, not a duplicate of the header's + // `subtitle`, so delegating the heading does not delete it. + const showPageDescription = !!schema.description && pageType !== 'record'; + const pageContent = (
- {/* Page header — suppressed on record pages (the page:header component - in the header region renders the record-bound title instead). + {/* Implicit page title — the fallback heading for a page that does NOT + author its own `page:header`. Suppressed whenever that component + owns the h1 (always on record pages, and on any page carrying a + titled `page:header`), so the document never has two `h1`. `title` is the objectui spelling; the spec's PageNodeSchema declares `label` (required), so dual-read it — mirrors the fallback DashboardRenderer already uses (framework#1878 §3 recheck). */} - {pageType !== 'record' && (pageTitle || schema.description) && ( + {(showPageTitle || showPageDescription) && (
- {pageTitle && ( + {showPageTitle && (

{pageTitle}

)} - {schema.description && ( + {showPageDescription && (

{schema.description}

)}