fix(a11y): add id={property.name} to all property editors for label association by arome · Pull Request #1508 · iTwin/appui

arome · 2026-02-13T17:19:12Z

Summary

All property editor components (both legacy and new editor system) now set id={propertyRecord.property.name} on their primary interactive element. This enables proper <label htmlFor> association and makes editors uniquely targetable in tests via getByLabelText.

Problem

The legacy editors relied on a shared, hardcoded data-testid (e.g. "components-select-editor"). When multiple editors of the same type appeared on the same page — e.g. two enum selects in a ToolSettings panel — they were indistinguishable. The new editor system had no id at all.

Changes

New editor system (`editorSystem="new"`)

Added id?: string to EditorProps in Types.ts
Threaded id={propertyRecord.property.name} from PropertyRecordEditor → CommittingEditor → EditorRenderer → each concrete editor
Applied to: TextEditor, NumericEditor, EnumEditor, BooleanEditor, ToggleEditor, FallbackEditor, DateEditor, DateTimeEditor (via DateInput)
Applied to all old-editor interop wrappers: TextArea, Color, CustomNumber, EnumButtonGroup, NumericInput, Slider

Legacy editor system

EnumEditor → id on <Select>
BooleanEditor → id on <Checkbox>
ToggleEditor → id on <ToggleSwitch>
ImageCheckBoxEditor → id on <input type="checkbox"> (required adding id prop to ImageCheckBoxProps in core-react)
NumericInputEditor → id on <input> via NumericEditor
TextareaEditor, DateTimeEditor, SliderEditor → id on outer container div (popup-based; interactive content is not always in DOM)
EnumButtonGroupEditor → id on container <div>
IconEditor → id on <IconPickerButton>
TextEditor and CustomNumberEditor already had id set

Testing

4 integration tests added to PropertyRecordEditor.test.tsx covering label association via getByLabelText
1 unit test added to EditorRenderer.test.tsx verifying id forwarding

…lity

GerardasB

Property editor elements are (usually) created by EditorContainer, how do you plan to pass down the itemId prop?
Additionally, itemId prop documentation states, that it adds data-item-id, not data-testid:

appui/ui/core-react/src/core-react/utils/Props.tsx

Line 28 in 17e28f4

    
           /** Optional unique identifier for item. If defined it will be added to DOM Element attribute as data-item-id */

(might be safer to add new attribute, rather than breaking existing?)

arome · 2026-02-20T16:18:26Z

Property editor elements are (usually) created by EditorContainer, how do you plan to pass down the itemId prop? Additionally, itemId prop documentation states, that it adds data-item-id, not data-testid:

appui/ui/core-react/src/core-react/utils/Props.tsx

Line 28 in 17e28f4

/** Optional unique identifier for item. If defined it will be added to DOM Element attribute as data-item-id */

(might be safer to add new attribute, rather than breaking existing?)

indeed i misread, i thought it added data-testid not data-item-id, will add a new attribute instead like you suggested

GerardasB · 2026-02-23T07:53:07Z

How about usage? Property editor elements are (usually) created by EditorContainer, how do you plan to pass down the itemId prop?

arome · 2026-02-23T16:49:22Z

How about usage? Property editor elements are (usually) created by EditorContainer, how do you plan to pass down the itemId prop?

Ya I looked more into it and the change actually needs to be made in itwinjs-core first. I think you saw the discussion there, I'll wait to see from our QA team if they're fine with using something other than the data-testid and will update this PR accordingly

arome · 2026-02-23T20:45:11Z

How about usage? Property editor elements are (usually) created by EditorContainer, how do you plan to pass down the itemId prop?

hey given that theyre not keen on adding data-testid support in itwinjs-core, can we instead dynamically define the data-testid by using the label for example as part of the data-testid?

GerardasB · 2026-02-24T08:24:20Z

Still waiting on the usage part, i.e. what APIs/components are you using and what you are trying to achieve? Is this needed for testing only?
Either way, the stable long term fix would be to correctly label the editors, allowing to use simple queries in tests.

arome · 2026-02-27T17:54:20Z

Still waiting on the usage part, i.e. what APIs/components are you using and what you are trying to achieve? Is this needed for testing only? Either way, the stable long term fix would be to correctly label the editors, allowing to use simple queries in tests.

We use the buildEnumPicklistEditorDescription from itwinjs-core to create two different select fields in the ToolSettings widget while drawing an arc.

