Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
matyasf merged 1 commit into from
Feb 26, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions cypress/component/FormField.cy.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
* SOFTWARE.
*/
import React from 'react'
// eslint-disable-next-line @instructure/no-relative-imports
import { FormFieldLayout } from '../../packages/ui'

import '../support/component'
Expand All @@ -36,7 +37,7 @@ describe('<FormField/>', () => {
</FormFieldLayout>
)

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

@matyasf matyasf Feb 20, 2025

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

cy.get('span[class$="-formFieldLayout__label"]')
.contains('Username')
.should('have.css', 'text-align', 'end')
})
Expand All @@ -50,7 +51,7 @@ describe('<FormField/>', () => {
</FormFieldLayout>
)

cy.get('span[class$="-formFieldLabel"]')
cy.get('span[class$="-formFieldLayout__label"]')
.contains('Username')
.should('have.css', 'text-align', 'start')
})
Expand Down
6 changes: 4 additions & 2 deletions packages/shared-types/src/ComponentThemeVariables.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -595,12 +595,15 @@ export type FormFieldGroupTheme = {
errorFieldsPadding: Spacing['xSmall']
}

export type FormFieldLabelTheme = {
export type FormFieldLayoutTheme = {
color: Colors['contrasts']['grey125125']
fontFamily: Typography['fontFamily']
fontWeight: Typography['fontWeightBold']
fontSize: Typography['fontSizeMedium']
lineHeight: Typography['lineHeightFit']
inlinePadding: Spacing['xxSmall']
stackedOrInlineBreakpoint: Breakpoints['medium']
spacing: any // TODO remove any
}

export type FormFieldMessageTheme = {
Expand Down Expand Up @@ -1748,7 +1751,6 @@ export interface ThemeVariables {
FileDrop: FileDropTheme
Flex: FlexTheme
FormFieldGroup: FormFieldGroupTheme
FormFieldLabel: FormFieldLabelTheme
FormFieldMessage: FormFieldMessageTheme
FormFieldMessages: FormFieldMessagesTheme
Grid: GridTheme
Expand Down
22 changes: 19 additions & 3 deletions packages/ui-checkbox/src/CheckboxGroup/__new-tests__/CheckboxGroup.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,8 @@ describe('<CheckboxGroup />', () => {
const { container } = renderCheckboxGroup({
messages: [{ text: TEST_ERROR_MESSAGE, type: 'error' }]
})
const fieldset = screen.getByRole('group')
const ariaDesc = fieldset.getAttribute('aria-describedby')
const group = screen.getByRole('group')
const ariaDesc = group.getAttribute('aria-describedby')
const messageById = container.querySelector(`[id="${ariaDesc}"]`)

expect(messageById).toBeInTheDocument()
Expand All @@ -98,7 +98,9 @@ describe('<CheckboxGroup />', () => {

it('displays description message inside the legend', () => {
const { container } = renderCheckboxGroup({ description: TEST_DESCRIPTION })
const legend = container.querySelector('legend')
const legend = container.querySelector(
'span[class$="-formFieldLayout__label"]'
)

expect(legend).toBeInTheDocument()
expect(legend).toHaveTextContent(TEST_DESCRIPTION)
Expand Down Expand Up @@ -217,5 +219,19 @@ describe('<CheckboxGroup />', () => {

expect(axeCheck).toBe(true)
})

it('adds the correct ARIA attributes', () => {
const { container } = renderCheckboxGroup({
disabled: true,
messages: [{ type: 'newError', text: 'abc' }],
// @ts-ignore This is a valid attribute
'data-id': 'group'
})
const group = container.querySelector(`[data-id="group"]`)
expect(group).toBeInTheDocument()
expect(group).toHaveAttribute('aria-disabled', 'true')
// ARIA role 'group' cannot have the 'aria-invalid' attribute
expect(group).not.toHaveAttribute('aria-invalid')
})
})
})
14 changes: 11 additions & 3 deletions packages/ui-checkbox/src/CheckboxGroup/props.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
* SOFTWARE.
*/

import React from 'react'
import React, { type InputHTMLAttributes } from 'react'
import PropTypes from 'prop-types'

import { FormPropTypes } from '@instructure/ui-form-field'
Expand All @@ -32,7 +32,10 @@ import {
} from '@instructure/ui-prop-types'

import type { FormMessage } from '@instructure/ui-form-field'
import type { PropValidators } from '@instructure/shared-types'
import type {
OtherHTMLAttributes,
PropValidators
} from '@instructure/shared-types'
import type { WithDeterministicIdProps } from '@instructure/ui-react-utils'

import { Checkbox } from '../Checkbox'
Expand All @@ -58,7 +61,12 @@ type PropKeys = keyof CheckboxGroupOwnProps

type AllowedPropKeys = Readonly<Array<PropKeys>>

type CheckboxGroupProps = CheckboxGroupOwnProps & WithDeterministicIdProps
type CheckboxGroupProps = CheckboxGroupOwnProps &
OtherHTMLAttributes<
CheckboxGroupOwnProps,
InputHTMLAttributes<CheckboxGroupOwnProps & Element>
> &
WithDeterministicIdProps

const propTypes: PropValidators<PropKeys> = {
name: PropTypes.string.isRequired,
Expand Down
4 changes: 3 additions & 1 deletion packages/ui-date-input/src/DateInput/__new-tests__/DateInput.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,9 @@ describe('<DateInput />', () => {
{generateDays()}
</DateInput>
)
const dateInput = container.querySelector("[class$='-formFieldLabel']")
const dateInput = container.querySelector(
'span[class$="-formFieldLayout__label"]'
)

expect(dateInput).toHaveTextContent('Choose date')

Expand Down
2 changes: 0 additions & 2 deletions packages/ui-date-input/src/DateInput2/__new-tests__/DateInput2.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,10 @@ describe('<DateInput2 />', () => {
it('should render an input label', async () => {
const { container } = render(<DateInputExample />)

const dateInput = container.querySelector('input')
const label = container.querySelector('label')

expect(label).toBeInTheDocument()
expect(label).toHaveTextContent(LABEL_TEXT)
expect(dateInput?.id).toBe(label?.htmlFor)
})

it('should render an input placeholder', async () => {
Expand Down
34 changes: 31 additions & 3 deletions packages/ui-form-field/src/FormField/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,35 @@ components. In most cases it shouldn't be used directly.
---
type: example
---
<FormField id="_foo123" label="Opacity" width="200px">
<input style={{display: 'block', width: '100%'}} id="_foo123"/>
</FormField>
<div>
Copy link
Collaborator Author

@matyasf matyasf Feb 20, 2025

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

<FormField id="_foo121" label="Stacked layout" width="400px" layout="stacked"
messages={[{type:'success', text: 'This is a success message'}, {type:'newError', text: 'An error message. It will wrap if the text is longer than the width of the container.'}]}>
<TextInput id="_foo121"/>
</FormField>
test
<hr/>
<FormField id="_foo122" label="Stacked layout (inline=true)" width="400px" layout="stacked" inline
messages={[{type:'success', text: 'This is a success message'}, {type:'newError', text: 'An error message. It will wrap if the text is longer than the width of the container.'}]}>
<TextInput id="_foo122"/>
</FormField>
test
<hr/>
<FormField id="_foo123" label="Inline layout" width="400px" layout="inline"
messages={[{type:'success', text: 'success!'}, {type:'newError', text: 'An error message. It will wrap if the text is longer than the width of the container.'}]}>
<TextInput id="_foo123"/>
</FormField>
test
<hr/>
<FormField id="_foo124" label="Inline layout (inline=true)" width="400px" layout="inline" inline
messages={[{type:'success', text: 'success!'}, {type:'newError', text: 'An error message. It will wrap if the text is longer than the width of the container.'}]}>
<TextInput id="_foo124"/>
</FormField>
test
<hr/>
<FormField id="_foo121" label={<ScreenReaderContent>hidden text</ScreenReaderContent>} width="400px" layout="stacked">
<TextInput id="_foo121" />
</FormField>
test
<hr/>
</div>
```
1 change: 0 additions & 1 deletion packages/ui-form-field/src/FormField/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@ class FormField extends Component<FormFieldProps> {
label={this.props.label}
vAlign={this.props.vAlign}
as="label"
htmlFor={this.props.id}
Copy link
Collaborator Author

@matyasf matyasf Feb 20, 2025

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

@matyasf matyasf Mar 6, 2025

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

elementRef={this.handleRef}
margin={this.props.margin}
/>
Expand Down
10 changes: 4 additions & 6 deletions packages/ui-form-field/src/FormFieldGroup/__new-tests__/FormFieldGroup.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ describe('<FormFieldGroup />', () => {
)

const formFieldGroup = container.querySelector(
"fieldset[class$='-formFieldLayout']"
"span[class$='-formFieldLayout__label']"
)
const firstNameInput = screen.getByLabelText('First:')
const middleNameInput = screen.getByLabelText('Middle:')
Expand Down Expand Up @@ -94,9 +94,7 @@ describe('<FormFieldGroup />', () => {
</FormFieldGroup>
)

const formFieldGroup = container.querySelector(
"fieldset[class$='-formFieldLayout']"
)
const formFieldGroup = container.querySelector('label')

expect(formFieldGroup).toBeInTheDocument()
})
Expand Down Expand Up @@ -136,7 +134,7 @@ describe('<FormFieldGroup />', () => {
expect(message).toHaveAttribute('id', messagesId)
})

it('displays description message inside the legend', () => {
it('displays description message inside the label', () => {
const description = 'Please enter your full name'

const { container } = render(
Expand All @@ -154,7 +152,7 @@ describe('<FormFieldGroup />', () => {
)

const legend = container.querySelector(
"legend[class$='-screenReaderContent']"
"span[class$='-formFieldLayout__label']"
)

expect(legend).toBeInTheDocument()
Expand Down
47 changes: 41 additions & 6 deletions packages/ui-form-field/src/FormFieldGroup/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
*/

/** @jsx jsx */
import { Component, Children, ReactElement } from 'react'
import { Component, Children, ReactElement, AriaAttributes } from 'react'

import { Grid } from '@instructure/ui-grid'
import { pickProps, omitProps } from '@instructure/ui-react-utils'
Expand Down Expand Up @@ -53,7 +53,8 @@ class FormFieldGroup extends Component<FormFieldGroupProps> {
disabled: false,
rowSpacing: 'medium',
colSpacing: 'small',
vAlign: 'middle'
vAlign: 'middle',
isGroup: true
}

ref: Element | null = null
Expand All @@ -77,14 +78,20 @@ class FormFieldGroup extends Component<FormFieldGroupProps> {
}

get makeStylesVariables(): FormFieldGroupStyleProps {
return { invalid: this.invalid }
// new form errors dont need borders
const oldInvalid =
!!this.props.messages &&
this.props.messages.findIndex((message) => {
return message.type === 'error'
}) >= 0
return { invalid: oldInvalid }
}

get invalid() {
return (
!!this.props.messages &&
this.props.messages.findIndex((message) => {
return message.type === 'error'
return message.type === 'error' || message.type === 'newError'
}) >= 0
)
}
Expand Down Expand Up @@ -134,7 +141,35 @@ class FormFieldGroup extends Component<FormFieldGroupProps> {

render() {
const { styles, makeStyles, isGroup, ...props } = this.props

// This is quite ugly, but according to ARIA spec the `aria-invalid` prop
// can only be used with certain roles see
// https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-invalid#associated_roles
// `aria-invalid` is put on in FormFieldLayout because the error message
// DOM part gets there its ID.
let ariaInvalid: AriaAttributes['aria-invalid'] = undefined
if (
this.props.role &&
this.invalid &&
[
'application',
'checkbox',
'combobox',
'gridcell',
'listbox',
'radiogroup',
'slider',
'spinbutton',
'textbox',
'tree',
'columnheader',
'rowheader',
'searchbox',
'switch',
'treegrid'
].includes(this.props.role)
) {
ariaInvalid = 'true'
}
return (
<FormFieldLayout
{...omitProps(props, FormFieldGroup.allowedProps)}
Expand All @@ -143,7 +178,7 @@ class FormFieldGroup extends Component<FormFieldGroupProps> {
layout={props.layout === 'inline' ? 'inline' : 'stacked'}
label={props.description}
aria-disabled={props.disabled ? 'true' : undefined}
aria-invalid={this.invalid ? 'true' : undefined}
aria-invalid={ariaInvalid}
elementRef={this.handleRef}
isGroup={isGroup}
>
Expand Down
Loading
Loading