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

🐛 [BUG] Incorrect value for --treeViewItem-leadingVisual-iconColor-rest in dark high contrast mode #1083

Open
ktravers opened this issue Nov 16, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@ktravers
Copy link

ktravers commented Nov 16, 2024

Describe the bug

TL;DR

In dark high contrast mode, the current value of --treeViewItem-leadingVisual-bgColor-rest (#b7bdc8) is incorrect. We need to update the value to the correct value, #f0f3f6.

Full story

While working on primer/react#5267, I ran into unexpected visual regression test failures. Specifically, the visual regression tests were failing in dark high contrast mode only because the expected value for the TreeView.Item LeadingVisual icon fill was different than the actual value.

Prior to primer/react#5267, the icon fill was set by custom theming within primer/react that referenced a deprecated variable and a hard-coded fallback value:

"treeViewItem": {
  "directory": {
    "fill": "var(--treeViewItem-leadingVisual-bgColor-rest, var(--color-tree-view-item-directory-fill, #f0f3f6))"
  }
},

The variable --treeViewItem-leadingVisual-bgColor-rest was replaced by --treeViewItem-leadingVisual-iconColor-rest in #686. When that replacement was made, we didn't update the theme in primer/react. As a result, when we calculate the fill value, it always uses the fallback, because the given variables no longer exist. This fallback is technically the "correct" color for dark high contrast mode.

Currently, --treeViewItem-leadingVisual-bgColor-rest is using a different value in dark high contrast mode, #b7bdc8. This value is incorrect. It should be using #f0f3f6 instead. We updated the primer/react snapshots as a temporary workaround, but now that primer/react#5267 has shipped, we should update the variable to the correct value.

Reproduction steps

Review vrt test report: vrt-all-flags.zip

Expected behavior

TreeView.Item leading visual icon fill is #f0f3f6 in dark high contrast mode.

Screenshots

Correct directory icon fill Current incorrect directory icon fill
Screenshot of tree view with correct fill Screenshot of tree view with incorrect fill

References

/cc @langermank

@ktravers ktravers added the bug Something isn't working label Nov 16, 2024
@lukasoppermann
Copy link
Contributor

Hey @ktravers,
can you explain why you feel that #f0f3f6 is the correct value?

It is a custom value not available in our tokens. Whereas #b7bdc8 is neutral-10 so part of our scale.

The contrast is >10 so should be fine as well.

Image

@langermank
Copy link
Contributor

langermank commented Nov 18, 2024

@lukasoppermann I think its just meant to be white in dark high contrast. The exact hex value @ktravers provided is probably from the old neutral scale because we're basing this off of an older VRT snapshot image. So basically, we should just increase the contrast a bit on this token so it matches up better with the previous experience.

@lukasoppermann
Copy link
Contributor

@langermank why do we need higher contrast? We could use a higher step like 11 or 12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants