[a11y] `Button` and `Link` high contrast outline issue fix #847

stamat · 2024-12-02T19:01:11Z

Summary

Outline of the Button component was always visible in high contrast themes which was causing a11y issues.

Outline of the Link component was invisible in the high contrast themes which is also an a11y issue.

Issue: [a11y] Issue with Button and Link outlines on Windows 11 high contrast themes #846

List of notable changes:

removed styles that were forcing the outline of the Button component to be always visible regardless of the focus in high contrast themes
updated outline of the Link component to use the native outline instead of box-shadow, since box shadow is invisible in high contrast themes
updated underline of the Link component to use border instead of background so the underline will be visible in high contrast themes

What should reviewers focus on?

Outline of the Button component should only be visible when the component is focused in high contrast themes
Outline of the Link component should be visible when the component is focused in high contrast themes
Underline of the Link component should be visible on hover in high contrast themes

Steps to test:

npm run start
Make the storybook port public
Test in high contrast theme enabled Windows 11 by focusing on Button and Link with keyboard

Supporting resources (related issues, external links, etc):

Contributor checklist:

All new and existing CI checks pass
Tests prove that the feature works and covers both happy and unhappy paths
Any drop in coverage, breaking changes or regressions have been documented above
UI Changes contain new visual snapshots (generated by adding update snapshots label to the PR)
All developer debugging and non-functional logging has been removed
Related issues have been referenced in the PR description

Reviewer checklist:

Check that pull request and proposed changes adhere to our contribution guidelines and code of conduct
Check that tests prove the feature works and covers both happy and unhappy paths
Check that there aren't other open Pull Requests for the same update/change

Screenshots:

Please try to provide before and after screenshots or videos

Before	After

…s on high contrast themes

changeset-bot · 2024-12-02T19:01:14Z

⚠️ No Changeset found

Latest commit: c889ba2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

github-actions · 2024-12-02T19:03:00Z

🟢 No design token changes found

github-actions · 2024-12-02T19:11:31Z

⚠️ Visual differences found

Our visual comparison tests found UI differences.

Please review the differences by using the test artifacts to ensure that the changes were intentional.

Artifacts can be downloaded and reviewed locally.

Download links are available at the bottom of the workflow summary screen.

Example:

If the changes are expected, please run npm run test:visual:update-snapshots to replace the previous fixtures.

Review visual differences

…be visible in high contrast themes

joshfarrant · 2024-12-03T08:48:52Z

Looks great @stamat, thank you!

Thanks for raising this, I'd not actually tested with Windows high-contrast themes before. It looks like our icons could do with switching to a different colour in some scenarios. Are you seeing the same thing that I'm seeing here (dark grey arrow icon is barely visible)?

Could you also add the Update snapshots label to this PR to regenerate the snapshots please?

stamat · 2024-12-03T10:21:49Z

@joshfarrant I'll take a deeper look into this man in few moments! I didn't know there was an icon lol...

joshfarrant · 2024-12-03T10:49:19Z

Sorry @stamat, I wasn't suggesting this was something you had to fix 😅

I just wanted to ask out of curiosity as I've not tested with Windows high-contrast themes before and I wanted to make sure I wasn't missing something obvious. I'm happy to take a look through all our components at some point in the future and make sure they're working as well as possible in high-contrast.

rezrah · 2024-12-03T11:21:33Z

packages/react/src/Button/Button.module.css

-  .Button--primary,
-  .Button--secondary,
-  .Button--disabled:hover {
-    outline: var(--brand-borderWidth-thick) solid transparent;
-    outline-offset: 2px;
-  }
-


FYI these styles were put in place by our accessibility engineering team to resolve similar issues in Windows HCM.

Before we undo that work (and for my own understanding), do the changes in this PR take precedence? cc. @TylerJDev who was involved in the original PR, if you have any thoughts too.

@rezrah I completely failed to understand how these button updates that I removed helped accessibility, seemed they had an opposite effect based on my understanding... Yeah it's a good call to ask @TylerJDev to shed some light on this. I'm all 👂 ! 🙏 ❤️

@rezrah I think I know what happened... Our old(er) buttons probably didn't have a border but used an inset box-shadow... https://github.com/primer/brand/blob/d763e2fb6584489f1277370457b4c2ebded2fd82/packages/react/src/Button/Button.module.css (This is the state from the era the change was made)

So this "fix" used the outline to draw the border in high contrast themes since backgrounds and shadows are disabled - instead of just adding the border!?

The reason older buttons had the box-shadow borders was to animate them escaping the jitter of the border, thus sacrificing accessibility for aesthetics.

CSI Miami 😎

Hello 👋

Looking at the change, and my own change, this was a time before borders were used for buttons, so we needed to add something to imitate the styles of box-shadow for HCM.

Since we've added borders to the button, we don't need to imitate the borders anymore, since those always show in HCM, so removing this is okay since it, and the focus styles. Right now it seems like the two are competing against each other, the outline provided in HCM and the border of the button.

As for the outline, this was more or less interchangeable with the border in HCM. I'm not too sure if there was another driving factor here, since it's been a minute 😅, but having there be some sort of presence of a border was the main goal to differentiate between what is/isn't a button.

@TylerJDev thanks for the reply dude! 💖 Yeah I figured it was something like that! ☺️

packages/react/src/Link/Link.module.css

rezrah

Thanks for helping to fix this @stamat 💖 ... I've left you some minor feedback. Would love your thoughts on this in particular.

A few other things before this lands:

We'll need a changeset added to this PR to describe what changed in this PR for our release change log. You can do it by clicking this link on this comment 👇
Updated snapshots would be useful to verify there aren't any visual regressions. You can add the 'update snapshots' label in this PR to run the tests.

Co-authored-by: Rez <[email protected]>

