Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Correct roles and add missing attributes #1314

Closed
wants to merge 9 commits into from
5 changes: 5 additions & 0 deletions .changeset/light-rats-enjoy.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/components": patch
---

fix: Correct `role`s and add missing attributes to improve `DropdownMenu`, `SelectMenu`, and `ActionMenu` accessibility.
73 changes: 35 additions & 38 deletions src/ActionList/Item.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {CheckIcon, IconProps} from '@primer/octicons-react'
import React, {useCallback} from 'react'
import React, {useCallback, useMemo} from 'react'
import {get} from '../constants'
import sx, {SxProp} from '../sx'
import Truncate from '../Truncate'
Expand Down Expand Up @@ -151,9 +151,13 @@ const getItemVariant = (variant = 'default', disabled?: boolean) => {
}
}

const StyledItemContent = styled.div`
const StyledItemContent = styled.div<{
descriptionVariant: ItemProps['descriptionVariant']
}>`
align-items: baseline;
display: flex;
min-width: 0;
flex-direction: ${({descriptionVariant}) => (descriptionVariant === 'inline' ? 'row' : 'column')};
flex-grow: 1;
position: relative;
`
Expand Down Expand Up @@ -229,16 +233,9 @@ const StyledItem = styled.div<
${sx}
`

export const TextContainer = styled.div<{
export const TextContainer = styled.span<{
dangerouslySetInnerHtml?: React.DOMAttributes<HTMLDivElement>['dangerouslySetInnerHTML']
descriptionVariant: ItemProps['descriptionVariant']
}>`
display: flex;
min-width: 0;
flex-grow: 1;
flex-direction: ${({descriptionVariant}) => (descriptionVariant === 'inline' ? 'row' : 'column')};
align-items: baseline;
`
}>``

