fluentui icon indicating copy to clipboard operation
fluentui copied to clipboard

[Bug]: Token assignment in v9ThemeShim makes Neutral Avatar fail contrast check

Open mamooty opened this issue 1 year ago • 8 comments

Library

React Components / v9 (@fluentui/react-components)

System Info

Colors assigned to --colorNeutralBackground6 and --colorNeutralForeground3 should contrast 4.5/1 for use in Avatar, but v9 shim has them using neutralLight and neutralTertiary, which do not contract 4.5/1 in the default v8 theme.

image

image

Are you reporting Accessibility issue?

no

Reproduction

https://codesandbox.io/p/sandbox/busy-golick-pyrdcg?file=%2Findex.tsx%3A14%2C17&layout=%257B%2522sidebarPanel%2522%253A%2522EXPLORER%2522%252C%2522rootPanelGroup%2522%253A%257B%2522direction%2522%253A%2522horizontal%2522%252C%2522contentType%2522%253A%2522UNKNOWN%2522%252C%2522type%2522%253A%2522PANEL_GROUP%2522%252C%2522id%2522%253A%2522ROOT_LAYOUT%2522%252C%2522panels%2522%253A%255B%257B%2522type%2522%253A%2522PANEL_GROUP%2522%252C%2522contentType%2522%253A%2522UNKNOWN%2522%252C%2522direction%2522%253A%2522vertical%2522%252C%2522id%2522%253A%2522cltyvunw700073j6gbxfug41p%2522%252C%2522sizes%2522%253A%255B100%252C0%255D%252C%2522panels%2522%253A%255B%257B%2522type%2522%253A%2522PANEL_GROUP%2522%252C%2522contentType%2522%253A%2522EDITOR%2522%252C%2522direction%2522%253A%2522horizontal%2522%252C%2522id%2522%253A%2522EDITOR%2522%252C%2522panels%2522%253A%255B%257B%2522type%2522%253A%2522PANEL%2522%252C%2522contentType%2522%253A%2522EDITOR%2522%252C%2522id%2522%253A%2522cltyvunw700033j6gnc92mi0h%2522%257D%255D%257D%252C%257B%2522type%2522%253A%2522PANEL_GROUP%2522%252C%2522contentType%2522%253A%2522SHELLS%2522%252C%2522direction%2522%253A%2522horizontal%2522%252C%2522id%2522%253A%2522SHELLS%2522%252C%2522panels%2522%253A%255B%257B%2522type%2522%253A%2522PANEL%2522%252C%2522contentType%2522%253A%2522SHELLS%2522%252C%2522id%2522%253A%2522cltyvunw700043j6gzwxw7ksi%2522%257D%255D%252C%2522sizes%2522%253A%255B100%255D%257D%255D%257D%252C%257B%2522type%2522%253A%2522PANEL_GROUP%2522%252C%2522contentType%2522%253A%2522DEVTOOLS%2522%252C%2522direction%2522%253A%2522vertical%2522%252C%2522id%2522%253A%2522DEVTOOLS%2522%252C%2522panels%2522%253A%255B%257B%2522type%2522%253A%2522PANEL%2522%252C%2522contentType%2522%253A%2522DEVTOOLS%2522%252C%2522id%2522%253A%2522cltyvunw700063j6g5bsmzteq%2522%257D%255D%252C%2522sizes%2522%253A%255B100%255D%257D%255D%252C%2522sizes%2522%253A%255B50%252C50%255D%257D%252C%2522tabbedPanels%2522%253A%257B%2522cltyvunw700033j6gnc92mi0h%2522%253A%257B%2522tabs%2522%253A%255B%257B%2522id%2522%253A%2522cltyvunw700023j6g0n7crz83%2522%252C%2522mode%2522%253A%2522permanent%2522%252C%2522type%2522%253A%2522FILE%2522%252C%2522initialSelections%2522%253A%255B%257B%2522startLineNumber%2522%253A14%252C%2522startColumn%2522%253A17%252C%2522endLineNumber%2522%253A14%252C%2522endColumn%2522%253A17%257D%255D%252C%2522filepath%2522%253A%2522%252Findex.tsx%2522%257D%255D%252C%2522id%2522%253A%2522cltyvunw700033j6gnc92mi0h%2522%252C%2522activeTabId%2522%253A%2522cltyvunw700023j6g0n7crz83%2522%257D%252C%2522cltyvunw700063j6g5bsmzteq%2522%253A%257B%2522id%2522%253A%2522cltyvunw700063j6g5bsmzteq%2522%252C%2522tabs%2522%253A%255B%257B%2522id%2522%253A%2522cltyvunw700053j6gen5fvn9x%2522%252C%2522mode%2522%253A%2522permanent%2522%252C%2522type%2522%253A%2522UNASSIGNED_PORT%2522%252C%2522port%2522%253A0%252C%2522path%2522%253A%2522%252F%2522%257D%255D%252C%2522activeTabId%2522%253A%2522cltyvunw700053j6gen5fvn9x%2522%257D%252C%2522cltyvunw700043j6gzwxw7ksi%2522%253A%257B%2522tabs%2522%253A%255B%255D%252C%2522id%2522%253A%2522cltyvunw700043j6gzwxw7ksi%2522%257D%257D%252C%2522showDevtools%2522%253Atrue%252C%2522showShells%2522%253Afalse%252C%2522showSidebar%2522%253Atrue%252C%2522sidebarPanelSize%2522%253A15%257D

