Skip to content

Color contrast: have skip links use a black color rather than navy#10583

Merged
srietkerk merged 2 commits intomasterfrom
srietkerk/skip-link-color
May 13, 2025
Merged

Color contrast: have skip links use a black color rather than navy#10583
srietkerk merged 2 commits intomasterfrom
srietkerk/skip-link-color

Conversation

@srietkerk
Copy link
Contributor

@srietkerk srietkerk requested review from a team and Copilot May 13, 2025 17:57
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 PR addresses color contrast by updating skip links to use a black color rather than the previous navy shade. Key changes include:

  • Updating the skip link color in style.css from #387894 to black.
  • Replacing the hex color in accessibility.less with the CSS variable var(--pxt-neutral-foreground1).

Reviewed Changes

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

File Description
theme/accessibility.less Updated accessible menu color to use a CSS variable
docfiles/style.css Explicitly set skip link color to black as required for better contrast
Comments suppressed due to low confidence (1)

theme/accessibility.less:14

  • Ensure that the CSS variable '--pxt-neutral-foreground1' resolves exactly to black for skip links, as the PR title indicates a requirement for black color contrast.
@accessibleMenuColor: var(--pxt-neutral-foreground1);


@accessibleMenuBackground: rgba(255,255,255,.9);
@accessibleMenuColor: #387894;
@accessibleMenuColor: var(--pxt-neutral-foreground1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the background is hard-coded, we should probably hard-code the foreground to black as well. (Otherwise, it'll probably be white on white in dark mode).

In an ideal world, we'd have both be theme colors instead of hard-coded, but I think a white fade makes sense here regardless of theme, so I guess it's fine. Otherwise we'd have to add theme vars for that 90% white color, which I don't think we really have at the moment (kind of the inverse of the neutral-alpha colors)...I wouldn't worry about it for now unless we find ourselves using this white fade in more places.

Copy link
Contributor

@thsparks thsparks May 13, 2025

Choose a reason for hiding this comment

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

To clarify, this is a long-winded way of me saying that this is a rare case where we should probably just hard-code the foreground to black so it's guaranteed to contrast with the hard-coded white fade.

@srietkerk srietkerk merged commit ab510f8 into master May 13, 2025
20 checks passed
@srietkerk srietkerk deleted the srietkerk/skip-link-color branch May 13, 2025 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Skip link text fails accessibility contrast check

3 participants