Fix: Enable tabbing to tag remove buttons (#8002)

* Fix tag keyboard navigation to allow tabbing to remove buttons

* fix lint

* Add more integration level tests and stories, fix controls

* fix case where tabbing moves to prev/next clear button instead of moving out of collection

* get rid of extraneous tree walker

---------

Co-authored-by: Kiril Matsiuk <[email protected]>
Co-authored-by: Yihui Liao <[email protected]>
Co-authored-by: Robert Snow <[email protected]>
Co-authored-by: Robert Snow <[email protected]>
Co-authored-by: Daniel Lu <[email protected]>

Author: 5 people

packages/@react-aria/gridlist/src/useGridListItem.ts

@@ -131,7 +131,7 @@ export function useGridListItem<T>(props: AriaGridListItemOptions, state: ListSt
     linkBehavior
   });
 
-  let onKeyDown = (e: ReactKeyboardEvent) => {
+  let onKeyDownCapture = (e: ReactKeyboardEvent) => {
     if (!e.currentTarget.contains(e.target as Element) || !ref.current || !document.activeElement) {
       return;
     }
@@ -225,18 +225,6 @@ export function useGridListItem<T>(props: AriaGridListItemOptions, state: ListSt
           );
         }
         break;
-      case 'Tab': {
-        if (keyboardNavigationBehavior === 'tab') {
-          // If there is another focusable element within this item, stop propagation so the tab key
-          // is handled by the browser and not by useSelectableCollection (which would take us out of the list).
-          let walker = getFocusableTreeWalker(ref.current, {tabbable: true});
-          walker.currentNode = document.activeElement;
-          let next = e.shiftKey ? walker.previousNode() : walker.nextNode();
-          if (next) {
-            e.stopPropagation();
-          }
-        }
-      }
     }
   };
 
@@ -256,6 +244,28 @@ export function useGridListItem<T>(props: AriaGridListItemOptions, state: ListSt
     }
   };
 
+  let onKeyDown = (e) => {
+    if (!e.currentTarget.contains(e.target as Element) || !ref.current || !document.activeElement) {
+      return;
+    }
+
+    switch (e.key) {
+      case 'Tab': {
+        if (keyboardNavigationBehavior === 'tab') {
+          // If there is another focusable element within this item, stop propagation so the tab key
+          // is handled by the browser and not by useSelectableCollection (which would take us out of the list).
+          let walker = getFocusableTreeWalker(ref.current, {tabbable: true});
+          walker.currentNode = document.activeElement;
+          let next = e.shiftKey ? walker.previousNode() : walker.nextNode();
+
+          if (next) {
+            e.stopPropagation();
+          }
+        }
+      }
+    }
+  };
+
   let syntheticLinkProps = useSyntheticLinkProps(node.props);
   let linkProps = itemStates.hasAction ? syntheticLinkProps : {};
   // TODO: re-add when we get translations and fix this for iOS VO
