-
Notifications
You must be signed in to change notification settings - Fork 518
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
Conversation
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. |
There was a problem hiding this 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?
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. |
FYI I think this was already touched in f32a584#diff-f21bbc22811d892093185d1644b55edd90f715a2fa108e00c07735e8cc6d63fa, although this change does make some contrast improvements. |
There was a problem hiding this 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!
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. :) |
Sure thing, I think we can merge this. Thank you! |
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