Skip to content

fix(many): keep form field messages out of the control's accessible name#2625

Open
git-nandor wants to merge 2 commits into
masterfrom
INSTUI-4724_form_field_labels_should_not_include_message
Open

fix(many): keep form field messages out of the control's accessible name#2625
git-nandor wants to merge 2 commits into
masterfrom
INSTUI-4724_form_field_labels_should_not_include_message

Conversation

@git-nandor

@git-nandor git-nandor commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

INSTUI-4724

Summary

Form control messages render inside the wrapping , so screen readers were reading them as part of the field's accessible name (e.g. "Password Password must be at least 6 characters") instead of as a description.

Each single form control now points its accessible name at the label text only (aria-labelledby) and references the messages as its description (aria-describedby), so a screen reader announces the label first, then the messages. The wrapping is unchanged, so click-on-label focus is preserved.

Test plan

Tab through the given exaple and verify in DevTools Accessibility tab that the Name and Description values are correct. With a screen reader enabled, confirm that the label is announced as the field name, followed by a brief pause before the description is announced.

image
const messages = [{ type: 'error', text: 'This field has an error' }]

render(
  <View as="div" display="block" width="24rem">
    <Text as="p">
      Tab through and verify in DevTools Accessibility tab that the Name and Description values are correct. With a screen reader enabled, confirm that the label is announced as the field name, followed by a brief pause before the description is announced.
    </Text>

    <View as="div" margin="medium 0 0">
      <TextInput renderLabel="Text Input" messages={messages} />
    </View>
    <View as="div" margin="medium 0 0">
      <NumberInput renderLabel="Number Input" messages={messages} />
    </View>
    <View as="div" margin="medium 0 0">
      <TextArea label="Text Area" messages={messages} />
    </View>
    <View as="div" margin="medium 0 0">
      <Checkbox label="Checkbox (simple)" value="c1" messages={messages} />
    </View>
    <View as="div" margin="medium 0 0">
      <Checkbox
        label="Checkbox (toggle)"
        variant="toggle"
        value="c2"
        messages={messages}
      />
    </View>
    <View as="div" margin="medium 0 0">
      <RangeInput
        label="Range Input"
        name="range"
        min={0}
        max={100}
        defaultValue={50}
        messages={messages}
      />
    </View>
  </View>
)

Co-Authored-By: 🤖 Claude

@git-nandor git-nandor self-assigned this Jul 6, 2026
@git-nandor
git-nandor requested a review from ToMESSKa July 6, 2026 16:48
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1

QR code for preview link

🚀 View preview at
https://instructure.design/pr-preview/pr-2625/

Built to branch gh-pages at 2026-07-17 07:11 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

Visual regression report

No changes.

Status Count
Unchanged 32
Changed 0
New 0
Removed 0

📊 View full report

Baselines come from the visual-baselines branch. They refresh on every merge to master.

github-actions Bot pushed a commit that referenced this pull request Jul 6, 2026
@git-nandor
git-nandor marked this pull request as ready for review July 7, 2026 10:18
Comment on lines +241 to +242
// Any consumer-provided `aria-labelledby` (e.g. Select's pill handling)
// takes precedence.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the Select's pill handling doesn't use aria-labelledby anymore see #2616 (comment) so this is not a relevant example

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, modified

Form control messages (error/hint/success) render inside the wrapping <label>, so they were
read as part of the field's accessible name. Point each control's name at the label text via
aria-labelledby and reference the messages via aria-describedby, so screen readers announce
them as the field's description. Label click-to-focus is preserved.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@git-nandor
git-nandor force-pushed the INSTUI-4724_form_field_labels_should_not_include_message branch from 9e15d7b to 8d8c8f4 Compare July 8, 2026 15:11
@git-nandor
git-nandor requested review from ToMESSKa and removed request for ToMESSKa July 8, 2026 15:11

@ToMESSKa ToMESSKa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A vitest test is failing, can you please check?

@git-nandor
git-nandor requested a review from ToMESSKa July 13, 2026 09:27
@ToMESSKa
ToMESSKa requested a review from matyasf July 14, 2026 09:56

@matyasf matyasf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • dateTimeInput is wrong, see the example in the /forms page: aria-describedby and aria-labelledby reference nothing.
  • While it works, I generally dislike this component structure. All this mess is needed, because we wrap everything in a , the nice way would be not to do so. I think its OK for now, but we need to completely revamp this in the future. (also support SSR better, now the layout falls apart when loading)
  • see my other comments

Comment thread packages/ui-checkbox/src/Checkbox/v2/index.tsx
Comment thread packages/ui-checkbox/src/Checkbox/v2/index.tsx Outdated
Comment thread packages/ui-text-area/src/TextArea/v2/index.tsx
Comment thread packages/ui-text-input/src/TextInput/v2/index.tsx Outdated
@git-nandor
git-nandor marked this pull request as draft July 16, 2026 07:07
Address PR review feedback on the form-field messages work:

- The aria-describedby wiring referenced a messages element even when the message had no
  text. FormField (via FormFieldLayout) only renders messages that have text, so this left
  a dangling reference — e.g. DateTimeInput passes an empty-text error message to its
  sub-inputs just to force the invalid styling. Gate the wiring on messages that actually
  render (`messages.some(m => !!m.text)`). Checkbox is unchanged here: it renders messages
  via FormFieldMessages on `length > 0`, so its reference always resolves.
- Drop the now-unnecessary type casts around the aria-* reads in Checkbox, TextArea,
  NumberInput and RangeInput.
- TextInput: expand the comment to explain why messages must be kept out of the name.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@git-nandor
git-nandor marked this pull request as ready for review July 17, 2026 07:06
@git-nandor
git-nandor requested a review from matyasf July 17, 2026 07:08
github-actions Bot pushed a commit that referenced this pull request Jul 17, 2026

@matyasf matyasf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, just a small nitpick

Comment on lines +163 to +167
// FormField only renders messages that have text, so only treat the field
// as having messages then. Otherwise the input's `aria-describedby` would
// reference a messages element that was never rendered — e.g. DateTimeInput
// passes an empty-text error message to its sub-inputs just to force the
// invalid styling.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make a bit more brief comments. Claude can be very very verbose and this just makes the code hard to read. For example this comment is totally unnecessary IMO, the code is pretty self-explanatory

Comment on lines +241 to +249
// FormField renders this control and its `messages` inside a single wrapping
// <label>, so by default the messages' text becomes part of the control's
// accessible *name* (e.g. "Password Password must be at least 6 characters"),
// which is confusing and repeats on every announcement. Messages should be
// the field's *description* instead. So when there are messages, reference
// them via `aria-describedby` and pin the name to the label text only via
// `aria-labelledby`. Keeping the messages inside the <label> preserves the
// native click-on-label focus behavior. A consumer-provided `aria-labelledby`
// takes precedence.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, way too verbose. I'd just write:

"Use the contents of the messages field as ARIA label; user supplied value takes precedence"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants