Theming on iTwinUI Radio and Alert

morganadrales commented 2 years ago

Describe the bug (actual behavior)

Toggling between light and dark mode in Design Review does not apply the correct styling to the itwinui-react radio button labels and the alert after the initial change. For instance, if I start in dark mode and toggle to light mode, then the correct styling is applied. But if I then toggle from light mode to dark mode, the light mode styling is kept. The same issue occurs for the alert where the background transparency/color is not switched to dark mode when toggling from light mode to dark. For reference, we were using the radio buttons and alert from @bentley/ui-core and the theming worked but we are wanting to move to using iTwinUI components.

Expected behavior

After toggling to light mode, the second toggle back to dark mode should have the dark mode theming applied to the button labels and the alert (i.e. the text should be the white-ish color and the alert background should be slightly transparent).

Reproduction

Link to a minimal repro:

https://user-images.githubusercontent.com/85196633/140411888-346d79a6-2f58-4be9-9709-06379dc7f796.mp4

Steps to reproduce

This is on a local branch of Design Review so reproduction would require pulling this branch since this is not in QA.

Additional information

Packages: @itwin/itwinui-react: 1.22.0 @bentley/imodeljs: 2.19.14

Questions: Are you using iframes anywhere?

No, not on the settings page

What classes does the html have if you inspect the DOM?

The specific class that affects the radio labels is label.iui-radio which has the following properties:

.iui-radio {
margin: 0;
padding: 0;
border: none;
vertical-align: baseline;
display: flex;
align-items: center;
font-size: 14px;
width: -webkit-fit-content;
width: -moz-fit-content;
width: fit-content;
-webkit-user-select: none;
-moz-user-select: none;
user-select: none;
position: relative;
cursor: pointer;
/* color: rgba(0, 0, 0, 0.8); */
color: rgba(var(--iui-color-foreground-body-rgb), var(--iui-opacity-2));
}

For additional reference, those variables are: --iui-color-foreground-body-rgb: 0, 0, 0; --iui-opacity-2: 0.8;

Are you able to reproduce this issue on a blank page?

If I have just a radio button wrapped in a ThemeProvider and explicitly set the theme to "dark" or "light", it applies the styling correctly. If I explicitly set the theme to "dark" or "light" mode (without having the toggling logic so the value is static) then the styling is also applied correctly.

Are there any other theming solutions that are conflicting with itwinui?

Possibly. We are getting the theme in one of two ways:
1. For the overall app theming, we get it from the app state's display theme which comes from a graphQL user settings query.
2. For the settings page components, I tried wrapping them in a ThemeProvider with theme from UiFramework.getColorTheme() which does seem to return the correct value when you toggle. Like I mentioned earlier, it does apply the theming correctly the first time, but toggling back to the dark mode does not work.

mayank99 commented 2 years ago

Hi @morganadrales 👋 Thanks for creating the issue.

Let me clarify this part:

What classes does the html have if you inspect the DOM?

The specific class that affects the radio labels is label.iui-radio which has the following properties: ... For additional reference, those variables are: --iui-color-foreground-body-rgb: 0, 0, 0;

By "html", I meant the html element of the document. iTwinUI applies iui-theme-light or iui-theme-dark class on that element. And based on that value, the css variables change. So in this case, the value of --iui-color-foreground-body-rgb should be 0, 0, 0 in light theme and 255, 255, 255 in dark theme.

I tried wrapping them in a ThemeProvider with theme from UiFramework.getColorTheme() which does seem to return the correct value when you toggle

For reference, we were using the radio buttons and alert from @bentley/ui-core and the theming worked

Can you verify that the value being passed into the theme prop is always correct at every stage (i.e. on mount, on page change, on toggle, etc)? I see there is a default value called SYSTEM_PREFERRED in UiFramework.ThemeManager which might be causing issues.

If I explicitly set the theme to "dark" or "light" mode (without having the toggling logic so the value is static) then the styling is also applied correctly.

Is this on a blank page or the same settings page? If it is on the settings page, I would like to see the toggling logic. If it is on a blank page (in the same app), does adding toggling break it?

Also, could you search for itwinui-css and itwinui-react in your lockfile and see if there are any older/conflicting versions coming from some other dependencies? Thanks.

morganadrales commented 2 years ago

Hi Mayank, thanks for your quick reply!

On further investigation, the Civil team had already brought to our attention that we were not using iui-theme-light and iui-theme-dark in the root html element of our app and it was causing theming issues for them as well.

Adding that class fixes the issue and everything looks great! Thanks for your help!

I'll close this bug since it's not actually a bug with itwinui-react.

veekeys commented 2 years ago

@mayank99 isnt our ThemeProvider supposed to add this class to the html automatically???

morganadrales commented 2 years ago

@mayank99 isnt our ThemeProvider supposed to add this class to the html automatically???

I think the issue here was that we were wrapping our individual components on this settings page in ThemeProviders but not the root component.

mayank99 commented 2 years ago

@mayank99 isnt our ThemeProvider supposed to add this class to the html automatically???

I think the issue here was that we were wrapping our individual components on this settings page in ThemeProviders but not the root component.

It should apply the class on the root element, regardless of where it is used. So this might be worth investigating further. Users should not be expected to apply that class on their end.

iTwin / iTwinUI-react