different visual styles for keyboard navigation (#1603)

primer · Nov 15, 2021 · f4057b8 · f4057b8
1 parent a2e195b
commit f4057b8
Show file tree

Hide file tree

Showing 3 changed files with 82 additions and 57 deletions.
diff --git a/.changeset/actionlist-keyboard-a11y.md b/.changeset/actionlist-keyboard-a11y.md
@@ -0,0 +1,5 @@
+---
+'@primer/components': patch
+---
+
+ActionList: Improve keyboard accessibility with focus styles cross browser
diff --git a/src/ActionList2/Item.tsx b/src/ActionList2/Item.tsx
@@ -94,7 +94,7 @@ export const Item = React.forwardRef<HTMLLIElement, ItemProps>(
       onSelect = () => null,
       sx: sxProp = {},
       id,
-      _PrivateItemWrapper = ({children}) => <>{children}</>,
+      _PrivateItemWrapper,
       ...props
     },
     forwardedRef
@@ -109,9 +109,9 @@ export const Item = React.forwardRef<HTMLLIElement, ItemProps>(
       fontSize: 1,
       paddingY: '6px', // custom value off the scale
       lineHeight: TEXT_ROW_HEIGHT,
-      marginX: listVariant === 'inset' ? 2 : 0,
       minHeight: 5,
-      borderRadius: 2,
+      marginX: listVariant === 'inset' ? 2 : 0,
+      borderRadius: listVariant === 'inset' ? 2 : 0,
       transition: 'background 33.333ms linear',
       color: getVariantStyles(variant, disabled).color,
       textDecoration: 'none', // for as="a"
@@ -121,11 +121,17 @@ export const Item = React.forwardRef<HTMLLIElement, ItemProps>(
           backgroundColor: `actionListItem.${variant}.hoverBg`,
           color: getVariantStyles(variant, disabled).hoverColor
         },
-        ':focus:not([aria-disabled])': {
+        ':focus:not([data-focus-visible-added])': {
           backgroundColor: `actionListItem.${variant}.selectedBg`,
           color: getVariantStyles(variant, disabled).hoverColor,
           outline: 'none'
         },
+        '&[data-focus-visible-added]': {
+          // we don't use :focus-visible because not all browsers (safari) have it yet
+          outline: 'none',
+          border: `2 solid`,
+          boxShadow: `0 0 0 2px ${theme?.colors.accent.emphasis}`
+        },
         ':active:not([aria-disabled])': {
           backgroundColor: `actionListItem.${variant}.activeBg`,
           color: getVariantStyles(variant, disabled).hoverColor
@@ -147,14 +153,16 @@ export const Item = React.forwardRef<HTMLLIElement, ItemProps>(
         borderColor: 'var(--divider-color, transparent)'
       },
       // show between 2 items
-      ':not(:first-of-type):not([aria-selected=true])': {'--divider-color': theme?.colors.actionListItem.inlineDivider},
+      ':not(:first-of-type)': {'--divider-color': theme?.colors.actionListItem.inlineDivider},
       // hide divider after dividers & group header, with higher importance!
       '[data-component="ActionList.Divider"] + &': {'--divider-color': 'transparent !important'},
       // hide border on current and previous item
-      '&:hover:not([aria-disabled]), &:focus:not([aria-disabled])': {'--divider-color': 'transparent'},
-      '&:hover:not([aria-disabled]) + &, &:focus:not([aria-disabled]) + &': {'--divider-color': 'transparent'},
-      // hide border around selected item
-      '&[aria-selected=true] + &': {'--divider-color': 'transparent'}
+      '&:hover:not([aria-disabled]), &:focus:not([aria-disabled]), &[data-focus-visible-added]:not([aria-disabled])': {
+        '--divider-color': 'transparent'
+      },
+      '&:hover:not([aria-disabled]) + &, &:focus:not([aria-disabled]) + &, &[data-focus-visible-added] + li': {
+        '--divider-color': 'transparent'
+      }
     }
 
     const clickHandler = React.useCallback(
@@ -165,26 +173,40 @@ export const Item = React.forwardRef<HTMLLIElement, ItemProps>(
       [onSelect, disabled]
     )
 
+    const keyPressHandler = React.useCallback(
+      event => {
+        if (disabled) return
+
+        if (!event.defaultPrevented && [' ', 'Enter'].includes(event.key)) {
+          onSelect(event)
+        }
+      },
+      [onSelect, disabled]
+    )
+
     // use props.id if provided, otherwise generate one.
     const labelId = useSSRSafeId(id)
     const inlineDescriptionId = useSSRSafeId(id && `${id}--inline-description`)
     const blockDescriptionId = useSSRSafeId(id && `${id}--block-description`)
 
+    const ItemWrapper = _PrivateItemWrapper || React.Fragment
+
     return (
       <Slots context={{variant, disabled, inlineDescriptionId, blockDescriptionId}}>
         {slots => (
           <LiBox
             ref={forwardedRef}
             sx={merge(styles, sxProp as SxProp)}
             onClick={clickHandler}
+            onKeyPress={keyPressHandler}
             aria-selected={selected}
             aria-disabled={disabled ? true : undefined}
-            tabIndex={disabled ? undefined : -1}
+            tabIndex={disabled || _PrivateItemWrapper ? undefined : 0}
             aria-labelledby={`${labelId} ${slots.InlineDescription ? inlineDescriptionId : ''}`}
             aria-describedby={slots.BlockDescription ? blockDescriptionId : undefined}
             {...props}
           >
-            <_PrivateItemWrapper>
+            <ItemWrapper>
               <Selection selected={selected} disabled={disabled} />
               {slots.LeadingVisual}
               <Box
@@ -205,7 +227,7 @@ export const Item = React.forwardRef<HTMLLIElement, ItemProps>(
                 </ConditionalBox>
                 {slots.BlockDescription}
               </Box>
-            </_PrivateItemWrapper>
+            </ItemWrapper>
           </LiBox>
         )}
       </Slots>

diff --git a/src/stories/ActionList2.stories.tsx b/src/stories/ActionList2.stories.tsx
@@ -73,15 +73,14 @@ export function SimpleListStory(): JSX.Element {
   return (
     <>
       <h1>Simple List</h1>
-      <ErsatzOverlay>
-        <ActionList>
-          <ActionList.Item>Copy link</ActionList.Item>
-          <ActionList.Item>Quote reply</ActionList.Item>
-          <ActionList.Item>Edit comment</ActionList.Item>
-          <ActionList.Divider />
-          <ActionList.Item variant="danger">Delete file</ActionList.Item>
-        </ActionList>
-      </ErsatzOverlay>
+
+      <ActionList>
+        <ActionList.Item>Copy link</ActionList.Item>
+        <ActionList.Item>Quote reply</ActionList.Item>
+        <ActionList.Item>Edit comment</ActionList.Item>
+        <ActionList.Divider />
+        <ActionList.Item variant="danger">Delete file</ActionList.Item>
+      </ActionList>
     </>
   )
 }
@@ -91,40 +90,39 @@ export function WithIcon(): JSX.Element {
   return (
     <>
       <h1>With Icon</h1>
-      <ErsatzOverlay>
-        <ActionList>
-          <ActionList.Item>
-            <ActionList.LeadingVisual>
-              <LinkIcon />
-            </ActionList.LeadingVisual>
-            github.com/primer
-          </ActionList.Item>
-          <ActionList.Item>
-            <ActionList.LeadingVisual>
-              <LawIcon />
-            </ActionList.LeadingVisual>
-            MIT License
-          </ActionList.Item>
-          <ActionList.Item>
-            <ActionList.LeadingVisual>
-              <StarIcon />
-            </ActionList.LeadingVisual>
-            256 stars
-          </ActionList.Item>
-          <ActionList.Item>
-            <ActionList.LeadingVisual>
-              <GitForkIcon />
-            </ActionList.LeadingVisual>
-            3 forks
-          </ActionList.Item>
-          <ActionList.Item variant="danger">
-            <ActionList.LeadingVisual>
-              <AlertIcon />
-            </ActionList.LeadingVisual>
-            4 vulnerabilities
-          </ActionList.Item>
-        </ActionList>
-      </ErsatzOverlay>
+
+      <ActionList>
+        <ActionList.Item>
+          <ActionList.LeadingVisual>
+            <LinkIcon />
+          </ActionList.LeadingVisual>
+          github.com/primer
+        </ActionList.Item>
+        <ActionList.Item>
+          <ActionList.LeadingVisual>
+            <LawIcon />
+          </ActionList.LeadingVisual>
+          MIT License
+        </ActionList.Item>
+        <ActionList.Item>
+          <ActionList.LeadingVisual>
+            <StarIcon />
+          </ActionList.LeadingVisual>
+          256 stars
+        </ActionList.Item>
+        <ActionList.Item>
+          <ActionList.LeadingVisual>
+            <GitForkIcon />
+          </ActionList.LeadingVisual>
+          3 forks
+        </ActionList.Item>
+        <ActionList.Item variant="danger">
+          <ActionList.LeadingVisual>
+            <AlertIcon />
+          </ActionList.LeadingVisual>
+          4 vulnerabilities
+        </ActionList.Item>
+      </ActionList>
     </>
   )
 }
@@ -637,7 +635,7 @@ export function DOMPropsStory(): JSX.Element {
       <h1>Simple List</h1>
       <ErsatzOverlay>
         <ActionList>
-          <ActionList.Item id="something" onClick={event => alert(`Id is '${event.currentTarget.getAttribute('id')}'`)}>
+          <ActionList.Item id="something" onClick={event => alert(`Id is '${event.target.id}'`)}>
             Has an id
           </ActionList.Item>
         </ActionList>
@@ -1185,7 +1183,7 @@ const SortableItem: React.FC<SortableItemProps> = ({option, onSelect, reorder})
       sx={{
         opacity: isDragging ? 0.5 : 1,
         boxShadow: isOver ? theme => `0px 2px 0 0px ${theme.colors.accent.emphasis}` : undefined,
-        borderRadius: isOver ? 0 : undefined
+        borderRadius: isOver ? 0 : 2
       }}
     >
       <ActionList.LeadingVisual>{option.icon}</ActionList.LeadingVisual>