During tests, they need to be able to target these different select fields.

(this issue is not limited to the select fields, the input fields also all share the same data-testid and are not easily distinguishable from each other).

the stable long term fix would be to correctly label the editors

can you clarify what you mean by labeling the editor?

GerardasB · 2026-03-02T08:55:45Z

Seems like inputs are labelled correctly when toolSettingsNewEditors is disabled, allowing you to use queries like getByLabelText. We should ensure that is the case for all other controls and when toolSettingsNewEditors is enabled. More importantly than testing - this would fix a11y issues.

Associate each new editor's interactive element (Input, Select, Checkbox, ToggleSwitch) with the property name via the HTML id attribute. This ensures that the existing <label htmlFor={propertyName}> in ComponentGenerator correctly targets the editor, enabling: - getByLabelText() queries in tests - Accessible label/input association when toolSettingsNewEditors is enabled Resolves the a11y gap Gerardas pointed out: inputs were correctly labelled with toolSettingsNewEditors disabled (legacy EditorContainer sets id) but not when it was enabled (new editors omitted the id). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Adds 5 new tests across two files: PropertyRecordEditor.test.tsx — integration: - sets id={property.name} on the input (new editor system) - allows getByLabelText for text properties (new editor system) - sets id={property.name} on the select for enum properties - guard: legacy editor system path still renders EditorContainer EditorRenderer.test.tsx — unit: - forwards id prop to the rendered editor element Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Three stories under 'Components/New Property Editors': - All editor types (new system): shows Text, Numeric, Enum, Boolean, and Toggle editors each paired with a <label htmlFor>. Clicking any label focuses its input — verifying the a11y fix works. - Arc Drawing — ToolSettings use case: two enum selects (Arc Type, Draw Method) plus a numeric field, all uniquely addressable by label. This is the concrete scenario that motivated the fix. - Legacy vs New — label association: side-by-side comparison of the same enum editor rendered with the legacy EditorContainer (label click doesn't focus) vs the new system (label click focuses). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Shows every legacy editor type (string, number, enum, boolean, toggle) in a table annotating what selector you can use in tests: string → id = property.name → getByLabelText / #name ✅ unique number → no id or data-testid on the <input> ⚠ not targetable enum → data-testid='components-select-editor' ⚠ not unique boolean → data-testid='components-checkbox-editor' ⚠ not unique toggle → data-testid='components-toggle-editor' ⚠ not unique Also renders two enum selects side-by-side to demonstrate the collision that motivated the a11y fix (both share the same data-testid). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

number typename with no specific editor name falls back to BasicPropertyEditor → TextEditor, which sets id={property.name}. So id-based targeting works for number just like string. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Mirrors what TextEditor already does. Each editor now sets id={propertyRecord.property.name} on its native element (<Select>, <Checkbox>, <ToggleSwitch>) alongside the existing data-testid, enabling label association and getByLabelText queries. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…ypes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

TextareaEditor, IconEditor, EnumButtonGroupEditor, DateTimeEditor, SliderEditor all now expose id={propertyRecord.property.name} on their root/interactive element. For popup-based editors (Textarea, DateTime, Slider) the id is placed on the outer container div — the always-visible trigger — since the popup content may not be in the DOM when closed. CustomNumberEditor was already fixed in a prior commit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

ImageCheckBoxProps lacked an id field (CommonProps only has itemId). Added id?: string to the interface and thread it to the native <input type="checkbox"> element, consistent with all other legacy editors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…pers TextArea, Color, CustomNumber, EnumButtonGroup, NumericInput, Slider were all missing id in their props destructuring and didn't forward it to their primary element. Enum already forwarded id via {...props}. Popup-based editors (TextArea, Color, Slider) place id on their trigger <Button>/<IconButton> — the always-visible element. Input-based editors (CustomNumber, NumericInput) place id on the <Input>. EnumButtonGroup places id on the <ButtonGroup> container. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

id threads from EditorProps → DateTimeEditor/DateEditor → DateInput → <Button> (the popup trigger, always visible in the DOM). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…tion

- common/api/core-react.api.md: updated to include id?: string added to ImageCheckBoxProps - .vscode/cSpell.json: add 'beforeunload' (pre-existing CHANGELOG word) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

GerardasB · 2026-04-27T08:12:54Z

        }
-        data-testid="components-checkbox-editor"
+        data-testid={this.props.itemId ?? "components-checkbox-editor"}
+        id={this.props.propertyRecord?.property.name}