const BaseVisualContainer = styled.div<{variant?: ItemProps['variant']; disabled?: boolean}>`
// Match visual height to adjacent text line height.
Expand Down Expand Up @@ -310,10 +307,12 @@ export function Item(itemProps: Partial<ItemProps> & {item?: ItemInput}): JSX.El
onKeyPress,
children,
onClick,
id,
id: _id,
...props
} = itemProps

const id = useMemo(() => _id ?? uniqueId(), [_id])

const keyPressHandler = useCallback(
event => {
if (disabled) {
Expand Down Expand Up @@ -358,6 +357,8 @@ export function Item(itemProps: Partial<ItemProps> & {item?: ItemInput}): JSX.El
variant={variant}
showDivider={showDivider}
aria-selected={selected}
aria-labelledby={text ? `${id}-label` : undefined}
aria-describedby={description ? `${id}-description` : undefined}
{...props}
data-id={id}
onKeyPress={keyPressHandler}
Expand Down Expand Up @@ -393,35 +394,31 @@ export function Item(itemProps: Partial<ItemProps> & {item?: ItemInput}): JSX.El
<LeadingVisual />
</LeadingVisualContainer>
)}
<StyledItemContent>
<StyledItemContent descriptionVariant={descriptionVariant}>
{children}
{(text || description) && (
<TextContainer descriptionVariant={descriptionVariant}>
{text && <div>{text}</div>}
{description && (
<DescriptionContainer descriptionVariant={descriptionVariant}>
{descriptionVariant === 'block' ? (
description
) : (
<Truncate title={description} inline={true} maxWidth="100%">
{description}
</Truncate>
)}
</DescriptionContainer>
)}
</TextContainer>
)}
{(TrailingIcon || trailingText) && (
<TrailingVisualContainer variant={variant} disabled={disabled}>
{trailingText && <div>{trailingText}</div>}
{TrailingIcon && (
<div>
<TrailingIcon />
</div>
{text ? <TextContainer id={`${id}-label`}>{text}</TextContainer> : null}
{description ? (
<DescriptionContainer id={`${id}-description`} descriptionVariant={descriptionVariant}>
{descriptionVariant === 'block' ? (
description
) : (
<Truncate title={description} inline={true} maxWidth="100%">
{description}
</Truncate>
)}
</TrailingVisualContainer>
)}
</DescriptionContainer>
) : null}
</StyledItemContent>
{(TrailingIcon || trailingText) && (
<TrailingVisualContainer variant={variant} disabled={disabled}>
{trailingText && <div>{trailingText}</div>}
{TrailingIcon && (
<div>
<TrailingIcon />
</div>
)}
</TrailingVisualContainer>
)}
</StyledItem>
)
}
6 changes: 3 additions & 3 deletions src/ActionList/List.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ function useListVariant(variant: ListProps['variant'] = 'inset'): {
/**
* Lists `Item`s, either grouped or ungrouped, with a `Divider` between each `Group`.
*/
export function List(props: ListProps): JSX.Element {
export const List = React.forwardRef<HTMLDivElement, ListProps>((props, ref): JSX.Element => {
// Get `sx` prop values for `List` children matching the given `List` style variation.
const {firstGroupStyle, lastGroupStyle, headerStyle, itemStyle} = useListVariant(props.variant)

Expand Down Expand Up @@ -216,7 +216,7 @@ export function List(props: ListProps): JSX.Element {
}

return (
<StyledList {...props}>
<StyledList {...props} ref={ref}>
{groups.map(({header, ...groupProps}, index) => {
const hasFilledHeader = header?.variant === 'filled'
const shouldShowDivider = index > 0 && !hasFilledHeader
Expand All @@ -242,4 +242,4 @@ export function List(props: ListProps): JSX.Element {
})}
</StyledList>
)
}
})
6 changes: 3 additions & 3 deletions src/ActionMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,9 @@ const ActionMenuBase = ({
}
return <T extends React.HTMLAttributes<HTMLElement>>(props: T) => {
return renderAnchor({
'aria-label': 'menu',
children: anchorContent,
...props
...props,
'aria-haspopup': 'menu'
})
}
}, [anchorContent, renderAnchor])
Expand Down Expand Up @@ -122,7 +122,7 @@ const ActionMenuBase = ({
open={combinedOpenState}
onOpen={onOpen}
onClose={onClose}
overlayProps={overlayProps}
overlayProps={{...overlayProps, role: 'menu'}}
>
<List {...listProps} role="menu" items={itemsToRender} />
</AnchoredOverlay>
Expand Down
9 changes: 7 additions & 2 deletions src/AnchoredOverlay/AnchoredOverlay.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ export const AnchoredOverlay: React.FC<AnchoredOverlayProps> = ({
const anchorRef = useProvidedRefOrCreate(externalAnchorRef)
const [overlayRef, updateOverlayRef] = useRenderForcingRef<HTMLDivElement>()
const anchorId = useMemo(uniqueId, [])
const overlayId = useMemo(() => `${anchorId}-overlay`, [anchorId])

const onClickOutside = useCallback(() => onClose?.('click-outside'), [onClose])
const onEscape = useCallback(() => onClose?.('escape'), [onClose])
Expand Down Expand Up @@ -141,19 +142,23 @@ export const AnchoredOverlay: React.FC<AnchoredOverlayProps> = ({
ref: anchorRef,
id: anchorId,
'aria-labelledby': anchorId,
'aria-haspopup': 'listbox',
'aria-haspopup': true,
'aria-expanded': open,
'aria-controls': overlayId,
tabIndex: 0,
onClick: onAnchorClick,
onKeyDown: onAnchorKeyDown
})}
{open ? (
<Overlay
id={overlayId}
returnFocusRef={anchorRef}
onClickOutside={onClickOutside}
ignoreClickRefs={[anchorRef]}
onEscape={onEscape}
ref={updateOverlayRef}
role="listbox"
role="dialog"
aria-labelledby={anchorId}
visibility={position ? 'visible' : 'hidden'}
height={height}
width={width}
Expand Down
2 changes: 1 addition & 1 deletion src/DropdownMenu/DropdownButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ export type DropdownButtonProps = ButtonProps

export const DropdownButton = React.forwardRef<HTMLElement, React.PropsWithChildren<DropdownButtonProps>>(
({children, ...props}: React.PropsWithChildren<DropdownButtonProps>, ref): JSX.Element => (
<Button ref={ref} {...props}>
<Button ref={ref} type="button" {...props}>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is a standard html button, we shouldn't have to define the type should we?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think an HTML button defaults to type="submit" which is for form submission, but for javascript-y behavior, we'd want to specify type="button".

{children}
<StyledOcticon icon={TriangleDownIcon} ml={1} />
</Button>
Expand Down
10 changes: 7 additions & 3 deletions src/DropdownMenu/DropdownMenu.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, {useCallback, useMemo, useState} from 'react'
import React, {useCallback, useMemo, useState, useRef} from 'react'
import {List, GroupedListProps, ListPropsBase, ItemInput} from '../ActionList/List'
import {DropdownButton, DropdownButtonProps} from './DropdownButton'
import {ItemProps} from '../ActionList/Item'
Expand Down Expand Up @@ -57,7 +57,8 @@ export function DropdownMenu({
<T extends React.HTMLAttributes<HTMLElement>>(props: T) => {
return renderAnchor({
...props,
children: selectedItem?.text ?? placeholder
children: selectedItem?.text ?? placeholder,
'aria-haspopup': 'listbox'
})
},
[placeholder, renderAnchor, selectedItem?.text]
Expand All @@ -83,15 +84,18 @@ export function DropdownMenu({
})
}, [items, onChange, onClose, selectedItem])

const listRef = useRef<HTMLDivElement>(null)

return (
<AnchoredOverlay
renderAnchor={renderMenuAnchor}
open={open}
onOpen={onOpen}
onClose={onClose}
overlayProps={overlayProps}
focusTrapSettings={{initialFocusRef: listRef}}
>
<List {...listProps} role="listbox" items={itemsToRender} />
<List {...listProps} ref={listRef} role="listbox" items={itemsToRender} />
</AnchoredOverlay>
)
}
Expand Down