skeleton icon indicating copy to clipboard operation
skeleton copied to clipboard

AppBar: ARIA role on incompatible element

Open xenogew opened this issue 7 months ago • 3 comments

Current Behavior

AppBar component contains role="toolbar" which is cause reducing Lighthouse score in "Accessibility" category.

Lighthouse Message: "Uses ARIA roles on incompatible elements"

After I drill down with the provided resource from lighthouse, I found https://www.w3.org/TR/html-aria/#docconformance (search with "header") There's no role="toolbar" in allow list. That's why it causes this issue in lighthouse.

Image

Expected Behavior

AppBar should specific role="..." that is allowed and appropriated.

Steps To Reproduce

  1. Using this component. (that's it)

Link to Reproduction / Stackblitz

No response

Environment Information

System: OS: Windows 11 10.0.26100 CPU: (8) x64 AMD Ryzen 3 7330U with Radeon Graphics Memory: 724.86 MB / 5.84 GB Binaries: Node: 22.14.0 - ~\AppData\Roaming\fnm\node-versions\v22.14.0\installation\node.EXE npm: 11.2.0 - ~\AppData\Roaming\fnm\node-versions\v22.14.0\installation\npm.CMD pnpm: 10.0.0 - ~\AppData\Local\pnpm\pnpm.EXE bun: 1.2.9 - ~.bun\bin\bun.EXE Browsers: Edge: Chromium (133.0.3065.59)

More Information

No response

xenogew avatar May 16 '25 01:05 xenogew

And I want to contribute to fix this issue.

xenogew avatar May 16 '25 01:05 xenogew

Hi, I've used Issue To PR to generate a solution for this issue. I've also reviewed #3546 , and this auto-generated plan avoids the problems discussed here.

Does the below plan help? If so, you can auto-generate a PR here for free (requires Github login)


Issue Analysis

Summary:
Both the React and Svelte implementations of the AppBar component assign role="toolbar" to elements that aren't valid toolbar containers per the W3C HTML-ARIA mapping.

Evidence

React (packages/skeleton-react/src/components/AppBar/AppBar.tsx)

  • The root AppBar element is a <div> and explicitly assigns role="toolbar".
  • <div className={`${base} ... ${classes}`} role="toolbar" data-testid="app-bar">
        {children}
    </div>
    

Svelte (packages/skeleton-svelte/src/components/AppBar/AppBar.svelte)

  • The root AppBar element is a <header> and explicitly assigns role="toolbar":
  • <header class="{base} ... {classes}" role="toolbar" data-testid="app-bar">...</header>
    

Accessibility Standard Reference

  • Per W3C HTML-ARIA, role="toolbar" is NOT allowed on <header>.
  • The correct semantic for <header> is normally the implicit banner role, but only in top-level contexts.

Classification

  • Type: Bug (Accessibility)
  • Components Affected:
    • Svelte: AppBar.svelte (<header> wrongly uses role="toolbar")
    • React: AppBar.tsx (while <div role="toolbar"> is technically valid, the question is whether the AppBar as a whole should be a "toolbar" at all—though at least it's not a <header>.)

Implementation Plan

Svelte

  1. Remove role="toolbar" from <header> in AppBar.svelte:
    • Let <header> have its implicit banner landmark.
  2. If an explicit toolbar region is needed, wrap the toolbar section in a <div role="toolbar"> inside the header.
  3. Update tests if they expect role="toolbar" on the outermost element.

React

  1. Option A (Likely Preferred):
    • Remove role="toolbar" from the outer <div> (AppBar root). Use no ARIA role, or use only where semantically needed for toolbars.
  2. Option B:
    • If AppBar is intended to wrap actual interactive controls akin to a toolbar, keep <div role="toolbar">, but update documentation to clarify. (But, do not use on <header>.)

Both

  • Ensure that where a true toolbar is needed (group of controls/actions), it's demarcated with <div role="toolbar"> only, not on container/landmark elements.

Code Changes Outline

Svelte (AppBar.svelte)

  • Before:
    <header ... role="toolbar">
    
  • After:
    <header ...> <!-- no role -->
        <section ... role="toolbar"> <!-- if appropriate, e.g. if this is a true grouping of controls -->
        </section>
    </header>
    

React (AppBar.tsx)

  • Before:
    <div ... role="toolbar">
    
  • After (Option A - recommended):
    <div ...> {/* no role */}
        <div ... role="toolbar"> {/* on section grouping controls, if needed */}
            ...
        </div>
    </div>
    

Edge Cases & Side Effects

  • If any tests assert for a role="toolbar" on the top-level AppBar, these need updating.
  • If consumers of these components relied on keyboard navigation/landmarking via toolbar, confirm their usage remains accessible.

youngchingjui avatar May 19 '25 07:05 youngchingjui

You guys may be making more complicated than it has to be.

  1. If toolbar is not the appropriate designation, then let's remove that
  2. If toolbar is the appropriate designation (which was the original intention) then let's follow the rules for this

Simple as that.

We deemed this a toolbar because it's a top pinned application bar that can OPTIONALLY contain a set of actions in the right-side panel (the trail region). If there's a better way to designate that, then I'm open for it. But at this point it sounds like we should just go ahead and remove the toolbar role.

Submit a PR and I'll review. Otherwise leave this one to me please.

endigo9740 avatar May 19 '25 15:05 endigo9740