The id properties must be unique in a document.
Looks like the same property record (with the same property name, which results to duplicate ids) might be rendered multiple times e.g. in a property grid. @saskliutas

Double checked PropertyRecords we created for iModel element property grid and it looks like we should create unique names inside that single property grid. But there might be custom property grids or I guess multiple iModel property grids present that might result in duplicating ids.

So I think we should think about how to ensure unique ids in document scope.

Any idea how we can tackle this?

… for storybook

…or components

Update data-testid attributes to use dynamic props for better flexibi…

106bcc4

…lity

arome requested a review from a team as a code owner February 13, 2026 17:19

arome enabled auto-merge (squash) February 13, 2026 17:21

arome added 3 commits February 13, 2026 12:24

run prettier fix

3403a38

Update data-testid attribute in EditorContainer for consistency

62659ec

Add support for customizable data-testids in PropertyEditor components

57617e1

GerardasB reviewed Feb 17, 2026

View reviewed changes

arome and others added 15 commits March 6, 2026 16:46

Merge remote-tracking branch 'origin/master' into omar/fix-data-testid

4ea504a

docs: update legacy targeting story to reflect id fix on all editor t…

14c2ee2

…ypes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

fix: add id={property.name} to legacy ImageCheckBoxEditor

3acc81e

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

fix: propagate id through DateTimeEditor, DateEditor, and DateInput

2cd20f3

id threads from EditorProps → DateTimeEditor/DateEditor → DateInput → <Button> (the popup trigger, always visible in the DOM). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

fix: add id={property.name} to legacy NumericInputEditor

e8e45c5

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Merge branch 'master' into omar/fix-data-testid

cfa944a

arome changed the title ~~Update data-testid attributes to use dynamic props~~ fix(a11y): add id={property.name} to all property editors for label association Apr 24, 2026

arome and others added 3 commits April 24, 2026 18:43

chore: remove NewPropertyEditors stories file to streamline documenta…

f468041

…tion

chore: fix prettier formatting and update components-react api.md

7e2871b

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

GerardasB requested a review from saskliutas April 27, 2026 07:35

saskliutas reviewed Apr 27, 2026

View reviewed changes

Comment thread ui/components-react/src/components-react/editors/BooleanEditor.tsx Outdated

GerardasB reviewed Apr 27, 2026

View reviewed changes

arome added 3 commits April 27, 2026 11:47

PR Comment: revert the data-testid changes

1771bec

fix: update runtimeArgs from "start" to "dev" in launch configuration…

4978487

… for storybook

fix: standardize data-testid attributes in ColorEditor and WeightEdit…

8af3bf0

…or components

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(a11y): add id={property.name} to all property editors for label association#1508

fix(a11y): add id={property.name} to all property editors for label association#1508
arome wants to merge 25 commits intomasterfrom
omar/fix-data-testid

arome commented Feb 13, 2026 •

edited

Loading

Uh oh!

GerardasB left a comment

Uh oh!

arome commented Feb 20, 2026

Uh oh!

GerardasB commented Feb 23, 2026

Uh oh!

arome commented Feb 23, 2026

Uh oh!

arome commented Feb 23, 2026 •

edited

Loading

Uh oh!

GerardasB commented Feb 24, 2026 •

edited

Loading

Uh oh!

arome commented Feb 27, 2026 •

edited

Loading

Uh oh!

GerardasB commented Mar 2, 2026 •

edited

Loading

Uh oh!

Uh oh!

GerardasB Apr 27, 2026

Uh oh!

saskliutas Apr 27, 2026

Uh oh!

arome Apr 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

arome commented Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Changes

New editor system (editorSystem="new")

Legacy editor system

Testing

Uh oh!

GerardasB left a comment

Choose a reason for hiding this comment

Uh oh!

arome commented Feb 20, 2026

Uh oh!

GerardasB commented Feb 23, 2026

Uh oh!

arome commented Feb 23, 2026

Uh oh!

arome commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GerardasB commented Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arome commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GerardasB commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

GerardasB Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

saskliutas Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

arome Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

arome commented Feb 13, 2026 •

edited

Loading

New editor system (`editorSystem="new"`)

arome commented Feb 23, 2026 •

edited

Loading

GerardasB commented Feb 24, 2026 •

edited

Loading

arome commented Feb 27, 2026 •

edited

Loading

GerardasB commented Mar 2, 2026 •

edited

Loading