Improve searchwithin labeling structure (#2838)

Author: devongovett

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} from '@react-aria/utils';
+import {chain, filterDOMProps, mergeProps, useId} 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,8 +121,7 @@ export function useSelect<T>(props: AriaSelectOptions<T>, state: SelectState<T>,
   let domProps = filterDOMProps(props, {labelable: true});
   let triggerProps = mergeProps(typeSelectProps, menuTriggerProps, fieldProps);
 
-  // used to make predictable id's based on the trigger which is already generated, this aids us in testing
-  let valueId = `${triggerProps.id}-value`;
+  let valueId = useId();
 
   return {
     labelProps: {

packages/@react-spectrum/searchwithin/package.json

@@ -33,9 +33,10 @@
   "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-aria/visually-hidden": "^3.2.3",
+    "@react-spectrum/form": "^3.2.3",
     "@react-spectrum/label": "^3.4.1",
     "@react-spectrum/utils": "^3.6.3",
     "@react-types/searchwithin": "3.0.0-alpha.0",

packages/@react-spectrum/searchwithin/src/SearchWithin.tsx

@@ -24,6 +24,7 @@ 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';
+import {VisuallyHidden} from '@react-aria/visually-hidden';
 
 function SearchWithin(props: SpectrumSearchWithinProps, ref: FocusableRef<HTMLElement>) {
   props = useProviderProps(props);
@@ -87,13 +88,14 @@ function SearchWithin(props: SpectrumSearchWithinProps, ref: FocusableRef<HTMLEl
 
   let searchFieldClassName = classNames(styles, 'spectrum-SearchWithin-searchfield');
   let pickerClassName = classNames(styles, 'spectrum-SearchWithin-picker');
+  let visuallyHiddenId = useId();
 
   let slots = {
     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`,
+      'aria-labelledby': `${labelledBy} ${visuallyHiddenId} ${pickerId}`,
       // When label is provided, input should have id referenced by htmlFor of label, instead of group
       id: label && fieldProps.id
     },
@@ -103,8 +105,7 @@ function SearchWithin(props: SpectrumSearchWithinProps, ref: FocusableRef<HTMLEl
       UNSAFE_className: pickerClassName,
       menuWidth,
       align: 'end',
-      'aria-label': formatMessage('searchWithin'),
-      'aria-labelledby': `${labelledBy} ${pickerId}`
+      'aria-labelledby': `${labelledBy} ${visuallyHiddenId}`
     }
   };
 
@@ -127,6 +128,7 @@ function SearchWithin(props: SpectrumSearchWithinProps, ref: FocusableRef<HTMLEl
         role="group"
         className={classNames(styles, 'spectrum-SearchWithin', styleProps.className)}
         ref={groupRef}>
+        <VisuallyHidden id={visuallyHiddenId}>{formatMessage('searchWithin')}</VisuallyHidden>
         <SlotProvider slots={slots}>
           {children}
         </SlotProvider>

packages/@react-spectrum/searchwithin/test/SearchWithin.test.js

@@ -94,17 +94,18 @@ describe('SearchWithin', function () {
   });
 
   it('searchfield and picker are labelled correctly', function () {
-    let {getByRole, getAllByText} = renderSearchWithin();
+    let {getByRole, getAllByText, getByText} = renderSearchWithin();
 
     let searchfield = getByRole('searchbox');
     let picker = getByRole('button');
     let group = getByRole('group');
+    let hiddenLabel = getByText('Search within');
     triggerPress(picker);
 
     let listbox = getByRole('listbox');
     let label = getAllByText('Test')[0];
-    expect(listbox).toHaveAttribute('aria-labelledby', `${label.id} ${picker.id}`);
-    expect(searchfield).toHaveAttribute('aria-labelledby', `${label.id} ${picker.id} ${picker.childNodes[0].id}`);
+    expect(listbox).toHaveAttribute('aria-labelledby', `${label.id} ${hiddenLabel.id}`);
+    expect(searchfield).toHaveAttribute('aria-labelledby', `${label.id} ${hiddenLabel.id} ${picker.id}`);
     expect(group).toHaveAttribute('aria-labelledby', label.id);
   });
 
@@ -135,7 +136,7 @@ describe('SearchWithin', function () {
   });
 
   it('slot props override props provided to children', function () {
-    let {getByRole, getAllByText} = renderSearchWithin(
+    let {getByRole, getAllByText, getByText} = renderSearchWithin(
       {isDisabled: true, isRequired: false, label: 'Test1'},
       {isDisabled: false, isRequired: true, label: 'Test2', isQuiet: true},
       {isDisabled: false, isRequired: true, label: 'Test3', isQuiet: true}
@@ -144,6 +145,7 @@ describe('SearchWithin', function () {
     let searchfield = getByRole('searchbox');
     let picker = getByRole('button');
     let group = getByRole('group');
+    let hiddenLabel = getByText('Search within');
     triggerPress(picker);
     let label = getAllByText('Test1')[0];
 
@@ -152,7 +154,7 @@ describe('SearchWithin', function () {
 
     expect(searchfield).not.toHaveAttribute('aria-required');
 
-    expect(searchfield).toHaveAttribute('aria-labelledby', `${label.id} ${picker.id} ${picker.childNodes[0].id}`);
+    expect(searchfield).toHaveAttribute('aria-labelledby', `${label.id} ${hiddenLabel.id} ${picker.id}`);
     expect(group).toHaveAttribute('aria-labelledby', label.id);
 
     expect(searchfield.classList.contains('is-quiet')).toBeFalsy();
@@ -162,17 +164,18 @@ describe('SearchWithin', function () {
 
 describe('SearchWithin labeling', function () {
   it('no label - default', function () {
-    let {getByRole} = renderSearchWithin({label: undefined});
+    let {getByRole, getByText} = renderSearchWithin({label: undefined});
 
     let group = getByRole('group');
     let searchfield = getByRole('searchbox');
     let picker = getByRole('button');
+    let hiddenLabel = getByText('Search within');
 
     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}`);
+    expect(searchfield).toHaveAttribute('aria-labelledby', `${hiddenLabel.id} ${picker.id}`);
+    expect(picker).toHaveAttribute('aria-labelledby', `${hiddenLabel.id} ${picker.childNodes[0].id}`);
   });
 
   it('label = foo', function () {
@@ -182,32 +185,34 @@ describe('SearchWithin labeling', function () {
     let searchfield = getByRole('searchbox');
     let picker = getByRole('button');
     let label = getByText('foo');
+    let hiddenLabel = getByText('Search within');
 
     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(searchfield).toHaveAttribute('aria-labelledby', `${label.id} ${hiddenLabel.id} ${picker.id}`);
+    expect(picker).toHaveAttribute('aria-labelledby', `${label.id} ${hiddenLabel.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 {getByRole, getByText} = renderSearchWithin({'aria-label': 'bar', label: undefined});
 
     let group = getByRole('group');
     let searchfield = getByRole('searchbox');
     let picker = getByRole('button');
+    let hiddenLabel = getByText('Search within');
 
     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}`);
+    expect(searchfield).toHaveAttribute('aria-labelledby', `${group.id} ${hiddenLabel.id} ${picker.id}`);
+    expect(picker).toHaveAttribute('aria-labelledby', `${group.id} ${hiddenLabel.id} ${picker.childNodes[0].id}`);
   });
 
   it('aria-labelledby = {id}', function () {
-    let {getByRole} = render(
+    let {getByRole, debug, getByText} = render(
       <Provider theme={theme}>
         <label id="id-foo-label" htmlFor="id-searchfield">
           Foo
@@ -223,16 +228,18 @@ describe('SearchWithin labeling', function () {
         </SearchWithin>
       </Provider>
     );
+    debug();
 
     let group = getByRole('group');
     let searchfield = getByRole('searchbox');
     let picker = getByRole('button');
+    let hiddenLabel = getByText('Search within');
 
     expect(group).not.toHaveAttribute('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('aria-labelledby', `id-foo-label ${hiddenLabel.id} ${picker.id}`);
     expect(searchfield).toHaveAttribute('id', 'id-searchfield');
-    expect(picker).toHaveAttribute('aria-labelledby', `id-foo-label ${picker.id} ${picker.childNodes[0].id}`);
+    expect(picker).toHaveAttribute('aria-labelledby', `id-foo-label ${hiddenLabel.id} ${picker.childNodes[0].id}`);
   });
 });