The "eye" icon (via navigation.icons.yml) SVG appears to be using a hard coded color (#55565B) instead of currentColor. Looks like the same issue as #3621244: Navigation mobile sidebar close button in dark mode has low color contrast but with a different icon.
Screenshots
Review
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | eye-after-light.png | 9.67 KB | abenbow |
| #7 | eye-after-unselected.png | 8.02 KB | abenbow |
| #7 | eye-after.png | 8.26 KB | abenbow |
| #7 | eye-before-light-theme.png | 7.9 KB | abenbow |
| #7 | eye-before-unselected.png | 6.6 KB | abenbow |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #4
abenbowI've opened up a new MR that resolves this request by making the fill color for the eye.svg file be "currentColor" instead of the hardcoded hex value.
It now inherits whatever the text color is for the given element.
To the issue opener's point, I do think there are a few other icons like this one in the stack that could stand a little SVG fixup in order to get the same effect.
Comment #5
abenbowI've added a link to a new issue I've opened that tries to reconcile the final hardcoded colors:
https://www.drupal.org/project/drupal/issues/3624877
Comment #6
quietone commentedHi, Issues for Drupal core should be targeted to the 'main' branch, our primary development branch. Changes are made on the main branch first, and are then back ported as needed according to the Core change policies. The version the problem was discovered on should be stated in the issue summary Problem/Motivation section. Thanks.
The MR here is on main so no need for a change there.
I am restoring standard issue template so we can track progress on this issue.
Changes to the user interface should have before and after screenshots, so tagging for that. The icon is not used in core but would be best to proof there are no typos or small errors introduced.
Comment #7
abenbow@quietone, thanks for the heads up about the screenshots, and for providing some further context around this issue.
Above, I've uploaded some screenshots I've put together demonstrating the differences that this commit introduces.
Namely, you'll notice that the light mode screenshots are virtually unaffected (I assume the original icon color was only tested in light mode), but the dark mode screenshots are where you can see a real difference.
I couldn't find any part of the navigation module that directly implements this eye icon, so my implementation for testing is custom. However, I'd assume that this is immediately noticeable for users who try to extend the navigation module for their own ends, which isn't entirely unimaginable given that the assets and the API are there and encourage that kind of work.
Comment #8
ironnuts commentedI have reviewed this issue. The code swaps a hardcoded hex value for the path fill attribute of the eye.svg with a CSS variable. No problem identified with the code at all.
There is an obvious improvement in the contrast and clarity of the icon in eye-after-unselected.svg.png compared to eye-before-unselected.svg.png.
There seems no difference between eye-before-light.svg.png and eye-after-light.svg.png.
However there is a clear improvement in contrast and clarity between eye-before.png (bad) and eye-after.png (good).
Checking back my instinctive reactions with the IS, this seems to be exactly what we need to resolve this issue. Moving to RTBTC and removing the tag Needs screenshots because it does not need any.
Comment #9
ironnuts commented