Improve accessibility of navigation components (#2297)

sumup-oss · Nov 14, 2023 · 2f4fbaf · 2f4fbaf · vercel · Nov 14, 2023
1 parent 6c536a2
commit 2f4fbaf
Show file tree

Hide file tree

Showing 21 changed files with 80 additions and 35 deletions.
diff --git a/.changeset/poor-rice-move.md b/.changeset/poor-rice-move.md
@@ -0,0 +1,5 @@
+---
+'@sumup/circuit-ui': patch
+---
+
+Fixed the HTML semantics in the TopNavigation and SideNavigation components.
diff --git a/.changeset/tasty-guests-work.md b/.changeset/tasty-guests-work.md
@@ -0,0 +1,5 @@
+---
+'@sumup/circuit-ui': patch
+---
+
+Properly hid presentational icons from the accessibility tree.
diff --git a/.changeset/weak-toes-fly.md b/.changeset/weak-toes-fly.md
@@ -0,0 +1,5 @@
+---
+'@sumup/circuit-ui': patch
+---
+
+Restored the focus styles of the utility links and profile menu in the TopNavigation.
diff --git a/packages/circuit-ui/components/Field/Field.tsx b/packages/circuit-ui/components/Field/Field.tsx
@@ -209,7 +209,7 @@ export const FieldValidationHint = ({
           <div className={classNames} {...props}>
             {Icon && (
               <div className={classes['validation-hint-icon']}>
-                <Icon role="presentation" size="16" />
+                <Icon aria-hidden="true" size="16" />
               </div>
             )}
             {validationHint}

diff --git a/packages/circuit-ui/components/ListItem/ListItem.stories.tsx b/packages/circuit-ui/components/ListItem/ListItem.stories.tsx
@@ -63,7 +63,7 @@ const Details = (
         marginRight: 'var(--cui-spacings-bit)',
         color: 'var(--cui-fg-success)',
       }}
-      role="presentation"
+      aria-hidden="true"
     />
     <Body
       size="two"

diff --git a/packages/circuit-ui/components/ListItemGroup/ListItemGroup.stories.tsx b/packages/circuit-ui/components/ListItemGroup/ListItemGroup.stories.tsx
@@ -71,7 +71,7 @@ const Details = ({ item }: { item: Item }) => (
           marginRight: 'var(--cui-spacings-bit)',
           color: 'var(--cui-fg-success)',
         }}
-        role="presentation"
+        aria-hidden="true"
       />
     ) : (
       <Alert
@@ -80,7 +80,7 @@ const Details = ({ item }: { item: Item }) => (
           marginRight: 'var(--cui-spacings-bit)',
           color: 'var(--cui-fg-danger)',
         }}
-        role="presentation"
+        aria-hidden="true"
       />
     )}
     <Body

diff --git a/packages/circuit-ui/components/NotificationInline/NotificationInline.tsx b/packages/circuit-ui/components/NotificationInline/NotificationInline.tsx
@@ -156,7 +156,7 @@ export const NotificationInline = forwardRef<
       >
         <div className={clsx(classes.wrapper, classes[variant])}>
           <div className={classes.icon}>
-            <Icon role="presentation" />
+            <Icon aria-hidden="true" />
           </div>
           <span className={utilityClasses.hideVisually}>{iconLabel}</span>
           <div className={classes.content}>

diff --git a/packages/circuit-ui/components/NotificationToast/NotificationToast.tsx b/packages/circuit-ui/components/NotificationToast/NotificationToast.tsx
@@ -110,7 +110,7 @@ export function NotificationToast({
     >
       <div className={classes.wrapper}>
         <div className={classes.icon}>
-          <Icon role="presentation" />
+          <Icon aria-hidden="true" />
         </div>
         <span className={utilityClasses.hideVisually}>{iconLabel}</span>
         <div className={classes.content}>

diff --git a/packages/circuit-ui/components/Popover/Popover.spec.tsx b/packages/circuit-ui/components/Popover/Popover.spec.tsx
@@ -264,11 +264,11 @@ describe('Popover', () => {
     });
   });
 