… button and the screen reader will read it as disabled. We need to track where the focus is visually

… chrome btw

stamat · 2024-12-03T12:29:15Z

Also a note, I believe aria-disabled="true" is assumed if the button has disabled attribute: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-disabled

When needing to disable native HTML form controls, developers will need to specify the disabled attribute, as it provides all of the generally expected features of disabling a control by default. However, there can be instances where elements need to be exposed as disabled, but are still available for users to find when navigating via the Tab key. Doing so can improve their discoverability as they will not be removed from the focus order of the web page, as aria-disabled does not change the focusability of such elements, nor will the elements be dimmed by default browser styling, making them easier to read.

I don't think this applies to buttons, for the buttons to be disabled we need to use disabled attr...

stamat · 2024-12-03T12:40:56Z

@joshfarrant @rezrah one final ✅ from you 🙏 ! Also should I up a patch version? 😇

stamat added 2 commits December 2, 2024 19:00

Link component should use native outline - boxShadow cause a11y issue…

35e5b80

…s on high contrast themes

Button should have an outline in high contrast themes

de44198

stamat temporarily deployed to github-pages December 2, 2024 19:08 — with GitHub Actions Inactive

use border instead of background for underline so the underline will …

f60f5a3

…be visible in high contrast themes

stamat temporarily deployed to github-pages December 2, 2024 19:50 — with GitHub Actions Inactive

stamat self-assigned this Dec 2, 2024

stamat requested a review from rezrah December 2, 2024 20:17

stamat mentioned this pull request Dec 2, 2024

[a11y] Issue with Button and Link outlines on Windows 11 high contrast themes #846

Open

stamat changed the title ~~Stamat/high-contrast-outline-issues~~ [a11y] Button and Link high contrast outline issue fix Dec 2, 2024

stamat marked this pull request as ready for review December 2, 2024 21:02

joshfarrant approved these changes Dec 3, 2024

View reviewed changes

rezrah reviewed Dec 3, 2024

View reviewed changes

packages/react/src/Link/Link.module.css Outdated Show resolved Hide resolved

rezrah reviewed Dec 3, 2024

View reviewed changes

packages/react/src/Link/Link.module.css Outdated Show resolved Hide resolved

rezrah reviewed Dec 3, 2024

View reviewed changes

stamat and others added 2 commits December 3, 2024 12:38

Update packages/react/src/Link/Link.module.css

56938e2

Co-authored-by: Rez <[email protected]>

Update packages/react/src/Link/Link.module.css

33faa7c

Co-authored-by: Rez <[email protected]>

stamat temporarily deployed to github-pages December 3, 2024 11:48 — with GitHub Actions Inactive

stamat added 3 commits December 3, 2024 12:14

this was causing the icon to be hidden in high contrast themes

6c2e2fb

outline needs to be visible since the focus will land on the disabled…

35fbbff

… button and the screen reader will read it as disabled. We need to track where the focus is visually

make outline consistent, disabled natively doesn't display outline on…

cc63b94

… chrome btw

stamat added the update snapshots label Dec 3, 2024

stamat deployed to github-pages December 3, 2024 12:33 — with GitHub Actions View deployment

github-actions[bot] Regenerated snapshots

c889ba2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[a11y] `Button` and `Link` high contrast outline issue fix #847

[a11y] `Button` and `Link` high contrast outline issue fix #847

stamat commented Dec 2, 2024 •

edited

Loading

changeset-bot bot commented Dec 2, 2024 •

edited

Loading

github-actions bot commented Dec 2, 2024 •

edited

Loading

github-actions bot commented Dec 2, 2024 •

edited

Loading

Example:

joshfarrant commented Dec 3, 2024

stamat commented Dec 3, 2024

joshfarrant commented Dec 3, 2024

rezrah Dec 3, 2024

stamat Dec 3, 2024

stamat Dec 3, 2024 •

edited

Loading

TylerJDev Dec 3, 2024

stamat Dec 3, 2024

rezrah left a comment

stamat commented Dec 3, 2024

stamat commented Dec 3, 2024

[a11y] Button and Link high contrast outline issue fix #847

Are you sure you want to change the base?

[a11y] Button and Link high contrast outline issue fix #847

Conversation

stamat commented Dec 2, 2024 • edited Loading

Summary

List of notable changes:

What should reviewers focus on?

Steps to test:

Supporting resources (related issues, external links, etc):

Contributor checklist:

Reviewer checklist:

Screenshots:

changeset-bot bot commented Dec 2, 2024 • edited Loading

⚠️ No Changeset found

github-actions bot commented Dec 2, 2024 • edited Loading

🟢 No design token changes found

github-actions bot commented Dec 2, 2024 • edited Loading

⚠️ Visual differences found

Example:

joshfarrant commented Dec 3, 2024

stamat commented Dec 3, 2024

joshfarrant commented Dec 3, 2024

rezrah Dec 3, 2024

Choose a reason for hiding this comment

stamat Dec 3, 2024

Choose a reason for hiding this comment

stamat Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

TylerJDev Dec 3, 2024

Choose a reason for hiding this comment

stamat Dec 3, 2024

Choose a reason for hiding this comment

rezrah left a comment

Choose a reason for hiding this comment

stamat commented Dec 3, 2024

stamat commented Dec 3, 2024

[a11y] `Button` and `Link` high contrast outline issue fix #847

[a11y] `Button` and `Link` high contrast outline issue fix #847

stamat commented Dec 2, 2024 •

edited

Loading

changeset-bot bot commented Dec 2, 2024 •

edited

Loading

github-actions bot commented Dec 2, 2024 •

edited

Loading

github-actions bot commented Dec 2, 2024 •

edited

Loading

stamat Dec 3, 2024 •

edited

Loading