From 5db5ad26286e174ae514089cc28f8a55a886b2d9 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 00:17:21 +0000 Subject: [PATCH] =?UTF-8?q?fix(app-shell):=20InspectorComboField=20?= =?UTF-8?q?=E7=9A=84=20label=20=E5=85=B3=E8=81=94=20trigger,=E6=97=A0?= =?UTF-8?q?=E5=90=8D=20combo=20=E4=B8=8D=E5=86=8D=E5=8F=AF=E7=BC=96?= =?UTF-8?q?=E8=AF=91=20(#3997)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 第四个字段原子,与 PR #3996 修掉的 `_shared.tsx` 三个原子形状完全相同 —— `Label` 是控件的兄弟节点,没有 `htmlFor`,trigger 没有 `id` 也没有 `aria-label`。 它在自己的模块里,所以那三个修好之后它仍旧带着缺陷。标签与 `button[role=combobox]` 之间只有视觉邻接:焦点落上去读到匿名 combobox,可见标签是 一段无归属文本,`getByLabelText` 到不了它,点标签什么都不会发生。非测试调用点 18 处 (对象字段 / 数据集 / 仪表盘部件 / 应用导航 / 视图变体检查器),打开任一面板即渲染。 带 label 分支照 #3994 的定型机制闭合:`React.useId()` 在原子内部生成 id,`Label` 补 `htmlFor`,id 落到 `PopoverTrigger asChild` 渲染出的那个 `Button`。⛔ 不落 `Popover` —— Radix `Popover.Root` 是纯 context provider,不渲染 DOM,给它的 id 会被静默丢弃、 `for` 随之悬空(#3976 / #3994 已为此付过两次学费)。 ## 无 label 分支:命名改成类型级要求,而不是消费端兜底 `label` 原本可选,无 label 分支是同缺陷更重一档 —— combobox 完全无名,而 18 处调用点 里有 5 处正是这么写的。没有采用宽松兜底(用 placeholder 合成名字会把「Select…」念成 字段名);改为「三条通道恰选其一」,零条和两条都不可编译: - `label` —— 原子渲染可见标签并自持关联。已传 label 的 13 处不变。 - `ariaLabel` —— 重复行里本就没有可见标签、加一个会破坏栅格:应用导航 URL 过滤条件的 `field = value` 行、数据集的 join 列表、依赖查找的「添加字段」选择器。 - `id` —— 外部 `Label htmlFor` 已经持有命名。`DashboardWidgetInspector` 的 `Field` 包装器渲染 `Label htmlFor={id}` 并把同一个 id 交给被包控件,其余字段都履行了这个约定 (`Input id`、`SelectTrigger id`),只有 dataset combo 落不下去,因为原子不收 id —— 那个 `for` 指向一个没有任何元素持有的 id,是悬空 IDREF,比无标签更糟,因为工具会报告 一个解析不到的关联。 两条通道同时给出是 #3961/#3978 要避免的重复播报,故也一并禁掉。两种错误都没有组件能 自行发现并报告的运行期症状 —— 无名 combobox 渲染、布局、提交值全都正常,只对看不见它 的用户是错的 —— 所以检查只能发生在编写期,否则就不存在。 ## 钉子与反向验证 - `_shared.labels.test.tsx`:第四原子直接加入既有的 `describe.each`(它已把「for 有 宿主 / 命中可聚焦控件 / 可访问名 / 单一命名通道 / 多实例不撞车」写成与组件无关的形 状),另加 trigger 落点、自定义值、`ariaLabel` 分支、外部 `id` 分支四组。 - `DashboardWidgetInspector.test.tsx`:真实调用点上钉 Dataset 标签解析到 combo trigger。 刻意只锁这一对,不做整面板「无悬空 for」扫描 —— `widget-color` 的 `ColorVariantPicker` 同样不收 id,那是另一个组件、本单范围外,已单独立单 #4010。 - `InspectorComboField.naming.types.test.tsx`(新增,列入 `tsconfig.typetests.json`): 类型级断言。第一稿把 `@ts-expect-error` 写在 `_shared.labels.test.tsx` 里,变异实测 显示那是**假绿** —— 把命名改回可选,`tsc --noEmit` 完全通过,因为包的构建 tsconfig 排除 `**/*.test.tsx`、vitest 又擦除类型,没有任何编译器读它。这正是 #3009 的失效模式, 故移入被 typetests 项目编译的独立文件;同一变异现在报 6 条错(4 条 assignability 断言 + 2 条 unused `@ts-expect-error`)。 反向验证三向(先预判后跑,变异未提交):撤 `htmlFor` → combo 8 行翻红,其中「mints exactly one owner for the id」按预判保持绿(id 还在,只是没人指向它);id 改落 `Popover` Root → 11 行翻红(多出 owner 计数、外部 id 分支、真实调用点钉);命名改回可选 → 运行期 全绿、信号只在 typetests。 Fixes #3997 Co-authored-by: Claude Claude-Session: https://claude.ai/code/session_01GTRjn8xBqp75dk7kFupVRt --- .../inspector-combo-field-label-3997.md | 19 +++ .../src/views/metadata-admin/i18n.ts | 2 + .../inspectors/AppNavInspector.tsx | 4 + .../DashboardWidgetInspector.test.tsx | 28 ++++ .../inspectors/DashboardWidgetInspector.tsx | 15 ++ .../inspectors/DatasetDefaultInspector.tsx | 4 + .../InspectorComboField.naming.types.test.tsx | 117 +++++++++++++ .../inspectors/InspectorComboField.tsx | 77 ++++++++- .../inspectors/ObjectFieldInspector.tsx | 4 + .../inspectors/_shared.labels.test.tsx | 156 +++++++++++++++++- packages/app-shell/tsconfig.typetests.json | 11 ++ 11 files changed, 432 insertions(+), 5 deletions(-) create mode 100644 .changeset/inspector-combo-field-label-3997.md create mode 100644 packages/app-shell/src/views/metadata-admin/inspectors/InspectorComboField.naming.types.test.tsx diff --git a/.changeset/inspector-combo-field-label-3997.md b/.changeset/inspector-combo-field-label-3997.md new file mode 100644 index 0000000000..a50a70cdc0 --- /dev/null +++ b/.changeset/inspector-combo-field-label-3997.md @@ -0,0 +1,19 @@ +--- +'@object-ui/app-shell': patch +--- + +Name the `InspectorComboField` trigger: the visible label now owns it, and an anonymous combo no longer compiles (objectui#3997). + +This is the fourth inspector field atom with the shape PR #3996 fixed for the three in `_shared.tsx` — a `Label` rendered as a plain sibling of the control, with no `htmlFor`, no `id` and no `aria-label`. It lives in its own module, so it stayed broken after the other three were closed. The label and the `button[role=combobox]` were adjacent only visually: assistive tech announced an anonymous combobox with the field name floating above it as unowned text, `getByLabelText` could not reach it, and clicking the visible label did nothing. It renders at eighteen call sites across the object-field, dataset, dashboard-widget, app-nav and view-variant inspectors (lookup display/description fields, `lookupFilters` rows, summary aggregates, dataset dimensions and measures, nav targets), so it is on screen the moment any of those panels opens. + +The labelled branch closes the pair the same way the other atoms do: `React.useId()` mints the id inside the atom, `Label` gets the `htmlFor`, and the id lands on the trigger `Button` that `PopoverTrigger asChild` renders. Never on `Popover` — Radix's `Popover.Root` is a context provider that renders no DOM element, so an id handed to it is dropped silently and the `for` dangles, which is the objectui#3976 / #3994 mistake this repo has now paid for twice. + +`label` was optional, and the un-labelled branch was the same defect one notch worse: a combobox with no name at all. Five of the eighteen call sites had authored exactly that. Rather than adding a lenient fallback (synthesising a name from the placeholder would have produced "Select…" as the announced name), naming became a type-level requirement of exactly one of three channels: + +- `label` — the atom renders the visible label and owns the association. Unchanged for the thirteen call sites that already passed one. +- `ariaLabel` — for repeated rows where no visible label exists and one would break the grid: an app-nav URL filter's `field = value` pair, a dataset's list of joined relationships, the dependent-lookup "add a field" picker. +- `id` — for when an external `Label htmlFor` already owns the naming. `DashboardWidgetInspector` wraps its controls in a `Field` that renders `Label htmlFor={id}` and hands the same id to the control; every other field honoured it (`Input id`, `SelectTrigger id`) but the dataset combo could not, because the atom accepted no id. That `for` pointed at an id nothing carried — a dangling IDREF, worse than an unnamed control, because tooling reports an association that resolves to nothing. + +Zero channels and two channels are now both unauthorable: zero is anonymous, and two is the double-announcement failure objectui#3961/#3978 exists to avoid. Neither has a runtime symptom the component could detect and report — an unnamed combobox renders, lays out and commits values perfectly, and is wrong only for the users who cannot see it — so the check is compile-time or nothing. It is pinned in `InspectorComboField.naming.types.test.tsx`, listed in `tsconfig.typetests.json` so a compiler actually reads it. + +One new pair of strings (`engine.inspector.widget.filterBindingField`, en-US + zh-CN) names the per-filter binding combo in the dashboard widget inspector, which sits under a heading that captions its whole row rather than the combo alone. diff --git a/packages/app-shell/src/views/metadata-admin/i18n.ts b/packages/app-shell/src/views/metadata-admin/i18n.ts index b5d6384266..00ed90875d 100644 --- a/packages/app-shell/src/views/metadata-admin/i18n.ts +++ b/packages/app-shell/src/views/metadata-admin/i18n.ts @@ -328,6 +328,7 @@ const ENGINE_STRINGS_EN: Record = { 'Map each dashboard-level filter to one of this widget’s own fields, or untick Apply to opt the widget out. Empty = the filter’s own field.', 'engine.inspector.widget.filterBindingApply': 'Apply', 'engine.inspector.widget.filterBindingDefault': 'Default ({field})', + 'engine.inspector.widget.filterBindingField': 'Bound field for {filter}', 'engine.inspector.widget.filterBindingReset': 'Reset', // Flow node inspector 'engine.inspector.flowNode.kind': 'Node', @@ -2074,6 +2075,7 @@ const ENGINE_STRINGS_ZH: Record = { '把每个仪表盘级过滤器映射到本组件自己的字段；取消勾选「应用」可让本组件不受该过滤器影响。留空表示使用过滤器自身的字段。', 'engine.inspector.widget.filterBindingApply': '应用', 'engine.inspector.widget.filterBindingDefault': '默认（{field}）', + 'engine.inspector.widget.filterBindingField': '{filter} 绑定的字段', 'engine.inspector.widget.filterBindingReset': '恢复默认', // Flow node inspector 'engine.inspector.flowNode.kind': '节点', diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/AppNavInspector.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/AppNavInspector.tsx index 11f69af61d..d0eaf80af6 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/AppNavInspector.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/AppNavInspector.tsx @@ -235,6 +235,10 @@ function FiltersEditor({
update(i, v, value)} options={fieldOptions} diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/DashboardWidgetInspector.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/DashboardWidgetInspector.test.tsx index bbbe264377..5e9e52bb3f 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/DashboardWidgetInspector.test.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/DashboardWidgetInspector.test.tsx @@ -95,6 +95,34 @@ describe('DashboardWidgetInspector — dataset binding', () => { expect(screen.getByText('维度')).toBeInTheDocument(); }); + it('the Dataset label resolves to the combo trigger, not to nothing (#3997)', () => { + // This panel labels its controls through a `Field` wrapper that renders + // `
`; the id + // has to reach the trigger or that `for` dangles (objectui#3997). + // Every other `Field` in this file already hands its id to the + // control it wraps (`Input id`, `SelectTrigger id`) — this one could + // not, because the combo took no id at all. + id="widget-dataset" value={datasetName} onCommit={(v) => patchWidget({ dataset: v || undefined } as Partial)} options={datasetComboOptions} @@ -346,6 +352,15 @@ export function DashboardWidgetInspector({
setBinding(v ? v : undefined)} options={fieldComboOptions} diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.tsx index dea37a25bc..be9ac90767 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.tsx @@ -412,6 +412,10 @@ export function DatasetDefaultInspector({ draft, onPatch, readOnly, name }: Meta include.map((rel, i) => (
onPatch({ include: include.map((r, idx) => (idx === i ? v : r)) })} options={relationshipComboOptions} diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/InspectorComboField.naming.types.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/InspectorComboField.naming.types.test.tsx new file mode 100644 index 0000000000..8ca29a83c6 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/inspectors/InspectorComboField.naming.types.test.tsx @@ -0,0 +1,117 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `InspectorComboField`'s naming contract is enforced by the TYPE, so it is + * pinned at compile time (objectui#3997). + * + * The atom accepts exactly one of `label` / `ariaLabel` / `id` — never zero, + * never two. Zero is an anonymous `button[role=combobox]`: assistive tech reads + * "combobox" with no idea which field it edits. Two is the double-announcement + * failure objectui#3961/#3978 exists to avoid. Both were authorable before this + * change, and five of the eleven live call sites had in fact authored zero. + * + * The reason this is a TYPE and not a runtime guard is that neither mistake has + * a runtime symptom the component could detect and report: an unnamed combobox + * renders, lays out and commits values perfectly. It is only wrong for the users + * who cannot see it. So the check has to happen at authoring time or not at all. + * + * ## Why this file exists separately, and why it is listed + * + * The assertions below are its entire point, so it is listed in + * `packages/app-shell/tsconfig.typetests.json`. That listing is the difference + * between a pin and a decoration: the package's build tsconfig excludes + * `**\/*.test.tsx`, and vitest erases types before running, so a + * `@ts-expect-error` written in an ordinary `*.test.tsx` file in this directory + * is read by NO compiler — it neither fails when the error disappears nor when + * the error was never there. This was drafted that way first, and the mutation + * run said so: making naming optional again produced a completely green + * `tsc --noEmit`. That is objectui#3009's failure verbatim (assertions that + * never ran, under a header calling them the real enforcement), and + * `tsconfig.typetests.json`'s own header warns about it — so the type-level + * cases moved here, out of `_shared.labels.test.tsx`, where they are compiled. + * + * The runtime `expect` at the bottom is deliberately thin: the DOM consequences + * (which element carries the id, what the accessible name resolves to, that the + * id never lands on the Radix `Popover` root) are pinned in + * `_shared.labels.test.tsx`, which renders. This file only has to be a file + * vitest can run without complaining that it holds no tests. + */ + +import * as React from 'react'; +import { describe, it, expect } from 'vitest'; +import { InspectorComboField, type InspectorComboFieldProps } from './InspectorComboField'; + +type Assert = T; +type Extends = [A] extends [B] ? true : false; +type IsAny = 0 extends 1 & T ? true : false; +type Equal = (() => T extends A ? 1 : 2) extends () => T extends B ? 1 : 2 + ? true + : false; + +const OPTIONS = [{ value: 'profile', label: 'Profile' }]; +const noop = (_v: string) => {}; + +/** Everything the combo needs EXCEPT a name. */ +type Base = { + value: string; + onCommit: (v: string) => void; + options: Array<{ value: string; label: string }>; +}; + +describe('InspectorComboField — naming is required, and singular (#3997)', () => { + it('is pinned at compile time', () => { + // Guard against the probe lying: were the props `any`, every assignability + // assertion below would pass while proving nothing. + type _PropsNotAny = Assert, false>>; + + // ── the three legal spellings ──────────────────────────────────────────── + // The atom renders the visible label and owns the `htmlFor` ⇄ `id` pair. + type _LabelIsANaming = Assert>; + // No visible label exists (repeated rows); the trigger names itself. + type _AriaLabelIsANaming = Assert>; + // An external `