-  it('should render items as role=menuitem and dividers as role=presentation', async () => {
-    renderPopover(baseProps);
+  it('should render items as role=menuitem and dividers as aria-hidden', async () => {
+    const { baseElement } = renderPopover(baseProps);
 
     const items = screen.getAllByRole('menuitem');
-    const dividers = screen.getAllByRole('presentation');
+    const dividers = baseElement.querySelectorAll('hr[aria-hidden="true"');
     expect(items.length).toBe(2);
     expect(dividers.length).toBe(1);
 

diff --git a/packages/circuit-ui/components/Popover/Popover.tsx b/packages/circuit-ui/components/Popover/Popover.tsx
@@ -347,7 +347,7 @@ export const Popover = ({
               isDivider(action) ? (
                 <Hr
                   className={classes.divider}
-                  role="presentation"
+                  aria-hidden="true"
                   key={index}
                 />
               ) : (

diff --git a/...s/circuit-ui/components/SideNavigation/components/DesktopNavigation/DesktopNavigation.tsx b/...s/circuit-ui/components/SideNavigation/components/DesktopNavigation/DesktopNavigation.tsx
@@ -81,7 +81,7 @@ export function DesktopNavigation({
           className={classes.secondary}
           aria-label={secondaryNavigationLabel}
         >
-          <Skeleton className={classes.headline}>
+          <Skeleton className={classes.headline} as="div">
             <Headline as="h2" size="four">
               {activePrimaryLink && activePrimaryLink.label}
             </Headline>

diff --git a/packages/circuit-ui/components/SideNavigation/components/PrimaryLink/PrimaryLink.tsx b/packages/circuit-ui/components/SideNavigation/components/PrimaryLink/PrimaryLink.tsx
@@ -48,7 +48,7 @@ export function PrimaryLink({
   const Element = props.href ? (Link as AsPropType) : 'button';
 
   const suffix = Suffix && (
-    <Suffix className={classes.suffix} role="presentation" />
+    <Suffix className={classes.suffix} aria-hidden="true" />
   );
   const isExternalLink = isExternal || props.target === '_blank';
 
@@ -65,7 +65,7 @@ export function PrimaryLink({
       aria-current={isActive ? 'page' : undefined}
     >
       <Skeleton className={clsx(classes.icon, badge && classes['icon-badge'])}>
-        <Icon role="presentation" size="24" />
+        <Icon aria-hidden="true" size="24" />
       </Skeleton>
       <Skeleton>
         <Body as="span" className={classes.label}>
@@ -76,7 +76,7 @@ export function PrimaryLink({
       {isExternalLink && (
         <ArrowRight
           size="16"
-          role="presentation"
+          aria-hidden="true"
           className={clsx(classes.suffix, classes['external-icon'])}
         />
       )}

diff --git a/packages/circuit-ui/components/SideNavigation/components/SecondaryLinks/SecondaryLinks.tsx b/packages/circuit-ui/components/SideNavigation/components/SecondaryLinks/SecondaryLinks.tsx
@@ -51,7 +51,11 @@ function SecondaryLink({ label, badge, ...props }: SecondaryLinkProps) {
         aria-current={props.isActive ? 'page' : undefined}
       >
         <Skeleton className={classes.label}>
-          <Body size="one" variant={props.isActive ? 'highlight' : undefined}>
+          <Body
+            as="span"
+            size="one"
+            variant={props.isActive ? 'highlight' : undefined}
+          >
             {label}
           </Body>
         </Skeleton>
@@ -69,7 +73,7 @@ function SecondaryGroup({
   return (
     <li>
       {label && (
-        <Skeleton className={classes['group-headline']}>
+        <Skeleton className={classes['group-headline']} as="div">
           <SubHeadline as="h3">{label}</SubHeadline>
         </Skeleton>
       )}

diff --git a/packages/circuit-ui/components/Sidebar/components/Separator/Separator.tsx b/packages/circuit-ui/components/Sidebar/components/Separator/Separator.tsx
@@ -29,7 +29,7 @@ const hrStyles = (theme: Theme) => css`
  */
 export function Separator(): JSX.Element {
   return (
-    <li role="presentation">
+    <li aria-hidden="true">
       <Hr css={hrStyles} />
     </li>
   );

diff --git a/.../circuit-ui/components/Sidebar/components/Separator/__snapshots__/Separator.spec.tsx.snap b/.../circuit-ui/components/Sidebar/components/Separator/__snapshots__/Separator.spec.tsx.snap
@@ -6,7 +6,7 @@ exports[`Separator > should render with default styles 1`] = `
 }
 
 <li
-  role="presentation"
+  aria-hidden="true"
 >
   <hr
     class="_base_5bcdd0 circuit-0"

diff --git a/packages/circuit-ui/components/Skeleton/Skeleton.tsx b/packages/circuit-ui/components/Skeleton/Skeleton.tsx
@@ -21,6 +21,7 @@ import {
   HTMLAttributes,
 } from 'react';
 
+import type { AsPropType } from '../../types/prop-types.js';
 import { clsx } from '../../styles/clsx.js';
 
 import classes from './Skeleton.module.css';
@@ -75,19 +76,23 @@ export interface SkeletonProps extends HTMLAttributes<HTMLSpanElement> {
    * Default: `false`.
    */
   circle?: boolean;
+  /**
+   * Render the skeleton using any HTML element.
+   */
+  as?: AsPropType;
 }
 
 /**
  * A placeholder for asynchronously loaded content with a subtle loading
  * animation. Only works when wrapped in a SkeletonContainer.
  */
 export const Skeleton = forwardRef<HTMLSpanElement, SkeletonProps>(
-  ({ children, className, circle, ...props }, ref) => {
+  ({ children, className, as: Element = 'span', circle, ...props }, ref) => {
     const isLoading = useContext(SkeletonContext);
 
     if (isLoading) {
       return (
-        <span
+        <Element
           {...props}
           className={clsx(
             classes.base,
@@ -98,14 +103,14 @@ export const Skeleton = forwardRef<HTMLSpanElement, SkeletonProps>(
           ref={ref}
         >
           {children}
-        </span>
+        </Element>
       );
     }
 
     return (
-      <span {...props} className={clsx(classes.base, className)} ref={ref}>
+      <Element {...props} className={clsx(classes.base, className)} ref={ref}>
         {children}
-      </span>
+      </Element>
     );
   },
 );

diff --git a/packages/circuit-ui/components/TopNavigation/TopNavigation.stories.tsx b/packages/circuit-ui/components/TopNavigation/TopNavigation.stories.tsx
@@ -86,7 +86,7 @@ Base.args = baseArgs;
 export const WithSideNavigation = (args: TopNavigationProps) => {
   const [isSideNavigationOpen, setSideNavigationOpen] = useState(false);
   const hamburger = {
-    ...args.hamburger,
+    ...args.hamburger!,
     isActive: isSideNavigationOpen,
     onClick: () => setSideNavigationOpen((prev) => !prev),
   };
@@ -116,7 +116,6 @@ export const WithSideNavigation = (args: TopNavigationProps) => {
 WithSideNavigation.storyName = 'With SideNavigation';
 WithSideNavigation.args = {
   ...baseArgs,
-  pageTitle: 'Home',
   hamburger: {
     activeLabel: 'Close side navigation',
     inactiveLabel: 'Open side navigation',

diff --git a/packages/circuit-ui/components/TopNavigation/TopNavigation.tsx b/packages/circuit-ui/components/TopNavigation/TopNavigation.tsx
@@ -67,7 +67,7 @@ export function TopNavigation({
   }, []);
 
   return (
-    <header role="banner" className={clsx(classes.base, className)} {...props}>
+    <header className={clsx(classes.base, className)} {...props}>
       <div className={classes.wrapper}>
         {hamburger && (
           <SkeletonContainer isLoading={Boolean(isLoading)}>

diff --git a/packages/circuit-ui/components/TopNavigation/components/ProfileMenu/ProfileMenu.module.css b/packages/circuit-ui/components/TopNavigation/components/ProfileMenu/ProfileMenu.module.css
@@ -9,6 +9,10 @@
   height: var(--cui-icon-sizes-mega);
 }
 
+.details {
+  display: block;
+}
+
 @media (min-width: 768px) {
   .avatar {
     width: var(--cui-icon-sizes-giga);

diff --git a/packages/circuit-ui/components/TopNavigation/components/ProfileMenu/ProfileMenu.tsx b/packages/circuit-ui/components/TopNavigation/components/ProfileMenu/ProfileMenu.tsx
@@ -21,6 +21,7 @@ import Body from '../../../Body/index.js';
 import Popover, { PopoverProps } from '../../../Popover/index.js';
 import { Skeleton } from '../../../Skeleton/index.js';
 import type { UserProps } from '../../types.js';
+import utilityClasses from '../../../../styles/utility.js';
 import sharedClasses from '../../../../styles/shared.js';
 import { clsx } from '../../../../styles/clsx.js';
 
@@ -38,12 +39,21 @@ interface ProfileProps extends ButtonHTMLAttributes<HTMLButtonElement> {
 }
 
 function Profile({ user, label, className, ...props }: ProfileProps) {
+  const ariaLabel = [user.name, user.id]
+    .filter((part) => Boolean(part))
+    .join(', ');
+
   return (
     <button
       {...props}
-      className={clsx(classes.profile, sharedClasses.navigationItem, className)}
+      className={clsx(
+        classes.profile,
+        sharedClasses.navigationItem,
+        utilityClasses.focusVisibleInset,
+        className,
+      )}
       type="button"
-      aria-label={label}
+      aria-label={ariaLabel}
       title={label}
     >
       <Skeleton circle>
@@ -54,21 +64,23 @@ function Profile({ user, label, className, ...props }: ProfileProps) {
             className={classes.avatar}
           />
         ) : (
-          <ProfileIcon role="presentation" />
+          <ProfileIcon aria-hidden="true" />
         )}
       </Skeleton>
-      <div className={classes.details}>
+      <span className={classes.details}>
         <Skeleton className={classes.truncate}>
-          <Body size="two" variant="highlight">
+          <Body as="span" size="two" variant="highlight">
             {user.name}
           </Body>
         </Skeleton>
         {user.id && (
           <Skeleton className={classes.truncate}>
-            <Body size="two">{user.id}</Body>
+            <Body as="span" size="two">
+              {user.id}
+            </Body>
           </Skeleton>
         )}
-      </div>
+      </span>
       <ChevronDown size="16" className={classes.chevron} />
     </button>
   );

diff --git a/packages/circuit-ui/components/TopNavigation/components/UtilityLinks/UtilityLinks.tsx b/packages/circuit-ui/components/TopNavigation/components/UtilityLinks/UtilityLinks.tsx
@@ -21,6 +21,7 @@ import Body from '../../../Body/index.js';
 import { useComponents } from '../../../ComponentsContext/index.js';
 import { Skeleton } from '../../../Skeleton/index.js';
 import { clsx } from '../../../../styles/clsx.js';
+import utilityClasses from '../../../../styles/utility.js';
 import sharedClasses from '../../../../styles/shared.js';
 
 import classes from './UtilityLinks.module.css';
@@ -64,10 +65,15 @@ function UtilityLink({
     <Element
       {...props}
       aria-current={isActive ? 'page' : undefined}
-      className={clsx(classes.anchor, sharedClasses.navigationItem, className)}
+      className={clsx(
+        classes.anchor,
+        sharedClasses.navigationItem,
+        utilityClasses.focusVisibleInset,
+        className,
+      )}
     >
       <Skeleton className={classes.icon}>
-        <Icon role="presentation" size="24" />
+        <Icon aria-hidden="true" size="24" />
       </Skeleton>
       <Skeleton>
         <Body as="span" className={classes.label}>
-Original file line number
+Diff line change
@@ Expand Up / @@ -6,7 +6,7 @@ exports[`Separator > should render with default styles 1`] = ` @@
     }
     <li
-      role="presentation"
+      aria-hidden="true"
     >
       <hr
         class="_base_5bcdd0 circuit-0"
@@ Expand Down @@