Overriding theme with nested nimble-theme-provider doesn't work in all cases

[msmithNI]

🐛 Bug Report

In some cases, overriding the theme on a page via a nested nimble-theme-provider does not work.

Acceptance criteria to resolve this issue:

• Remove workarounds added for that theme provider bug in SystemLinkShared. The workaround that needs to be removed is in the Security UI. It got added here.
• Remove the following workaround from nimble: https://github.com/ni/nimble/blob/main/packages/nimble-components/src/anchor-menu-item/index.ts#L107-L122
• Unpin fast-foundation and fast-react-wrapper versions, see: Uptake fast-foundation fixes and remove workaround #1775 (comment)

💻 Repro or Code Sample

In the Nimble Angular example app main page (angular-workspace\projects\example-client-app\src\app\app.component.html), replace the outer nimble-theme-provider with:

<nimble-theme-provider [theme]="themeDark">
    <nimble-theme-provider [theme]="themeLight">
<!-- ... --> 
    </nimble-theme-provider>
</nimble-theme-provider>

with these property declarations in app.component.ts:

public themeLight: Theme = Theme.light;
public themeDark: Theme = Theme.dark;

🤔 Expected Behavior

Example app content renders with the Light theme (as it's the innermost theme provider, and customapp content is rendered inside of that)

😯 Current Behavior

Example app content renders with the Dark theme.

💁 Possible Solution

In nimble-components, we think removing the property initializer for theme on ThemeProvider is part of the fix, but may not fix this issue in all cases.

🔦 Context

The original context is an SLE team running into issues overriding theme in this PR: https://dev.azure.com/ni/DevCentral/_git/Skyline/pullrequest/592543

We've seen different behavior of this bug depending on how the theme is set, and what Angular components set the theme. A few examples:

• In the Nimble Angular example app, have a child/nested theme provider in customapp.component.html and override the theme there instead
• Instead of setting theme like [theme]="themeDark" in a template, set it directly without a binding: theme="dark"

Optimally a fix to this issue will result in the innermost theme taking effect regardless of how the theme is set ([theme] vs theme), and whether the nested theme provider is in the same Angular component or a child component.

🌍 Your Environment

Windows 10 / Chrome browser

[m-akinc]

I think all the various behaviors can be explained by an apparent bug in how FAST's design tokens work. I can reproduce similar buggy behavior in Nimble's storybook. Essentially, the problem is that setting a design token value for an element is a no-op if that value is the same as the inherited value of the token. E.g. if the default value of applicationBackgroundColor (which is defined on the root node) is #ffffff, then you set applicationBackgroundColor to #ffffff on a nimble-theme-provider element, it does nothing. This is a problem, because if you subsequently change the value being inherited, the nimble-theme-provider's applicationBackgroundColor won't stay #ffffff. It will change to whatever the new inherited value is.

So in the scenario with Angular binding of theme, the nested theme provider element sets the theme to "light", which is the default, so it does nothing. Then the parent theme provider's theme gets set to "dark", and that theming cascades to all descendants, because the inner theme provider has no token values set on it.

I looked at the source code and unfortunately couldn't identify any logic that would lead to the observed behavior. I'm fairly confident it's somewhere in there, though.

[rajsite]

This is a problem, because if you subsequently change the value being inherited

Just to be clear are you referring to changing the value with the DesignToken.setValueFor or setting it via mutating css custom properties? Changing values via CSS Custom Properties in devtools / via CSS is not observable by the token system. The fact that we do it in several places is sketchy. All changes to DesignToken values should be via the DesignToken.setValueFor apis.

The theme / direction / lang tokens don't emit css custom properties. They are just programmatically observed.

But that makes me wonder if calling setValueFor in attached and detached of the theme-provider could make a difference.

[m-akinc]

are you referring to changing the value with the DesignToken.setValueFor

Yes, this is using DesignToken.setValueFor.

The theme / direction / lang tokens don't emit css custom properties.

Yes, but changing the theme token causes derived tokens like applicationBackgroundColor to emit css custom properties, and in the case where the value would be the same as the inherited css custom property, it is not emitted.

[rajsite]

Sounds great, wasn't sure since no example branch, etc. was given. Also wasn't mentioned in the reply but hopefully this comment isn't lost as another thing to try:

But that makes me wonder if calling setValueFor in attached and detached of the theme-provider could make a difference.

[m-akinc]

But that makes me wonder if calling setValueFor in attached and detached of the theme-provider could make a difference.

Tested this out by creating and appending a nested theme provider in ngAfterViewInit. From what I can tell, it does not change the behavior. It's as if the css custom properties don't get emitted until the element is connected to the DOM, and at that point, they only get emitted if they have different values than what is being inherited.

[rajsite]

But that makes me wonder if calling setValueFor in attached and detached of the theme-provider could make a difference.

Tested this out by creating and appending a nested theme provider in ngAfterViewInit. From what I can tell, it does not change the behavior. It's as if the css custom properties don't get emitted until the element is connected to the DOM, and at that point, they only get emitted if they have different values than what is being inherited.

Sorry again. So you modified the theme provider custom element and in the attached and detached callbacks used DesignToken.setValueFor?

[m-akinc]

So you modified the theme provider custom element and in the attached and detached callbacks used DesignToken.setValueFor?

No, all I did was programmatically create a theme provider element, and before attaching it to the DOM, I set the theme property to "dark". This should cause theme.setValueFor(this, "dark") to be called. After that, I append it as a child of another theme provider element (that was already in the example app's template). I observed that if the template-defined theme provider had been set to "dark", then the theme provider I appended would not get its own stylesheet with css custom properties. If the template-defined theme provider had been set to "light", then the one I appended would have a stylesheet with css custom properties.

[m-akinc]

By stepping through the FAST code, I think I've found the logic responsible for this. When the parent's value for the token is the same as the new value for the token, it is not "reflected" to the CSS (if it had previously been reflected to the CSS, it stops being reflected). I looked through FAST's issues, and couldn't find anything about exactly this issue. I'm still thinking about the feasibility of fixing this behavior.

[m-akinc]

I've filed a bug to FAST, and I have a potential fix for our legacy branch. I'm not super confident that it's the best fix, but I've noted it in the bug, so we'll see what they say.

[rajsite]

I think we should do more to push this forward. The created issue microsoft/fast#6864
Discusses that a PR would be possible to make. I think we should go further:

• Actually open a PR to the fast legacy branch. Make sure to add test coverage to have confidence in the change.
• See if the issue is reproducible in the main branch.
  • Make a PR if so. Make sure to add test coverage to have confidence in the change.
  • Make a PR for test coverage if not and if lacking.

Creating the PRs for both branches is much more likely to get engagement from FAST than just reporting the issue.

Re-adding the triage tag so it can be proritized.

[m-akinc]

I have created a PR into the fast-element-1 branch. It includes a couple new test cases. There were already fairly extensive existing test cases.

I verified that this is not an issue in the main branch. They significantly reworked the design token code.

[rajsite]

Discussed at stand-up, @m-akinc will create a PR to fast main to include the newly added fast-element-1 tests to main as well

[m-akinc]

PR with new tests into master: microsoft/fast#6883

[m-akinc]

My fix was submitted to FAST, but it exposed a Chromium bug that prevents us from uptaking the FAST release. There is a Discord thread that talks about it and workaround options. I have PRs open to both the master and fast-element-1 branches that simultaneously fix an unrelated, minor issue and workaround the bug.

[m-akinc]

Security UI workaround removed in https://ni.visualstudio.com/DevCentral/_git/Skyline/pullrequest/691733

[jattasNI]

@m-akinc Re-opening this because a Blazor client says they're still seeing it. From a Teams thread:

I made a simple reproducing case - the solution and other files are in a branch here. It looks like it has to do with having multiple NimbleThemeProvider objects. If I only include
this NimbleThemeProvider around the context menu, it correctly gets the Dark theme style. But if I add in this NimbleThemeProvider around the content, now the context menu does not get the Dark theme style, just like I'm seeing with Armstrong

[rajsite]

Investigated the blazor example a bit:

• Was able to run the blazor example and reproduce the behavior. Uses .NET 8 and opened and ran in visual studio 2022.
• Stepping in dev tools I was seeing multiple property updates to the theme-provider's theme property because the property is (likely incorrectly) initialized to light theme. So it ran through all the binding logic twice, once to set a bunch of tokens for light theme and then dark theme. Avoiding the extra initialization didn't improve the Blazor behavior though.
• I tried to re-create the issue on jsbin by taking the resulting html and having the content dynamically generate on the page after 2 seconds but it does not reproduce the incorrect behavior
• I found a workaround: nested theme providers that cause a theme switch do cause the theme to apply correctly:
  

[m-akinc]

Let's characterize this before deciding the priority to fix.

[m-akinc]

I did some investigation (might do some more), but it looks like the context menu elements (including the theme provider) are initially inserted into the DOM under the other, top-level theme provider. When the FAST design token code is responding to setting the theme token value to 'dark', it sees that the theme provider has an ancestor with the same value for that token, so it skips reflecting all the derivative tokens (e.g. --ni-nimble-application-background-color) to CSS (since it expects the element to just inherit the values). Then, soon after, the context menu items get moved in the DOM to children of the <body>, so they no longer inherit the correct token values (they get the default, light theme values from the root element). Seems like the issue is in the FAST code not responding to element reparenting.

[m-akinc]

I have created a PR to FAST to fix this issue.