From f4057b897cc4c23012fb1b1881cefacbb57e081d Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Mon, 15 Nov 2021 15:09:33 +0100 Subject: [PATCH] different visual styles for keyboard navigation (#1603) --- .changeset/actionlist-keyboard-a11y.md | 5 ++ src/ActionList2/Item.tsx | 46 ++++++++++---- src/stories/ActionList2.stories.tsx | 88 +++++++++++++------------- 3 files changed, 82 insertions(+), 57 deletions(-) create mode 100644 .changeset/actionlist-keyboard-a11y.md diff --git a/.changeset/actionlist-keyboard-a11y.md b/.changeset/actionlist-keyboard-a11y.md new file mode 100644 index 00000000000..783a7875609 --- /dev/null +++ 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 index ceba9a841d4..b7ddd462b9d 100644 --- a/src/ActionList2/Item.tsx +++ b/src/ActionList2/Item.tsx @@ -94,7 +94,7 @@ export const Item = React.forwardRef( onSelect = () => null, sx: sxProp = {}, id, - _PrivateItemWrapper = ({children}) => <>{children}, + _PrivateItemWrapper, ...props }, forwardedRef @@ -109,9 +109,9 @@ export const Item = React.forwardRef( 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( 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( 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,11 +173,24 @@ export const Item = React.forwardRef( [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 => ( @@ -177,14 +198,15 @@ export const Item = React.forwardRef( 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> + {slots.LeadingVisual} ( {slots.BlockDescription} - + )} diff --git a/src/stories/ActionList2.stories.tsx b/src/stories/ActionList2.stories.tsx index 069642f65ed..7adebe0d748 100644 --- a/src/stories/ActionList2.stories.tsx +++ b/src/stories/ActionList2.stories.tsx @@ -73,15 +73,14 @@ export function SimpleListStory(): JSX.Element { return ( <>

Simple List

- - - Copy link - Quote reply - Edit comment - - Delete file - - + + + Copy link + Quote reply + Edit comment + + Delete file + ) } @@ -91,40 +90,39 @@ export function WithIcon(): JSX.Element { return ( <>

With Icon

- - - - - - - github.com/primer - - - - - - MIT License - - - - - - 256 stars - - - - - - 3 forks - - - - - - 4 vulnerabilities - - - + + + + + + + github.com/primer + + + + + + MIT License + + + + + + 256 stars + + + + + + 3 forks + + + + + + 4 vulnerabilities + + ) } @@ -637,7 +635,7 @@ export function DOMPropsStory(): JSX.Element {

Simple List

- alert(`Id is '${event.currentTarget.getAttribute('id')}'`)}> + alert(`Id is '${event.target.id}'`)}> Has an id @@ -1185,7 +1183,7 @@ const SortableItem: React.FC = ({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 }} > {option.icon}