Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Improve contrast of SVG element #2771

Merged
merged 3 commits into from
Aug 2, 2024
Merged

fix: Improve contrast of SVG element #2771

merged 3 commits into from
Aug 2, 2024

Conversation

onno-vdbroek
Copy link
Contributor

@onno-vdbroek onno-vdbroek commented May 3, 2024

Description

Improve contrast of the SVG element's fill to the SVG element's border and page background for issue #2054

Motivation

The SVG element's fill didn't comply with the WCAG contrast criteria. By improving the contrast it now complies with the WCAG AA criteria for Graphical Objects and User Interface Components.

Additional details

Related issues and pull requests

@onno-vdbroek onno-vdbroek requested a review from a team as a code owner May 3, 2024 14:08
@onno-vdbroek onno-vdbroek requested review from chrisdavidmills and removed request for a team May 3, 2024 14:08
Copy link

github-actions bot commented May 3, 2024

It looks like this is your first pull request. 🎉 Thank you for your contribution! One of the project maintainers will triage and assign the pull request for review. We appreciate your patience. To safeguard the health of the project, please take a moment to read our code of conduct.

Copy link
Contributor

@chrisdavidmills chrisdavidmills left a comment

Choose a reason for hiding this comment

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

Hi there @onno-vdbroek!

I've had a look at this, and I don't really get the point of this change.

To clarify — this is the example at the top of https://developer.mozilla.org/en-US/docs/Web/CSS/pointer-events, correct?

When I test the original background fill versus the text color on https://webaim.org/resources/contrastchecker/, it passes the tests fine. When I test your modified background fill, it fails WGAC AAA for Normal Text.

Am I missing something here?

@onno-vdbroek
Copy link
Contributor Author

Hello @chrisdavidmills!

Thank you for reviewing my pull request.

The example we're talking about is indeed https://developer.mozilla.org/en-US/docs/Web/CSS/pointer-events.

As mentioned in a comment on this issue (#2054 (comment)) the example doesn't meet proper contrast guidelines in dark theme. My modification is about the background fill of the SVG element versus the dark theme page background (hex color #1b1b1b), not the text.

I found that the original background fill of the SVG element versus the dark theme page background didn't pass any WGAC tests. To comply with the contrast criteria I changed the SVG background fill to a lighter color so it passed the WCAG AA test for Graphical Objects and User Interface Components.

You're right that my modified background fill fails with the WGAC AAA for Normal Text. But it still complies with the WGAC AA for Normal Text.

I think by changing the SVG background fill color we now have the best possible outcome: it complies with the WGAC AA for Normal Text (by testing the SVG background fill versus the text) and it passes the WCAG AA test for Graphical Objects and User Interface Components. (by testing the SVG background fill versus the dark theme page background)

I hope I clarified the reason for my pull request. It was a bit tricky to find the right color that passes the most amount of tests (SVG background fill versus page background/versus SVG stroke/versus text).

This is my first pull request ever so I'm thankful for your help. If anything is still unclear I'm happy to try to explain it further.

@github-actions github-actions bot added the idle Issues and pull requests with no activity for three months. label Jun 8, 2024
@bsmth
Copy link
Member

bsmth commented Jul 18, 2024

FYI I think this was already touched in f32a584#diff-f21bbc22811d892093185d1644b55edd90f715a2fa108e00c07735e8cc6d63fa, although this change does make some contrast improvements.

@bsmth bsmth requested a review from chrisdavidmills July 18, 2024 11:09
@bsmth bsmth changed the title Improve contrast of SVG element fix: Improve contrast of SVG element Jul 18, 2024
Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

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

I would leave a +1, curious what @chrisdavidmills thinks!

@onno-vdbroek
Copy link
Contributor Author

Thanks for reviewing my change. Unfortunately, I didn't know this issue was already touched because when I made the change, the checkbox for this issue/fix wasn't checked. Although I hope my contribution was helpful. :)

@github-actions github-actions bot removed the idle Issues and pull requests with no activity for three months. label Jul 19, 2024
@bsmth
Copy link
Member

bsmth commented Aug 2, 2024

Sure thing, I think we can merge this. Thank you!

@bsmth bsmth merged commit 35365c1 into mdn:main Aug 2, 2024
5 checks passed
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.

3 participants