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

ktravers commented 3 days ago

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 https://github.com/primer/react/pull/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 https://github.com/primer/react/pull/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 https://github.com/primer/primitives/pull/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 https://github.com/primer/react/pull/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

References

/cc @langermank

lukasoppermann commented 1 day ago

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.

langermank commented 22 hours ago

@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 commented 21 hours ago

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

primer / primitives