@@ -270,7 +280,8 @@ export function useGridListItem<T>(props: AriaGridListItemOptions, state: ListSt
 
   let rowProps: DOMAttributes = mergeProps(itemProps, linkProps, {
     role: 'row',
-    onKeyDownCapture: onKeyDown,
+    onKeyDownCapture,
+    onKeyDown,
     onFocus,
     // 'aria-label': [(node.textValue || undefined), rowAnnouncement].filter(Boolean).join(', '),
     'aria-label': node.textValue || undefined,

packages/@react-aria/tag/src/useTag.ts

@@ -56,8 +56,6 @@ export function useTag<T>(props: AriaTagProps<T>, state: ListState<T>, ref: RefO
     node: item
   }, state, ref);
 
-  // We want the group to handle keyboard navigation between tags.
-  delete rowProps.onKeyDownCapture;
   // eslint-disable-next-line @typescript-eslint/no-unused-vars
   let {descriptionProps: _, ...stateWithoutDescription} = states;
 
@@ -103,8 +101,7 @@ export function useTag<T>(props: AriaTagProps<T>, state: ListState<T>, ref: RefO
       'aria-labelledby': `${buttonId} ${rowProps.id}`,
       isDisabled,
       id: buttonId,
-      onPress: () => onRemove ? onRemove(new Set([item.key])) : null,
-      excludeFromTabOrder: true
+      onPress: () => onRemove ? onRemove(new Set([item.key])) : null
     },
     rowProps: mergeProps(focusableProps, rowProps, domProps, linkProps, {
       tabIndex,

packages/@react-aria/tag/src/useTagGroup.ts

@@ -80,7 +80,8 @@ export function useTagGroup<T>(props: AriaTagGroupOptions<T>, state: ListState<T
     ...fieldProps,
     keyboardDelegate,
     shouldFocusWrap: true,
-    linkBehavior: 'override'
+    linkBehavior: 'override',
+    keyboardNavigationBehavior: 'tab'
   }, state, ref);
 
   let [isFocusWithin, setFocusWithin] = useState(false);

packages/@react-aria/tag/test/useTagGroup.test.js

@@ -112,4 +112,103 @@ describe('useTagGroup', function () {
     expect(onRemove).toHaveBeenCalledTimes(3);
     expect(onRemove).toHaveBeenLastCalledWith(new Set(['pool']));
   });
+
+  it('should support tabbing to tags and arrow navigation between tags', async () => {
+    let {getAllByRole} = render(
+      <TagGroup
+        label="Amenities">
+        <Item key="laundry">Laundry</Item>
+        <Item key="fitness">Fitness center</Item>
+        <Item key="parking">Parking</Item>
+      </TagGroup>
+    );
+
+    let tags = getAllByRole('row');
+    expect(tags).toHaveLength(3);
+    
+    // Initially, nothing should be focused
+    expect(document.activeElement).not.toBe(tags[0]);
+    
+    // Tab to focus the first tag
+    await user.tab();
+    expect(document.activeElement).toBe(tags[0]);
+    
+    // Check that we can focus each tag with keyboard
+    // Instead of using tab, let's verify we can move with arrow keys
+    await user.keyboard('{ArrowRight}');
+    expect(document.activeElement).toBe(tags[1]);
+    
+    await user.keyboard('{ArrowRight}');
+    expect(document.activeElement).toBe(tags[2]);
+    
+    // Test we can go back
+    await user.keyboard('{ArrowLeft}');
+    expect(document.activeElement).toBe(tags[1]);
+  });
+
+  it('should support tabbing to remove buttons', async () => {
+    let onRemove = jest.fn();
+    let {getAllByRole, getAllByText} = render(
+      <TagGroup
+        label="Amenities"
+        onRemove={onRemove}>
+        <Item key="laundry">Laundry</Item>
+        <Item key="fitness">Fitness center</Item>
+        <Item key="parking">Parking</Item>
+      </TagGroup>
+    );
+
+    let tags = getAllByRole('row');
+    let removeButtons = getAllByText('x');
+    expect(removeButtons).toHaveLength(3);
+    
+    // Tab to focus the first tag
+    await user.tab();
+    expect(document.activeElement).toBe(tags[0]);
+    
+    // Tab to focus the first remove button
+    await user.tab();
+    expect(document.activeElement).toBe(removeButtons[0]);
+    
+    // Test remove button functionality
+    await user.keyboard(' '); // Press space to activate button
+    expect(onRemove).toHaveBeenCalledTimes(1);
+    expect(onRemove).toHaveBeenLastCalledWith(new Set(['laundry']));
+  });
+
+  it('should support keyboard selection while navigating between tags', async () => {
+    let onSelectionChange = jest.fn();
+    let {getAllByRole} = render(
+      <TagGroup
+        label="Amenities"
+        selectionMode="multiple"
+        onSelectionChange={onSelectionChange}>
+        <Item key="laundry">Laundry</Item>
+        <Item key="fitness">Fitness center</Item>
+        <Item key="parking">Parking</Item>
+      </TagGroup>
+    );
+
+    let tags = getAllByRole('row');
+    
+    // Tab to focus the first tag
+    await user.tab();
+    expect(document.activeElement).toBe(tags[0]);
+    
+    // Press space to select the first tag
+    await user.keyboard(' ');
+    expect(onSelectionChange).toHaveBeenCalledTimes(1);
+    
+    // Move to the second tag using arrow key
+    await user.keyboard('{ArrowRight}');
+    expect(document.activeElement).toBe(tags[1]);
+    
+    // Press space to select the second tag
+    await user.keyboard(' ');
+    expect(onSelectionChange).toHaveBeenCalledTimes(2);
+    
+    // Move to the third tag
+    await user.keyboard('{ArrowRight}');
+    expect(document.activeElement).toBe(tags[2]);
+  });
 });

packages/react-aria-components/stories/GridList.stories.tsx

@@ -10,12 +10,12 @@
  * governing permissions and limitations under the License.
  */
 
+import {action} from '@storybook/addon-actions';
 import {Button, Checkbox, CheckboxProps, DropIndicator, GridLayout, GridList, GridListItem, GridListItemProps, ListLayout, Size, Tag, TagGroup, TagList, useDragAndDrop, Virtualizer} from 'react-aria-components';
 import {classNames} from '@react-spectrum/utils';
 import React from 'react';
 import styles from '../example/index.css';
 import {useListData} from 'react-stately';
-
 export default {
   title: 'React Aria Components'
 };
@@ -187,11 +187,18 @@ export function TagGroupInsideGridList() {
       }}>
       <MyGridListItem textValue="Tags">
         1,1
-        <TagGroup aria-label="Tag group">
+        <TagGroup aria-label="Tag group 1" onRemove={action('onRemove')}>
           <TagList style={{display: 'flex', gap: 10}}>
-            <Tag key="1">Tag 1</Tag>
-            <Tag key="2">Tag 2</Tag>
-            <Tag key="3">Tag 3</Tag>
+            <Tag key="1">Tag 1<Button slot="remove">X</Button></Tag>
+            <Tag key="2">Tag 2<Button slot="remove">X</Button></Tag>
+            <Tag key="3">Tag 3<Button slot="remove">X</Button></Tag>
+          </TagList>
+        </TagGroup>
+        <TagGroup aria-label="Tag group 2" onRemove={action('onRemove')}>
+          <TagList style={{display: 'flex', gap: 10}}>
+            <Tag key="1">Tag 1<Button slot="remove">X</Button></Tag>
+            <Tag key="2">Tag 2<Button slot="remove">X</Button></Tag>
+            <Tag key="3">Tag 3<Button slot="remove">X</Button></Tag>
           </TagList>
         </TagGroup>
       </MyGridListItem>

packages/react-aria-components/stories/TagGroup.stories.tsx

@@ -10,67 +10,103 @@
  * governing permissions and limitations under the License.
  */
 
-import {Label, OverlayArrow, Tag, TagGroup, TagGroupProps, TagList, TagProps, Tooltip, TooltipTrigger} from 'react-aria-components';
+import {action} from '@storybook/addon-actions';
+import {Button, Label, OverlayArrow, Tag, TagGroup, TagGroupProps, TagList, TagProps, Tooltip, TooltipTrigger} from 'react-aria-components';
+import {Meta, StoryObj} from '@storybook/react';
 import React from 'react';
 
