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.

Sign up for GitHub

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

feat(Row): improve VoiceOver compatibility #1079

Merged
merged 6 commits into from
Apr 29, 2024
Merged

feat(Row): improve VoiceOver compatibility #1079

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2aa6b6e
WEB-1724 feat(Row): improve VoiceOver compatibility
atabel Apr 11, 2024
2fc4a14
fix test
atabel Apr 11, 2024
0b3e27d
fix titleAs in Rows
atabel Apr 12, 2024
f9dede2
increase sheet tests timeout
atabel Apr 12, 2024
f74083f
Merge branch 'master' into atoledano-screen-reader
atabel Apr 19, 2024
d67f944
improve reading order and extract extra text for link aria-label
atabel Apr 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/__tests__/list-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,9 +152,8 @@ test('Row list with radio buttons', async () => {
</ThemeContextProvider>
);

const radioBanana = screen.getByRole('radio', {name: 'Banana'});
// alternate accessible selector
const radioApple = screen.getByLabelText('Apple');
const radioBanana = screen.getByRole('radio', {name: 'Banana bananabanana'});
const radioApple = screen.getByRole('radio', {name: 'Apple appleapple'});

expect(radioBanana).not.toBeChecked();
expect(radioApple).not.toBeChecked();
Expand Down
3 changes: 3 additions & 0 deletions src/box.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ type Props = {
/** "data-" prefix is automatically added. For example, use "testid" instead of "data-testid" */
dataAttributes?: DataAttributes;
'aria-label'?: string;
'aria-hidden'?: React.HTMLAttributes<HTMLAnchorElement>['aria-hidden'];
};

const Box = React.forwardRef<HTMLDivElement, Props>(
Expand All @@ -40,6 +41,7 @@ const Box = React.forwardRef<HTMLDivElement, Props>(
role,
dataAttributes,
'aria-label': ariaLabel,
'aria-hidden': ariaHidden,
},
ref
) => {
Expand All @@ -64,6 +66,7 @@ const Box = React.forwardRef<HTMLDivElement, Props>(
{...getPrefixedDataAttributes(dataAttributes)}
role={role}
aria-label={ariaLabel}
aria-hidden={ariaHidden}
ref={ref}
className={classnames(className, paddingClasses)}
style={{
Expand Down
4 changes: 2 additions & 2 deletions src/icons/icon-error.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const IconErrorO2 = ({size = 48}: Props): JSX.Element => {
const {platformOverrides} = useTheme();

return (
<svg width={size} height={size} viewBox="0 0 64 64" overflow="visible">
<svg role="presentation" width={size} height={size} viewBox="0 0 64 64" overflow="visible">
<g
stroke={vars.colors.error}
fill={vars.colors.error}
Expand Down Expand Up @@ -62,7 +62,7 @@ const IconErrorDefault = ({size = 48}: Props): JSX.Element => {
const {platformOverrides} = useTheme();

return (
<svg width={size} height={size} viewBox="0 0 64 64" overflow="visible">
<svg role="presentation" width={size} height={size} viewBox="0 0 64 64" overflow="visible">
<g
stroke={vars.colors.error}
fill="none"
Expand Down
4 changes: 2 additions & 2 deletions src/icons/icon-info.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ type Props = {

const IconInfoO2 = ({size = 48}: Props): JSX.Element => {
return (
<svg width={size} height={size} viewBox="0 0 64 64">
<svg role="presentation" width={size} height={size} viewBox="0 0 64 64">
<g
stroke={vars.colors.brand}
fill={vars.colors.brand}
Expand All @@ -32,7 +32,7 @@ const IconInfoO2 = ({size = 48}: Props): JSX.Element => {

const IconInfoDefault = ({size = 48}: Props): JSX.Element => {
return (
<svg width={size} height={size} viewBox="0 0 64 64">
<svg role="presentation" width={size} height={size} viewBox="0 0 64 64">
<g fill={vars.colors.brand}>
<path
fillRule="nonzero"
Expand Down
2 changes: 1 addition & 1 deletion src/icons/icon-success-vivo-new.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ const IconSuccessVivoNew = ({size = 48}: Props): JSX.Element => {
const gradientId = useAriaId();

return (
<svg width={size} height={size} viewBox="0 0 64 64" fill="none">
<svg role="presentation" width={size} height={size} viewBox="0 0 64 64" fill="none">
<circle
cx="32"
cy="32"
Expand Down
2 changes: 1 addition & 1 deletion src/icons/icon-success-vivo.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const IconSuccessVivo = ({size = 48}: Props): JSX.Element => {
const {platformOverrides} = useTheme();

return (
<svg width={size} height={size} viewBox="0 0 64 64" overflow="visible">
<svg role="presentation" width={size} height={size} viewBox="0 0 64 64" overflow="visible">
<g
fill={isInverse ? vars.colors.inverse : vars.colors.brand}
stroke={isInverse ? vars.colors.inverse : vars.colors.brand}
Expand Down
37 changes: 26 additions & 11 deletions src/list.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ interface CommonProps {
dataAttributes?: DataAttributes;
disabled?: boolean;
withChevron?: boolean;
'aria-label'?: string;
}

type Right = (({centerY}: {centerY: boolean}) => React.ReactNode) | React.ReactNode;
Expand Down Expand Up @@ -129,6 +130,7 @@ export const Content: React.FC<ContentProps> = ({
<div
className={classNames(styles.rowBody, {[styles.disabled]: disabled})}
style={{justifyContent: centerY ? 'center' : 'flex-start'}}
id={labelId}
>
<Stack space={4}>
{headline && (
Expand All @@ -141,7 +143,6 @@ export const Content: React.FC<ContentProps> = ({
regular
color={vars.colors.textPrimary}
truncate={titleLinesMax}
id={labelId}
hyphens="auto"
as={titleAs}
>
Expand Down Expand Up @@ -350,8 +351,12 @@ const RowContent = React.forwardRef<TouchableElement, RowContentProps>((props, r
role,
extra,
dataAttributes,
'aria-label': ariaLabel,
} = props;

// iOS voiceover reads links with multiple lines as separate links. By setting aria-label and marking content as aria-hidden, we can make it read the whole row as one link.
const iosVoiceOverLabelForLink = [title, subtitle, description, detail].filter(Boolean).join(' ');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

caveat: headline is not read because it isn't included here. The problem is that headline prop is a React.Node, so I can't extract the text


const radioContext = useRadioContext();
const disabled = props.disabled || (props.radioValue !== undefined && radioContext.disabled);
const hasHoverDefault = !disabled && !isInverse;
Expand Down Expand Up @@ -388,22 +393,20 @@ const RowContent = React.forwardRef<TouchableElement, RowContentProps>((props, r
/>
);

const renderBaseTouchableContent = (
props: HrefRowContentProps | ToRowContentProps | OnPressRowContentProps
) => {
const renderBaseTouchableContent = ({hidden, right}: {hidden?: boolean; right?: Right} = {}) => {
let type: ContentProps['type'] = 'chevron';

if (props.right === null) {
if (right === null) {
type = 'basic';
}

if (props.right) {
if (right) {
type = 'custom';
}

return (
<Box paddingX={16} ref={ref as React.Ref<HTMLDivElement>}>
{renderContent({type, right: props.right})}
<Box paddingX={16} aria-hidden={hidden || undefined}>
{renderContent({type, right})}
</Box>
);
};
Expand All @@ -428,15 +431,17 @@ const RowContent = React.forwardRef<TouchableElement, RowContentProps>((props, r
role={role}
dataAttributes={dataAttributes}
disabled={disabled}
aria-label={ariaLabel}
>
{renderBaseTouchableContent(props)}
{renderBaseTouchableContent({right: props.right})}
</BaseTouchable>
);
}

if (props.to) {
return (
<BaseTouchable
ref={ref}
className={classNames(styles.rowContent, {
[styles.touchableBackground]: hasHoverDefault,
[styles.touchableBackgroundInverse]: hasHoverInverse,
Expand All @@ -448,15 +453,17 @@ const RowContent = React.forwardRef<TouchableElement, RowContentProps>((props, r
role={role}
dataAttributes={dataAttributes}
disabled={disabled}
aria-label={ariaLabel ?? iosVoiceOverLabelForLink}
>
{renderBaseTouchableContent(props)}
{renderBaseTouchableContent({right: props.right, hidden: true})}
</BaseTouchable>
);
}

if (props.href) {
return (
<BaseTouchable
ref={ref}
className={classNames(styles.rowContent, {
[styles.touchableBackground]: hasHoverDefault,
[styles.touchableBackgroundInverse]: hasHoverInverse,
Expand All @@ -468,8 +475,9 @@ const RowContent = React.forwardRef<TouchableElement, RowContentProps>((props, r
role={role}
dataAttributes={dataAttributes}
disabled={disabled}
aria-label={ariaLabel ?? iosVoiceOverLabelForLink}
atabel marked this conversation as resolved.
Show resolved Hide resolved
>
{renderBaseTouchableContent(props)}
{renderBaseTouchableContent({right: props.right, hidden: true})}
</BaseTouchable>
);
}
Expand All @@ -489,6 +497,7 @@ const RowContent = React.forwardRef<TouchableElement, RowContentProps>((props, r
[styles.touchableBackground]: hasHoverDefault,
[styles.touchableBackgroundInverse]: hasHoverInverse,
})}
aria-label={ariaLabel}
>
{renderContent({type: 'basic', labelId: titleId})}
</BaseTouchable>
Expand All @@ -498,6 +507,7 @@ const RowContent = React.forwardRef<TouchableElement, RowContentProps>((props, r
disabled={disabled}
name={name}
checked={isChecked}
aria-label={ariaLabel}
aria-labelledby={titleId}
onChange={toggle}
render={({controlElement}) => (
Expand All @@ -519,6 +529,7 @@ const RowContent = React.forwardRef<TouchableElement, RowContentProps>((props, r
name={name}
checked={isChecked}
onChange={toggle}
aria-label={ariaLabel}
render={({controlElement, labelId}) => (
<Box paddingX={16} role={role}>
{renderContent({
Expand Down Expand Up @@ -554,6 +565,7 @@ const RowContent = React.forwardRef<TouchableElement, RowContentProps>((props, r
[styles.touchableBackground]: hasHoverDefault,
[styles.touchableBackgroundInverse]: hasHoverInverse,
})}
aria-label={ariaLabel}
>
{renderContent({type: 'basic', labelId: titleId})}
</BaseTouchable>
Expand Down Expand Up @@ -604,6 +616,7 @@ const RowContent = React.forwardRef<TouchableElement, RowContentProps>((props, r
[styles.touchableBackground]: hasHoverDefault,
[styles.touchableBackgroundInverse]: hasHoverInverse,
})}
aria-label={ariaLabel}
>
{renderContent({type: 'basic', labelId: titleId})}
</BaseTouchable>
Expand All @@ -612,6 +625,7 @@ const RowContent = React.forwardRef<TouchableElement, RowContentProps>((props, r
dataAttributes={dataAttributes}
value={props.radioValue}
aria-labelledby={titleId}
aria-label={ariaLabel}
render={({controlElement}) => (
<Stack space="around">
<Box paddingX={16}>{controlElement}</Box>
Expand All @@ -633,6 +647,7 @@ const RowContent = React.forwardRef<TouchableElement, RowContentProps>((props, r
dataAttributes={dataAttributes}
value={props.radioValue}
aria-labelledby={titleId}
aria-label={ariaLabel}
render={({controlElement}) => (
<Box paddingX={16}>
{renderContent({
Expand Down
Loading
Loading