Bug Description

  1. Use an OOB SharePoint Theme from Fluent v8
  2. use createV9Theme from the @fluentui/react-migration-v8-v9 package.
  3. use the product of that function for a Fluent v9 theme

Actual Behavior

Avatars do not have enough contrast between foreground and background Placeholder text and section text within Dropdown controls is too light of a gray Badges do not have enough contrast and appear disabled

Expected Behavior

Avatars, Dropdown placeholder text & section text, and Badges should all have sufficient contrast

Avatar uses colorNeutralForeground3 when used with "neutral" color, as seen in the repro. Placeholder text uses colorNeutralForeground4 in various components including the Dropdown, which I've also included in the repro. Dropdown OptionGroup labels use colorNeutralForeground3, also visible in the repro. Badges with color="informative" use colorNeutralForeground3 as well, and are also visible in the repro in various appearances.

Looking at the examples, it appears that neutralQuaternary is used for colorNeutralForeground4, but it is too far toward the light end of the neutral palette to be appropriate for this usage. Same problem with colorNeutralForeground3 being assigned neutralTertiary. Recommend shifting the colorNeutralForeground colors so that they increase as you go from colorNeutralForeground1 to 4, but so that colorNeutralForeground4 is at neutralSecondary or neutralSecondaryAlt.

Logs

No response

Requested priority

High

Products/sites affected

No response

Are you willing to submit a PR to fix?

yes

Validations

  • [X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
  • [X] The provided reproduction is a minimal reproducible example of the bug.

mamooty avatar Mar 20 '24 19:03 mamooty

Verified, and updated

micahgodbolt avatar Mar 20 '24 21:03 micahgodbolt

@mamooty note that the returned v9 theme is just a large object of key value pairs. You should be able to swap in the colors you want for those tokens without any issue.

micahgodbolt avatar Mar 20 '24 21:03 micahgodbolt

@micahgodbolt I can do that as a workaround if needed, but my request is for this to be changed within Fluent. It appears to be an incorrect behavior. Further, I am hoping to avoid adding overriding mappings on top of Fluent where possible, as that can cause sustainability problems long-term.

mamooty avatar Mar 22 '24 16:03 mamooty

cc @GeoffCoxMSFT @mltejera

tudorpopams avatar Mar 25 '24 16:03 tudorpopams

This is solid feedback - thanks @mamooty! I think we're going to keep the status as low for now. If you're able to contribute a non-breaking and non-disruptive fix, we'd gladly take it. In the meantime, we're going to focus cycles on the next token system for Fluent.

mltejera avatar Apr 01 '24 20:04 mltejera

Resuming discussion around this since we still need to resolve it.

@mltejera / @GeoffCoxMSFT I hear your concern about not wanting to take a breaking/disruptive fix. We can reassign the tokens ourselves if we need to, but I think Fluent's behavior here is incorrect, and any new user of this function has a reasonable chance of hitting this issue.

How can we handle any breaking changes in this function? I think there is a generic problem in that regard. Is there a pattern for including a version parameter when calling the function, so that we can avoid breaking changes while still updating the algorithm for correctness?

mamooty avatar Jun 10 '24 22:06 mamooty

Looking into this - the components listed above do not implement the neutrals correctly. That is NeutralForeground3 should not be applied on top of NeutralBackground6.

Will continue looking into whether the shims output is correct on the neutrals.

mltejera avatar Jun 24 '24 16:06 mltejera

Hey @mamooty - after more consideration, this will need to be fixed on the consumption end for the time being.

The functions being called for this bug are created under the assumption that neutrals wouldn't be changed by down stream consumers. We simply don't have the necessary tooling in place to confidently test themes.

We are working on it though, and hope to have the infrastructure in place by September to make changing these things faster and easier.

mltejera avatar Jun 24 '24 21:06 mltejera

This issue has not had activity for over 180 days! We're adding Soft close label and will close it soon for house-keeping purposes. Still require assistance? Please add comment - "keep open".

Because this reported issue has not had any activity for over 180 days, we're automatically closing it for house-keeping reasons.

Still require assistance? Please, create a new issue with up-to date details and latest version of Fluent.