-export default {
-  title: 'React Aria Components'
-};
-
-export const TagGroupExample = (props: TagGroupProps) => (
-  <TagGroup {...props}>
-    <Label>Categories</Label>
-    <TagList style={{display: 'flex', gap: 4}}>
-      <MyTag href="https://nytimes.com">News</MyTag>
-      <MyTag>Travel</MyTag>
-      <MyTag>Gaming</MyTag>
-      <TooltipTrigger>
-        <MyTag>Shopping</MyTag>
-        <Tooltip
-          offset={5}
-          style={{
-            background: 'Canvas',
-            color: 'CanvasText',
-            border: '1px solid gray',
-            padding: 5,
-            borderRadius: 4
-          }}>
-          <OverlayArrow style={{transform: 'translateX(-50%)'}}>
-            <svg width="8" height="8" style={{display: 'block'}}>
-              <path d="M0 0L4 4L8 0" fill="white" strokeWidth={1} stroke="gray" />
-            </svg>
-          </OverlayArrow>
-          I am a tooltip
-        </Tooltip>
-      </TooltipTrigger>
-    </TagList>
-  </TagGroup>
-);
-
-TagGroupExample.args = {
-  selectionMode: 'none',
-  selectionBehavior: 'toggle'
-};
-
-TagGroupExample.argTypes = {
-  selectionMode: {
-    control: {
-      type: 'inline-radio',
-      options: ['none', 'single', 'multiple']
-    }
+const meta: Meta<typeof TagGroup> = {
+  title: 'React Aria Components',
+  component: TagGroup,
+  args: {
+    selectionMode: 'none',
+    selectionBehavior: 'toggle'
   },
-  selectionBehavior: {
-    control: {
-      type: 'inline-radio',
+  argTypes: {
+    selectionMode: {
+      control: 'inline-radio',
+      options: ['none', 'single', 'multiple']
+    },
+    selectionBehavior: {
+      control: 'inline-radio',
       options: ['toggle', 'replace']
     }
   }
 };
 
+export default meta;
+type Story = StoryObj<typeof TagGroup>;
+
+export const TagGroupExample: Story = {
+  render: (props: TagGroupProps) => (
+    <TagGroup {...props}>
+      <Label>Categories</Label>
+      <TagList style={{display: 'flex', gap: 4}}>
+        <MyTag href="https://nytimes.com">News</MyTag>
+        <MyTag>Travel</MyTag>
+        <MyTag>Gaming</MyTag>
+        <TooltipTrigger>
+          <MyTag>Shopping</MyTag>
+          <Tooltip
+            offset={5}
+            style={{
+              background: 'Canvas',
+              color: 'CanvasText',
+              border: '1px solid gray',
+              padding: 5,
+              borderRadius: 4
+            }}>
+            <OverlayArrow style={{transform: 'translateX(-50%)'}}>
+              <svg width="8" height="8" style={{display: 'block'}}>
+                <path d="M0 0L4 4L8 0" fill="white" strokeWidth={1} stroke="gray" />
+              </svg>
+            </OverlayArrow>
+            I am a tooltip
+          </Tooltip>
+        </TooltipTrigger>
+      </TagList>
+    </TagGroup>
+  )
+};
+
+
 function MyTag(props: TagProps) {
   return (
     <Tag
       {...props}
       style={({isSelected}) => ({border: '1px solid gray', borderRadius: 4, padding: '0 4px', background: isSelected ? 'black' : '', color: isSelected ? 'white' : '', cursor: props.href ? 'pointer' : 'default'})} />
   );
 }
+
+
+export const TagGroupExampleWithRemove: Story = {
+  render: (props: TagGroupProps) => (
+    <TagGroup {...props} onRemove={action('onRemove')}>
+      <Label>Categories</Label>
+      <TagList style={{display: 'flex', gap: 4}}>
+        <MyTag>Marsupial<Button slot="remove">X</Button></MyTag>
+        <MyTag>Animal<Button slot="remove">X</Button></MyTag>
+        <MyTag>Mammal<Button slot="remove">X</Button></MyTag>
+        <TooltipTrigger>
+          <MyTag>Chordate<Button slot="remove">X</Button></MyTag>
+          <Tooltip
+            offset={5}
+            style={{
+              background: 'Canvas',
+              color: 'CanvasText',
+              border: '1px solid gray',
+              padding: 5,
+              borderRadius: 4
+            }}>
+            <OverlayArrow style={{transform: 'translateX(-50%)'}}>
+              <svg width="8" height="8" style={{display: 'block'}}>
+                <path d="M0 0L4 4L8 0" fill="white" strokeWidth={1} stroke="gray" />
+              </svg>
+            </OverlayArrow>
+            I am a tooltip
+          </Tooltip>
+        </TooltipTrigger>
+      </TagList>
+    </TagGroup>
+  )
+};