fix(many): fix form label not read by NVDA in hover mode and other layout 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

Author: matyasf

cypress/component/FormField.cy.tsx

@@ -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'
@@ -36,7 +37,7 @@ describe('<FormField/>', () => {
       </FormFieldLayout>
     )
 
-    cy.get('span[class$="-formFieldLabel"]')
+    cy.get('span[class$="-formFieldLayout__label"]')
       .contains('Username')
       .should('have.css', 'text-align', 'end')
   })
@@ -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')
   })

packages/shared-types/src/ComponentThemeVariables.ts

@@ -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 = {
@@ -1748,7 +1751,6 @@ export interface ThemeVariables {
   FileDrop: FileDropTheme
   Flex: FlexTheme
   FormFieldGroup: FormFieldGroupTheme
-  FormFieldLabel: FormFieldLabelTheme
   FormFieldMessage: FormFieldMessageTheme
   FormFieldMessages: FormFieldMessagesTheme
   Grid: GridTheme

packages/ui-checkbox/src/CheckboxGroup/__new-tests__/CheckboxGroup.test.tsx

@@ -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()
@@ -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)
@@ -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')
+    })
   })
 })

packages/ui-checkbox/src/CheckboxGroup/props.ts

@@ -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'
@@ -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'
@@ -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,

packages/ui-date-input/src/DateInput/__new-tests__/DateInput.test.tsx

@@ -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')
 

packages/ui-date-input/src/DateInput2/__new-tests__/DateInput2.test.tsx

@@ -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 () => {

packages/ui-form-field/src/FormField/README.md

@@ -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>
+  <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>
 ```

packages/ui-form-field/src/FormField/index.tsx

@@ -68,7 +68,6 @@ class FormField extends Component<FormFieldProps> {
         label={this.props.label}
         vAlign={this.props.vAlign}
         as="label"
-        htmlFor={this.props.id}
         elementRef={this.handleRef}
         margin={this.props.margin}
       />

packages/ui-form-field/src/FormFieldGroup/__new-tests__/FormFieldGroup.test.tsx

@@ -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:')
@@ -94,9 +94,7 @@ describe('<FormFieldGroup />', () => {
       </FormFieldGroup>
     )
 
-    const formFieldGroup = container.querySelector(
-      "fieldset[class$='-formFieldLayout']"
-    )
+    const formFieldGroup = container.querySelector('label')
 
     expect(formFieldGroup).toBeInTheDocument()
   })
@@ -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(
@@ -154,7 +152,7 @@ describe('<FormFieldGroup />', () => {
     )
 
     const legend = container.querySelector(
-      "legend[class$='-screenReaderContent']"
+      "span[class$='-formFieldLayout__label']"
     )
 
     expect(legend).toBeInTheDocument()

packages/ui-form-field/src/FormFieldGroup/index.tsx

@@ -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'
@@ -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
@@ -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
     )
   }
@@ -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)}
@@ -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}
       >