Skip to content

Conversation

@srietkerk
Copy link
Contributor

Fixes microsoft/pxt-arcade#7231

Also addresses some other menuitem issues that we ran into in skillmap,

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request improves accessibility by updating link colors to meet WCAG contrast ratio requirements and adding proper ARIA attributes to menuitem components in the skillmap interface.

Key Changes:

  • Updated link color from #3977b4 to #3671ab for improved contrast compliance
  • Added role="menuitem" attributes to Button components in the header menu bar
  • Added aria-hidden="true" to decorative logo elements in the header

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
theme/docs.less Updated documentation link color variable for better contrast
react-common/styles/theming/base-theme.less Updated base theme link color variable for consistency
skillmap/src/components/HeaderBar.tsx Added ARIA attributes to improve accessibility of header navigation elements
Comments suppressed due to low confidence (2)

skillmap/src/components/HeaderBar.tsx:113

  • The parent div has aria-hidden="true", which makes all child elements invisible to screen readers, including the img element with alt text on line 111. This creates conflicting accessibility attributes. Either remove aria-hidden from the parent div to allow screen readers to access the logo, or remove the alt attribute from the img since it won't be announced anyway. If the logo is purely decorative and should be hidden from screen readers, remove the alt attribute. If the logo should be accessible, remove aria-hidden from the parent div.
        return <div className="header-logo" aria-hidden="true">
            {logoUrl
                ? <img src={isLocal() ? `./assets/${logoUrl}`: logoUrl} alt={lf("{0} Logo", targetTheme.organization)}/>
                : <span className="name">{targetTheme.organization}</span>}
        </div>

skillmap/src/components/HeaderBar.tsx:126

  • The parent div has aria-hidden="true", which makes all child elements invisible to screen readers. However, this div contains clickable spans (lines 120-121) with onClick handlers and img elements with alt text (line 123). This creates conflicting accessibility - interactive elements should not be hidden from screen readers. If this logo should be accessible to screen reader users, remove aria-hidden from the parent div. If the interactive elements should be keyboard accessible, they also need to be screen reader accessible.
        return <div className={`ui item logo brand ${!activityOpen ? "noclick" : ""}`} aria-hidden="true">
            {targetTheme.useTextLogo
                ? [<span className="name" key="org-name" onClick={this.onBackClicked}>{targetTheme.organizationText}</span>,
                   <span className="name-short" key="org-name-short" onClick={this.onBackClicked}>{targetTheme.organizationShortText || targetTheme.organizationText}</span>]
                : (targetTheme.logo || targetTheme.portraitLogo
                    ? <img className="logo" src={targetTheme.logo || targetTheme.portraitLogo} alt={lf("{0} Logo", targetTheme.boardName)}/>
                    : <span className="name"> {targetTheme.boardName}</span>)
            }
        </div>

protected getTargetLogo(targetTheme: pxt.AppTheme) {
const { activityOpen } = this.props;
return <div className={`ui item logo brand ${!activityOpen ? "noclick" : ""}`}>
return <div className={`ui item logo brand ${!activityOpen ? "noclick" : ""}`} aria-hidden="true">
Copy link
Member

Choose a reason for hiding this comment

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

i thought aria-hidden on this would be an accessibility issue, as this div wraps an interactive element with onclick? or am i misremembering / perhaps that doesn't cascade down as i would think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't get flagged for it on the accessibility insights fast pass, but it is a good point.

With some quick testing, it looks like the target logo is pretty inaccessible as it is. The span makes it so that you can't interact with the text at all with the Windows Narrator at least. When just using tabbing without a screen reader, you also can't interact with the logo text. I'll look into making this actually accessible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants