Skip to content

Fix description text not read on hover by NVDA and other FormFieldLayout bugs#1863

Merged
matyasf merged 1 commit intomasterfrom
fix_form_aria
Feb 26, 2025
Merged

Fix description text not read on hover by NVDA and other FormFieldLayout bugs#1863
matyasf merged 1 commit intomasterfrom
fix_form_aria

Conversation

@matyasf
Copy link
Collaborator

@matyasf matyasf commented Feb 6, 2025

The issue was that NVDA hover mode was not reading form labels because they were set to aria-hidden.
This was needed because screen readers only read the <legend> tag if its the first child of the <fieldset>, so the visible label was added with aria-hidden and another inivisible one was added as a first child.
This PR refactors mainly the FormFieldLayout component, so it uses now CSS grid, so its possible to have the <legend> as the first child of the <fieldset>.

To test:

  • Check the examples of CheckboxGroup, FormField, RadioInputGroup with NVDA, JAWS, VoiceOver.
  • NVDA should read the label on hover. Form groups should be announced
  • Check components that use FormFieldLayout, they should have no visual changes except the ones I listed below. A good sample page is the "form errors" page https://instructure.design/pr-preview/pr-1863/#form-errors
  • compare form field errors against the Figma designs

Visual bugs fixed:

  • formFieldMessage was right aligned in inline layouts (see https://instructure.design/pr-preview/pr-1863/#FormField )
  • when layout=inline and inline=true the messages were spanning the whole bottom area. I assumed that this is a bug, now they look like other messages and are displayed only under the control
  • When using the new error type and its a group the errors will be above the controls as per design Figma

Closes: INSTUI-4444

@matyasf matyasf self-assigned this Feb 6, 2025
@github-actions
Copy link

github-actions bot commented Feb 6, 2025

PR Preview Action v1.6.0
Preview removed because the pull request was closed.
2025-02-26 14:33 UTC

@matyasf matyasf requested review from ToMESSKa and balzss February 7, 2025 15:56
@matyasf matyasf changed the title (WIP) Fix description text not read on hover by NVDA Fix description text not read on hover by NVDA Feb 7, 2025
@matyasf matyasf changed the title Fix description text not read on hover by NVDA (WIP) Fix description text not read on hover by NVDA Feb 10, 2025
</FormFieldLayout>
)

cy.get('span[class$="-formFieldLabel"]')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For tests likely this will be most of the change

<FormField id="_foo123" label="Opacity" width="200px">
<input style={{display: 'block', width: '100%'}} id="_foo123"/>
</FormField>
<div>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've added here a more complex example that we can use to test this PR

label={this.props.label}
vAlign={this.props.vAlign}
as="label"
htmlFor={this.props.id}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is not needed because the labels child is the control, https://developer.mozilla.org/en-US/docs/Web/HTML/Element/label#defining_an_implicit_label

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

EDIT 2 weeks later: we should not do this, it will bring the last leaf on the focus causing issues with e.g. multiple select

@matyasf matyasf changed the title (WIP) Fix description text not read on hover by NVDA Fix description text not read on hover by NVDA and other Focusable bugs Feb 21, 2025
@matyasf matyasf requested a review from ToMESSKa February 21, 2025 09:58
@matyasf matyasf changed the title Fix description text not read on hover by NVDA and other Focusable bugs Fix description text not read on hover by NVDA and other FormFieldLayout bugs Feb 21, 2025
Copy link
Contributor

@ToMESSKa ToMESSKa left a comment

Choose a reason for hiding this comment

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

DateTimeInput error messages are not read by VoiceOver for me

@matyasf
Copy link
Collaborator Author

matyasf commented Feb 21, 2025

DateTimeInput error messages are not read by VoiceOver for me

This is a tricky one. it worked in the old code because it had a hidden <legend> where we copied all the messages. But we agreed with a11y engineers that this is not a good practice.

I tried to implement a nice, no-hacks solution here. That is to use aria-invalid with aria-errormessage. For some strange reason these are not supported in the group ARIA role (that we use for CheckboxGroup and DateTimeInput), so I could not add it. They are only supported to radioInput groups , I have added them there, but VoiceOver does not seem to support this tag (at least OSX 14.7); NVDA reads them nicely.

I'll show this to our a11y engineers and let them decide

…yout issues

This commit fixes the following:
- Form labels not read in NVDA hover
- FormFieldMessage was right aligned in inline layouts
- When layout=inline and inline=true the messages are now not spanning the whole bottom area
- When using the new error type and its a group the errors will be above the controls as per
design requirements

Fixes INSTUI-4444
@matyasf matyasf merged commit ef77281 into master Feb 26, 2025
11 checks passed
@matyasf matyasf deleted the fix_form_aria branch February 26, 2025 14:33
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