diff --git a/.changeset/cel-predicate-field-label-binding.md b/.changeset/cel-predicate-field-label-binding.md new file mode 100644 index 000000000..1aec7ffd3 --- /dev/null +++ b/.changeset/cel-predicate-field-label-binding.md @@ -0,0 +1,23 @@ +--- +'@object-ui/app-shell': patch +--- + +fix(app-shell): every CelPredicateField binds its label, so the RLS row-filter editors have an accessible name + +`CelPredicateField` renders both halves of its own label association — the +`Label htmlFor` and the `Textarea id` — but `id` was an optional prop with no +fallback, so a call site that omitted it left both `undefined`: a label that +read correctly on screen and resolved to nothing, and an editor with no +accessible name. + +The row-level-security `USING (read filter)` and `CHECK (write filter)` clauses +on the permission-set editor were mounted that way, which made the row-filter +authoring surface unreachable by label for screen readers and for +`getByLabelText`. The condition builder's CEL escape hatch was affected too. + +`id` now falls back to a generated `useId()`, so the association is a property +of mounting the component rather than of every caller remembering the prop. An +explicit `id` still wins unchanged — the field inspector's four editors and the +conditional-formatting editor keep the exact ids they already had. The +autocomplete listbox is named off the same resolved id, so an id-less mount now +also carries the `aria-controls` that ties the combobox to its popup. diff --git a/packages/app-shell/src/views/metadata-admin/CelPredicateField.labelBinding.test.tsx b/packages/app-shell/src/views/metadata-admin/CelPredicateField.labelBinding.test.tsx new file mode 100644 index 000000000..351e7e003 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/CelPredicateField.labelBinding.test.tsx @@ -0,0 +1,274 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * objectui#4533 — every `CelPredicateField` mount must bind its own label. + * + * The component renders BOTH halves of the association itself: + * `Label htmlFor={id}` and `Textarea id={id}`. While `id` was an optional prop + * with no fallback, a call site that omitted it produced `undefined` on both — + * a label that reads correctly on screen and resolves to nothing, so the + * textarea has no accessible name at all. + * + * The card surfaced from the authoring side: writing objectui#4302's wire pins, + * the obvious selector was refused verbatim by testing-library — + * + * TestingLibraryElementError: Found a label with the text of: + * USING (read filter), however no form control was found associated to that + * label. + * + * — and that test had to select the RLS editor by placeholder instead + * (`PermissionMatrixEditor.packageDoorFacets.test.tsx`, whose comment points + * here). So the query below is not a proxy for the defect; it IS the refused + * query, and the row-filter authoring surface of a permission set is the + * surface it was refused on. + * + * The fix is at the COMPONENT, not the call sites: `id` falls back to + * `React.useId()`, so the association is a property of mounting the component + * rather than of every caller remembering a prop. That is what these pins are + * shaped to prove — the id-less cases are the red-first, and the explicit-id + * cases pin that a passed `id` still wins unchanged, since four call sites + * depend on their exact ids. + * + * Two call sites the card's table did not have right, both measured on + * `origin/main` before this suite was written: + * + * - `ConditionalFormattingEditor` was listed as omitting `id`; it does not, + * and never has (it has passed `cf-condition-${i}` since #2558, and four + * assertions in `ConditionalFormattingEditor.test.tsx` select on that id). + * The card recorded the mount's line number as the defect. Pinned below as + * an explicit-id site. + * - `inspectors/ConditionBuilder.tsx` DOES omit `id` and was not in the table. + * It is owned by another in-flight change and is not edited here — the + * component-level fallback repairs it without a call-site edit, which is + * precisely the argument for fixing the contract instead of the callers. + */ + +import '@testing-library/jest-dom/vitest'; +import * as React from 'react'; +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, screen, fireEvent, cleanup, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; + +vi.mock('./useMetadata', () => ({ + useMetadataClient: () => ({ + list: vi.fn().mockResolvedValue([]), + listDrafts: vi.fn().mockResolvedValue([]), + }), +})); + +vi.mock('./previews/useObjectFields', () => ({ + useObjectFields: () => ({ fields: [], loading: false, error: null }), +})); + +import { CelPredicateField } from './CelPredicateField'; +import { PermissionAdvancedFacets } from './PermissionAdvancedFacets'; +import { ConditionalFormattingEditor } from './ConditionalFormattingEditor'; +import { ObjectFieldInspector } from './inspectors/ObjectFieldInspector'; +import { __setCelFormulaLoader } from './celAuthoring'; +import { t as translate } from './i18n'; + +afterEach(() => { + cleanup(); + __setCelFormulaLoader(undefined); +}); + +/** Real en-US strings, so the labels below are the ones an author reads. */ +const t = (k: string) => translate(k, 'en-US'); + +/** The exact label text testing-library refused to resolve (i18n `perm.rls.using`). */ +const USING_LABEL = 'USING (read filter)'; +const CHECK_LABEL = 'CHECK (write filter)'; + +/** Controlled harness — the field is controlled, so hold its value in state. */ +function Harness({ initial = '', ...rest }: { initial?: string } & Record) { + const [v, setV] = React.useState(initial); + return ( + + ); +} + +function renderFacets(over: Record = {}) { + const props = { + draft: { + rowLevelSecurity: [ + { name: 'p1', object: 'account', operation: 'all', using: '', check: '', enabled: true }, + ], + }, + setDraft: () => {}, + writable: true, + allSetNames: [] as string[], + loadObjectFields: async () => ['organization_id', 'owner_id'], + t, + ...over, + }; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return render(); +} + +/** The RLS facet is collapsed by default — expand it to mount the CEL editors. */ +async function openRls() { + fireEvent.click(screen.getByText(t('perm.rls.title'))); + await screen.findAllByPlaceholderText('organization_id == current_user.organization_id'); +} + +describe('CelPredicateField · binds its label with no `id` prop (objectui#4533)', () => { + it('resolves the accessibility query that was refused, with no `id` passed', () => { + render(); + // The refused query itself. Before the fallback this threw + // "no form control was found associated to that label". + const box = screen.getByLabelText(USING_LABEL); + expect(box.tagName).toBe('TEXTAREA'); + expect(box).toHaveAccessibleName(USING_LABEL); + }); + + it('gives each id-less mount its OWN id, so labels do not cross-bind', () => { + render( + <> + + + , + ); + const using = screen.getByLabelText(USING_LABEL); + const check = screen.getByLabelText(CHECK_LABEL); + expect(using.id).toBeTruthy(); + expect(check.id).toBeTruthy(); + // A constant fallback id would bind both labels to the first control and + // still pass the single-mount case above. + expect(using.id).not.toBe(check.id); + expect(using).not.toBe(check); + }); + + it('names the autocomplete listbox off the same resolved id', async () => { + __setCelFormulaLoader(() => + Promise.resolve({ + validateExpression: () => ({ ok: true, errors: [], warnings: [] }), + introspectScope: () => ({ + fields: ['organization_id', 'owner_id'], + roots: ['current_user'], + functions: ['has'], + }), + }), + ); + const user = userEvent.setup(); + render(); + const box = screen.getByLabelText(USING_LABEL); + // Typed through userEvent so the caret moves — `refreshAc` reads + // `selectionStart` to find the token under it. + await user.click(box); + await user.type(box, 'org'); + const list = await screen.findByRole('listbox', {}, { timeout: 3000 }); + // `aria-controls` was unreachable while the id was undefined: the menu had + // no id to point at, so the combobox never announced its own popup. + await waitFor(() => expect(box).toHaveAttribute('aria-controls', list.id)); + expect(list.id).toBeTruthy(); + }); +}); + +describe('RLS policy editors bind their labels (objectui#4533 · the card surface)', () => { + it('resolves USING by its label — the query objectui#4302 was refused', async () => { + renderFacets(); + await openRls(); + const box = screen.getByLabelText(USING_LABEL); + expect(box.tagName).toBe('TEXTAREA'); + expect(box).toHaveAttribute('placeholder', 'organization_id == current_user.organization_id'); + }); + + it('resolves CHECK by its label', async () => { + renderFacets(); + await openRls(); + const box = screen.getByLabelText(CHECK_LABEL); + expect(box.tagName).toBe('TEXTAREA'); + expect(box).toHaveAccessibleName(CHECK_LABEL); + }); + + it('binds every clause of every policy to its own editor', async () => { + renderFacets({ + draft: { + rowLevelSecurity: [ + { name: 'p1', object: 'account', operation: 'read', using: '', check: '', enabled: true }, + { name: 'p2', object: 'account', operation: 'write', using: '', check: '', enabled: true }, + ], + }, + }); + await openRls(); + const using = screen.getAllByLabelText(USING_LABEL); + const check = screen.getAllByLabelText(CHECK_LABEL); + expect(using).toHaveLength(2); + expect(check).toHaveLength(2); + const ids = [...using, ...check].map((el) => el.id); + expect(new Set(ids).size).toBe(4); + }); +}); + +describe('an explicit `id` still wins unchanged (objectui#4533 · pins)', () => { + it.each([ + 'field-formula-total', + 'field-rule-visible-total', + 'field-rule-readonly-total', + 'field-rule-required-total', + 'cf-condition-0', + ])('renders %s verbatim on the textarea and on the label', (id) => { + render(); + const box = screen.getByLabelText(USING_LABEL); + expect(box).toHaveAttribute('id', id); + expect(document.getElementById(id)).toBe(box); + }); + + it("keeps ObjectFieldInspector's four call-site ids exactly as they were", async () => { + render( + {}} + onClearSelection={() => {}} + onSelectionChange={() => {}} + readOnly={false} + locale={'en-US'} + />, + ); + for (const id of [ + 'field-formula-total', + 'field-rule-visible-total', + 'field-rule-readonly-total', + 'field-rule-required-total', + ]) { + const box = await waitFor(() => { + const el = document.getElementById(id); + expect(el).toBeTruthy(); + return el as HTMLElement; + }); + expect(box.tagName).toBe('TEXTAREA'); + // The fallback must not have displaced the explicit id. + expect(box.id).toBe(id); + } + }); + + it("keeps ConditionalFormattingEditor's cf-condition-{i} id", async () => { + render( + {}} + objectName="account" + fieldNames={['status']} + t={t} + />, + ); + const box = await waitFor(() => { + const el = document.getElementById('cf-condition-0'); + expect(el).toBeTruthy(); + return el as HTMLElement; + }); + expect(box.tagName).toBe('TEXTAREA'); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/CelPredicateField.tsx b/packages/app-shell/src/views/metadata-admin/CelPredicateField.tsx index 95f64eff7..911008b91 100644 --- a/packages/app-shell/src/views/metadata-admin/CelPredicateField.tsx +++ b/packages/app-shell/src/views/metadata-admin/CelPredicateField.tsx @@ -124,6 +124,22 @@ export function CelPredicateField({ t, id, }: CelPredicateFieldProps) { + /** + * This component renders BOTH halves of the label association itself — the + * `Label htmlFor` and the `Textarea id` below — so binding them is its own + * guarantee, not something every call site has to remember (objectui#4533). + * + * While `id` was optional with no fallback, omitting it left both `undefined`: + * the label read correctly on screen and resolved to nothing, so the editor + * had no accessible name. The RLS `USING` / `CHECK` clauses — the row-filter + * authoring surface of a permission set — were mounted that way, and + * `getByLabelText` refused them outright. + * + * An explicit `id` still wins: four inspector call sites and the + * conditional-formatting editor address their editors by exact id. + */ + const fallbackId = React.useId(); + const fieldId = id ?? fallbackId; const taRef = React.useRef(null); const [issues, setIssues] = React.useState([]); const [linted, setLinted] = React.useState(false); @@ -304,17 +320,19 @@ export function CelPredicateField({ // even alongside warnings — the type is what dataset measure eligibility // keys off, so the author should see it whenever it is known. const showType = role === 'value' && linted && !!value.trim() && errors.length === 0 && inferredType !== null; - const listId = id ? `${id}-ac` : undefined; + // Derived from the resolved id, so the combobox can name its own popup even + // when the call site passed no id (objectui#4533 — same association hole). + const listId = `${fieldId}-ac`; return (
-