[Bug]: useButtonStyles_unstable produces duplicate classnames when passing in referentially equal state object

[ashwinGokhale]

Library

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

System Info

System:
    OS: Windows 11 10.0.22631
    CPU: (20) x64 13th Gen Intel(R) Core(TM) i7-1370P
    Memory: 7.16 GB / 31.66 GB
  Browsers:
    Edge: Chromium (121.0.2277.112)
    Internet Explorer: 11.0.22621.1

Are you reporting Accessibility issue?

no

Reproduction

https://codesandbox.io/p/sandbox/fluent9-button-style-bug-d5nd2l

Bug Description

Actual Behavior

Getting the following error when using a referentially stable object for button state in the useButtonStyles_unstable hook:

mergeClasses(): a passed string contains multiple classes produced by makeResetStyles (fui-Button r1alrhcs & fui-Button r1alrhcs , this will lead to non-deterministic behavior. Learn more:https://griffel.js.org/react/api/make-reset-styles#limitationsSource string: fui-Button r1alrhcs     at MyButton (https://d5nd2l.csb.app/src/App.tsx:31:70)    at div    at TextDirectionProvider (https://d5nd2l.csb.app/node_modules/@griffel/react/TextDirectionContext.esm.js:23:33)    at eval (https://d5nd2l.csb.app/node_modules/@fluentui/react-provider/lib-commonjs/components/FluentProvider/FluentProvider.js:18:69)    at App (https://d5nd2l.csb.app/src/App.tsx:43:53)

Expected Behavior

The output of useButtonStyles_unstable should be the exact same object every time since the button state param does not change.

Logs

No response

Requested priority

Low

Products/sites affected

No response

Are you willing to submit a PR to fix?

no

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.

[smhigley]

@ashwinGokhale the codesandbox link doesn't seem to be working for me, could you double check that the sandbox is public and that the link is correct?

The text of the error appears to be about multiple uses of a makeResetStyles className on the same element rather than changes between renders. Since styles generated by makeResetStyles are not merged, applying multiple classnames from makeResetStyles calls can be unpredictable (i.e. because conflicting styles in the reset styles block depend on the order of the classNames in the DOM).

The useButtonStyles hook does have two classes generated by makeResetStyles, one for the root and one for the icon slot. I can't really be sure why that error is showing up without being able to see the example, though.

[ashwinGokhale]

@smhigley can you check the codesandbox link again?

[khmakoto]

@ashwinGokhale I think the main reason why this is happening is because you are declaring your state as a const in your file, whereas the hook assumes that the state is being created by the component itself. You can see how this doesn't happen if you move the state initialization within your MyButton component:

https://codesandbox.io/p/sandbox/fluent9-button-style-bug-forked-nyvll4?file=%2Fsrc%2FApp.tsx%3A22%2C1

Basically, because the buttonState is never being recreated in your code, you're consistently appending the same set of class names to the class list.

@layershifter do you think there would be a value in updating mergeClassNames so that it skips not only duplicate atomic styles, but also duplicate class names so this error is not encountered?

[ashwinGokhale]

Another weird behavior is if you make the appearance: 'primary' then it works fine. This is very weird behavior and as a user it is not very intuitive.

[smhigley]

@ashwinGokhale thanks for updating the codepen! As @khmakoto mentioned, the reason this is happening is because on each render you are re-generating the styles using useButtonStyles while passing in the className generated from the previous output of useButtonStyles. This causes the classNames to stack, which is why on each render the className string keeps getting longer with multiple repeated classes.

The reason your console.log equality check is returning true is because useButtonStyles is modifying the root.className prop in-place and not re-creating the entire object. If you add this line, it will return false: console.log("className is equal", buttonStyles.root.className === buttonRef.current.className);

The reason this is only happening with appearance="secondary" is because the secondary appearance uses only the base styles, and is not adding any additional atomic styles. While strictly this error should also be thrown for all other appearances, it's not doing so because of this check in griffel, coupled with (I believe) griffel not really expecting additional reset styles to be processed after makeStyles styles: https://github.com/microsoft/griffel/blob/main/packages/core/src/mergeClasses.ts#L56

The best immediate solution to this would be to ensure you're not passing the result of the previous useButtonStyles call into the current one. @khmakoto's codesandbox shows the recommended way to do so (by moving state initialization into your component), though you could theoretically also fix it by just resetting state.root.className and state.icon.className (in case you ever have an icon) before passing the state back into the style hook.

[ashwinGokhale]

The classnames are computed every time the hook is called. Would it be possible to wrap the mergeClasses calls in a useMemo? Something like this? https://github.com/microsoft/fluentui/pull/30640

[spmonahan]

The classnames are computed every time the hook is called. Would it be possible to wrap the mergeClasses calls in a useMemo? Something like this? #30640

Do you have any data that suggests this is necessary?

[ashwinGokhale]

@spmonahan No that's why I'm asking.

[spmonahan]

I don't think it's necessary and may actually regress performance. useMemo() isn't free and in your example PR the memoization depends on so many values it's likely to be recomputed on many (maybe most) calls to useButtonStyles_unstable() so now in addition to computing the classes we also have to do the work to evaluate and update the memo.

[ashwinGokhale]

Then should this be handled by Griffel?

[microsoft-github-policy-service[bot]]

no assignees

Gentle ping that this issue needs attention.

[khmakoto]

@ashwinGokhale this is only happening because of your current set up as I showed with my codesandbox above. Currently, there is no reason to check for something like this in Griffel or to use useMemo, since there might be a regression in performance. If you really want to make this change, I'd suggest getting some data on scenarios where this is absolutely necessary and there's not another, more performant way of doing it.

[microsoft-github-policy-service[bot]]

This issue has been automatically marked as stale because it has marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment. Thank you for your contributions to Fluent UI!