Fix SearchWithin aria-label and aria-labelledby (#2288)

reidbarber · snowystinger · LFDanLu · web-flow · commit 1597756b0666 · 2022-01-11T09:26:38.000-06:00
* fix aria-label and aria-labelledby in searchwithin

* update tests

* Get translations started, block help text on sub components

* fix SearchWithin deps

* fix labels for group and elements

* fix test cases for aria-label application

* fix style regression when focuses

* add story for when no label or aria-label provided

* improve input query in tests

* modify useSelect id to use id from triggerProps

* override user-provided aria-label for searchfield

* add story for external label

Co-authored-by: Robert Snow &lt;rsnow@adobe.com&gt;
Co-authored-by: Daniel Lu &lt;dl1644@gmail.com&gt;
Co-authored-by: Michael Jordan &lt;mijordan@adobe.com&gt;
diff --git a/packages/@react-aria/select/src/useSelect.ts b/packages/@react-aria/select/src/useSelect.ts
@@ -13,7 +13,7 @@
 import {AriaButtonProps} from '@react-types/button';
 import {AriaListBoxOptions} from '@react-aria/listbox';
 import {AriaSelectProps} from '@react-types/select';
-import {chain, filterDOMProps, mergeProps, useId} from '@react-aria/utils';
+import {chain, filterDOMProps, mergeProps} from '@react-aria/utils';
 import {FocusEvent, HTMLAttributes, RefObject, useMemo} from 'react';
 import {KeyboardDelegate} from '@react-types/shared';
 import {ListKeyboardDelegate, useTypeSelect} from '@react-aria/selection';
@@ -121,7 +121,8 @@ export function useSelect<T>(props: AriaSelectOptions<T>, state: SelectState<T>,
   let domProps = filterDOMProps(props, {labelable: true});
   let triggerProps = mergeProps(typeSelectProps, menuTriggerProps, fieldProps);
 
-  let valueId = useId();
+  // used to make predictable id's based on the trigger which is already generated, this aids us in testing
+  let valueId = `${triggerProps.id}-value`;
 
   return {
     labelProps: {
diff --git a/packages/@react-spectrum/searchwithin/intl/en-US.json b/packages/@react-spectrum/searchwithin/intl/en-US.json
@@ -0,0 +1,4 @@
+{
+    "search": "Search",
+    "searchWithin": "Search within"
+}
diff --git a/packages/@react-spectrum/searchwithin/package.json b/packages/@react-spectrum/searchwithin/package.json
@@ -32,6 +32,8 @@
   },
   "dependencies": {
     "@babel/runtime": "^7.6.2",
+    "@react-aria/i18n": "^3.3.2",
+    "@react-spectrum/form": "^3.2.3",
     "@react-aria/label": "^3.2.1",
     "@react-aria/utils": "^3.10.0",
     "@react-spectrum/label": "^3.4.1",
diff --git a/packages/@react-spectrum/searchwithin/src/SearchWithin.tsx b/packages/@react-spectrum/searchwithin/src/SearchWithin.tsx
@@ -13,26 +13,42 @@
 import {classNames, SlotProvider, useFocusableRef, useResizeObserver, useStyleProps} from '@react-spectrum/utils';
 import {Field} from '@react-spectrum/label';
 import {FocusableRef} from '@react-types/shared';
+// @ts-ignore
+import intlMessages from '../intl/*.json';
 import React, {useCallback, useRef, useState} from 'react';
 import {SpectrumSearchWithinProps} from '@react-types/searchwithin';
 import styles from '@adobe/spectrum-css-temp/components/searchwithin/vars.css';
+import {useFormProps} from '@react-spectrum/form';
+import {useId} from '@react-aria/utils';
 import {useLabel} from '@react-aria/label';
 import {useLayoutEffect} from '@react-aria/utils';
+import {useMessageFormatter} from '@react-aria/i18n';
 import {useProvider, useProviderProps} from '@react-spectrum/provider';
 
 function SearchWithin(props: SpectrumSearchWithinProps, ref: FocusableRef<HTMLElement>) {
   props = useProviderProps(props);
+  props = useFormProps(props);
+  let formatMessage = useMessageFormatter(intlMessages);
   let {styleProps} = useStyleProps(props);
-  let {labelProps, fieldProps} = useLabel(props);
   let {
     children,
     isDisabled,
     isRequired,
-    label,
-    'aria-label': ariaLabel,
-    'aria-labelledby': ariaLabelledby
+    label
   } = props;
 
+  let defaultAriaLabel = formatMessage('search');
+  if (!label && !props['aria-label'] && !props['aria-labelledby']) {
+    props['aria-label'] = defaultAriaLabel;
+  }
+  // Get label and group props (aka fieldProps)
+  let {labelProps, fieldProps} = useLabel(props);
+
+  // Grab aria-labelledby for the search input. Will need the entire concatenated aria-labelledby if it exists since pointing at the group id doesn’t
+  // suffice if there is a external label
+  let labelledBy = fieldProps['aria-labelledby'] || (fieldProps['aria-label'] !== defaultAriaLabel ? fieldProps.id : '');
+  let pickerId = useId();
+
   let domRef = useFocusableRef(ref);
   let groupRef = useRef<HTMLDivElement>();
 
@@ -61,18 +77,40 @@ function SearchWithin(props: SpectrumSearchWithinProps, ref: FocusableRef<HTMLEl
     isRequired,
     label: null,
     isQuiet: false,
-    'aria-labelledby': labelProps.id || ariaLabel,
-    validationState: null
+    validationState: null,
+    description: null,
+    errorMessage: null,
+    descriptionProps: null,
+    errorMessageProps: null,
+    'aria-label': null
   };
+
   let searchFieldClassName = classNames(styles, 'spectrum-SearchWithin-searchfield');
   let pickerClassName = classNames(styles, 'spectrum-SearchWithin-picker');
+
   let slots = {
-    searchfield: {UNSAFE_className: searchFieldClassName, ...fieldProps, ...defaultSlotValues},
-    picker: {UNSAFE_className: pickerClassName, menuWidth, align: 'end', ...defaultSlotValues}
+    searchfield: {
+      ...defaultSlotValues,
+      UNSAFE_className: searchFieldClassName,
+      // Apply aria-labelledby of group or the group id to searchfield. No need to pass the group id (we want a new one) and aria-label (aria-labelledby will suffice)
+      'aria-labelledby': `${labelledBy} ${pickerId} ${pickerId}-value`,
+      // When label is provided, input should have id referenced by htmlFor of label, instead of group
+      id: label && fieldProps.id
+    },
+    picker: {
+      ...defaultSlotValues,
+      id: pickerId,
+      UNSAFE_className: pickerClassName,
+      menuWidth,
+      align: 'end',
+      'aria-label': formatMessage('searchWithin'),
+      'aria-labelledby': `${labelledBy} ${pickerId}`
+    }
   };
 
-  if (!label && !ariaLabel && !ariaLabelledby) {
-    console.warn('If you do not provide a `label` prop, you must specify an aria-label or aria-labelledby attribute for accessibility');
+  if (label) {
+    // When label is provided, input should have id referenced by htmlFor of label, instead of group
+    delete fieldProps.id;
   }
 
   return (
@@ -85,8 +123,8 @@ function SearchWithin(props: SpectrumSearchWithinProps, ref: FocusableRef<HTMLEl
         'spectrum-SearchWithin-container'
       )}>
       <div
+        {...fieldProps}
         role="group"
-        aria-labelledby={labelProps.id || ariaLabel}
         className={classNames(styles, 'spectrum-SearchWithin', styleProps.className)}
         ref={groupRef}>
         <SlotProvider slots={slots}>
diff --git a/packages/@react-spectrum/searchwithin/stories/SearchWithin.stories.tsx b/packages/@react-spectrum/searchwithin/stories/SearchWithin.stories.tsx
@@ -26,7 +26,7 @@ export default {
 
 function render(props: Omit<SpectrumSearchWithinProps, 'children'> = {}, searchFieldProps: SearchFieldProps = {}, pickerProps: Omit<SpectrumPickerProps<object>, 'children'> = {}) {
   return (
-    <SearchWithin label="Search" {...props}>
+    <SearchWithin label="This is label" {...props}>
       <SearchField placeholder="Search" {...searchFieldProps} onChange={action('change')} onSubmit={action('submit')} />
       <Picker defaultSelectedKey="all" {...pickerProps} onSelectionChange={action('selectionChange')}>
         <Item key="all">All</Item>
@@ -41,7 +41,7 @@ function render(props: Omit<SpectrumSearchWithinProps, 'children'> = {}, searchF
 
 function renderReverse(props: Omit<SpectrumSearchWithinProps, 'children'> = {}, searchFieldProps: SearchFieldProps = {}, pickerProps: Omit<SpectrumPickerProps<object>, 'children'> = {}) {
   return (
-    <SearchWithin label="Search" {...props}>
+    <SearchWithin label="Test label" {...props}>
       <Picker defaultSelectedKey="all" {...pickerProps} onSelectionChange={action('selectionChange')}>
         <Item key="all">All</Item>
         <Item key="campaigns">Campaigns</Item>
@@ -60,7 +60,7 @@ function ResizeSearchWithinApp(props) {
   return (
     <Flex direction="column" gap="size-200" alignItems="start">
       <div style={{width: state ? '300px' : '400px'}}>
-        <SearchWithin label="Search" {...props} width="100%">
+        <SearchWithin label="Test label" {...props} width="100%">
           <SearchField placeholder="Search" onChange={action('change')} onSubmit={action('submit')} />
           <Picker defaultSelectedKey="all" onSelectionChange={action('selectionChange')}>
             <Item key="all">All</Item>
@@ -119,7 +119,16 @@ CustomWidth30.storyName = 'Custom width: 30';
 export const LabelPositionSide = () => render({labelPosition: 'side'});
 LabelPositionSide.storyName = 'labelPosition: side';
 
-export const NoLabel = () => render({label: undefined, 'aria-label': 'Aria Label'});
+export const NoVisibleLabel = () => render({label: undefined, 'aria-label': 'Test aria label'});
+
+export const NoLabels = () => render({label: undefined});
+
+export const ExternalLabel = () => (
+  <div style={{display: 'flex', flexDirection: 'column'}}>
+    <span id="foo">External label</span>
+    {render({label: undefined, 'aria-labelledby': 'foo'})}
+  </div>
+);
 
 export const AutoFocusSearchField = () => render({}, {autoFocus: true});
 AutoFocusSearchField.storyName = 'autoFocus: true on SearchField';
diff --git a/packages/@react-spectrum/searchwithin/test/SearchWithin.test.js b/packages/@react-spectrum/searchwithin/test/SearchWithin.test.js
@@ -53,9 +53,9 @@ describe('SearchWithin', function () {
   });
 
   it('renders correctly', function () {
-    let {getAllByText, getByRole, getByLabelText} = renderSearchWithin();
+    let {getAllByText, getByRole} = renderSearchWithin();
 
-    let searchfield = getByLabelText('Test', {selector: 'input'});
+    let searchfield = getByRole('searchbox');
     expect(searchfield).toBeVisible();
     expect(searchfield).toHaveAttribute('type', 'search');
 
@@ -69,8 +69,8 @@ describe('SearchWithin', function () {
 
   it('can type in search and get onChange', function () {
     let onChange = jest.fn();
-    let {getByLabelText} = renderSearchWithin({}, {onChange});
-    let searchfield = getByLabelText('Test', {selector: 'input'});
+    let {getByRole} = renderSearchWithin({}, {onChange});
+    let searchfield = getByRole('searchbox');
     expect(searchfield).toHaveAttribute('value', '');
 
     act(() => {searchfield.focus();});
@@ -94,34 +94,34 @@ describe('SearchWithin', function () {
   });
 
   it('searchfield and picker are labelled correctly', function () {
-    let {getByRole, getAllByText, getByLabelText} = renderSearchWithin();
+    let {getByRole, getAllByText} = renderSearchWithin();
 
-    let searchfield = getByLabelText('Test', {selector: 'input'});
+    let searchfield = getByRole('searchbox');
     let picker = getByRole('button');
     let group = getByRole('group');
     triggerPress(picker);
 
     let listbox = getByRole('listbox');
     let label = getAllByText('Test')[0];
-    expect(listbox).toHaveAttribute('aria-labelledby', label.id);
-    expect(searchfield).toHaveAttribute('aria-labelledby', label.id);
+    expect(listbox).toHaveAttribute('aria-labelledby', `${label.id} ${picker.id}`);
+    expect(searchfield).toHaveAttribute('aria-labelledby', `${label.id} ${picker.id} ${picker.childNodes[0].id}`);
     expect(group).toHaveAttribute('aria-labelledby', label.id);
   });
 
   it('isDisabled=true disables both the searchfield and picker', function () {
-    let {getByRole, getByLabelText} = renderSearchWithin({isDisabled: true});
+    let {getByRole} = renderSearchWithin({isDisabled: true});
 
-    let searchfield = getByLabelText('Test', {selector: 'input'});
+    let searchfield = getByRole('searchbox');
     let picker = getByRole('button');
 
     expect(searchfield).toHaveAttribute('disabled');
     expect(picker).toHaveAttribute('disabled');
   });
 
   it('autoFocus=true on searchfield will automatically focus the input', function () {
-    let {getByLabelText} = renderSearchWithin({}, {autoFocus: true});
+    let {getByRole} = renderSearchWithin({}, {autoFocus: true});
 
-    let searchfield = getByLabelText('Test', {selector: 'input'});
+    let searchfield = getByRole('searchbox');
 
     expect(searchfield).toHaveFocus();
   });
@@ -135,13 +135,13 @@ describe('SearchWithin', function () {
   });
 
   it('slot props override props provided to children', function () {
-    let {getByRole, getAllByText, getByLabelText} = renderSearchWithin(
+    let {getByRole, getAllByText} = renderSearchWithin(
       {isDisabled: true, isRequired: false, label: 'Test1'},
       {isDisabled: false, isRequired: true, label: 'Test2', isQuiet: true},
       {isDisabled: false, isRequired: true, label: 'Test3', isQuiet: true}
     );
 
-    let searchfield = getByLabelText('Test1', {selector: 'input'});
+    let searchfield = getByRole('searchbox');
     let picker = getByRole('button');
     let group = getByRole('group');
     triggerPress(picker);
@@ -152,20 +152,87 @@ describe('SearchWithin', function () {
 
     expect(searchfield).not.toHaveAttribute('aria-required');
 
-    expect(searchfield).toHaveAttribute('aria-labelledby', label.id);
+    expect(searchfield).toHaveAttribute('aria-labelledby', `${label.id} ${picker.id} ${picker.childNodes[0].id}`);
     expect(group).toHaveAttribute('aria-labelledby', label.id);
 
     expect(searchfield.classList.contains('is-quiet')).toBeFalsy();
     expect(picker.classList.contains('spectrum-Dropdown--quiet')).toBeFalsy();
   });
+});
+
+describe('SearchWithin labeling', function () {
+  it('no label - default', function () {
+    let {getByRole} = renderSearchWithin({label: undefined});
+
+    let group = getByRole('group');
+    let searchfield = getByRole('searchbox');
+    let picker = getByRole('button');
+
+    expect(group).toHaveAttribute('aria-label', 'Search');
+
+    expect(group).not.toHaveAttribute('aria-labelledby');
+    expect(searchfield).toHaveAttribute('aria-labelledby', `${picker.id} ${picker.childNodes[0].id}`);
+    expect(picker).toHaveAttribute('aria-labelledby', `${picker.id} ${picker.childNodes[0].id}`);
+  });
 
-  it('searchfield and group are labelled correctly if aria-label used', function () {
-    let {getByRole, getByLabelText} = renderSearchWithin({label: undefined, 'aria-label': 'Aria Label'});
+  it('label = foo', function () {
+    let {getByRole, getByText} = renderSearchWithin({label: 'foo'});
 
-    let searchfield = getByLabelText('Aria Label', {selector: 'input'});
     let group = getByRole('group');
+    let searchfield = getByRole('searchbox');
+    let picker = getByRole('button');
+    let label = getByText('foo');
+
+    expect(group).not.toHaveAttribute('aria-label');
+
+    expect(group).toHaveAttribute('aria-labelledby', label.id);
+    expect(searchfield).toHaveAttribute('aria-labelledby', `${label.id} ${picker.id} ${picker.childNodes[0].id}`);
+    expect(picker).toHaveAttribute('aria-labelledby', `${label.id} ${picker.id} ${picker.childNodes[0].id}`);
+
+    expect(label).toHaveAttribute('for', searchfield.id);
+  });
+
+  it('aria-label = bar', function () {
+    let {getByRole} = renderSearchWithin({'aria-label': 'bar', label: undefined});
+
+    let group = getByRole('group');
+    let searchfield = getByRole('searchbox');
+    let picker = getByRole('button');
+
+    expect(group).toHaveAttribute('aria-label', 'bar');
+
+    expect(group).not.toHaveAttribute('aria-labelledby');
+    expect(searchfield).toHaveAttribute('aria-labelledby', `${group.id} ${picker.id} ${picker.childNodes[0].id}`);
+    expect(picker).toHaveAttribute('aria-labelledby', `${group.id} ${picker.id} ${picker.childNodes[0].id}`);
+  });
+
+  it('aria-labelledby = {id}', function () {
+    let {getByRole} = render(
+      <Provider theme={theme}>
+        <label id="id-foo-label" htmlFor="id-searchfield">
+          Foo
+        </label>
+        <SearchWithin aria-labelledby="id-foo-label">
+          <SearchField id="id-searchfield" placeholder="Search"  />
+          <Picker defaultSelectedKey="all">
+            <Item key="all">All</Item>
+            <Item key="campaigns">Campaigns</Item>
+            <Item key="audiences">Audiences</Item>
+            <Item key="tags">Tags</Item>
+          </Picker>
+        </SearchWithin>
+      </Provider>
+    );
+
+    let group = getByRole('group');
+    let searchfield = getByRole('searchbox');
+    let picker = getByRole('button');
+
+    expect(group).not.toHaveAttribute('aria-label');
 
-    expect(searchfield).toHaveAttribute('aria-label', 'Aria Label');
-    expect(group).toHaveAttribute('aria-labelledby', 'Aria Label');
+    expect(group).toHaveAttribute('aria-labelledby', 'id-foo-label');
+    expect(searchfield).toHaveAttribute('aria-labelledby', `id-foo-label ${picker.id} ${picker.childNodes[0].id}`);
+    expect(searchfield).toHaveAttribute('id', 'id-searchfield');
+    expect(picker).toHaveAttribute('aria-labelledby', `id-foo-label ${picker.id} ${picker.childNodes[0].id}`);
   });
 });

-Original file line number
+Diff line change
@@ @@ -0,0 +1,4 @@ @@
 +{
 +    "search": "Search",
 +    "searchWithin": "Search within"
 +}