Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for light/dark theme from system setting #89340

Closed
lukasolson opened this issue Jan 26, 2021 · 7 comments · Fixed by #173044
Closed

Support for light/dark theme from system setting #89340

lukasolson opened this issue Jan 26, 2021 · 7 comments · Fixed by #173044
Labels
enhancement New value added to drive a business result Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins.

Comments

@lukasolson
Copy link
Member

Describe the feature:

Currently, an advanced setting theme:darkMode controls whether you see Kibana in light or dark mode.

Instead of having a simple toggle (dark mode on/off), it'd really be nice if there were three options: Light mode, dark mode, or browser setting (similar to how we do timezones). It'd be especially nice for those Mac users who have the preference set to "auto" so it changes based on the time of day automatically :)

Most modern browsers support inferring the user's preference via window.matchMedia('(prefers-color-scheme: dark)') (or light).

@lukasolson lukasolson added Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. enhancement New value added to drive a business result labels Jan 26, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-design (Team:Kibana-Design)

@knechtionscoding
Copy link

Also, having this be a user specific setting. Having to set it for an entire space is clunky and very odd compared to most other implementations.

@pgayvallet
Copy link
Contributor

Tentatively assigning for on-week

@pgayvallet
Copy link
Contributor

So, I was able to develop a POC within a day.

I was hoping to simply get away by moving the css files "logic" from the server-side rendering service to somewhere in the UI side (by sending the list of css files for each theme to the browser) and add a third system option for theme:darkMode.

It worked fairly well on the advanced settings page, even for dynamic theme switching, which was very encouraging.

Screen.Recording.2023-12-11.at.13.25.59.mov

Then I started navigating though the site, and observed some strange behavior in most pages:

Screen.Recording.2023-12-11.at.20.34.52.mov

This is when I started to suspect that some things were still using "static" versions of some of their styles, that would be loaded only once or something.

The first obvious one was the inline styles that we are using for the global loader:

const InlineStyles: FC<{ darkMode: boolean }> = ({ darkMode }) => {
/* eslint-disable react/no-danger */
return (
<style

The second one are the plugins scss files and the way we're loading them.

For example, we have a component importing a sass file:

and then the sass file using an eui varible that depends on the theme:

.controlFrame__control {
height: 100%;
transition: opacity .1s;
background-color: $euiFormBackgroundColor !important;

Under the hood, our optimizer / bundler is looking at the window.__kbnThemeTag__ variable to determine which theme should be used for variable interpolation for sass files.

However, once the component/themed-css file is loaded, dynamically switching without a page reload doesn't reload the css (as it does for the new EUI theme provided based on Emotion/jss)

TLDR:

  • applying system theme during page load mostly (fully?) works
  • changing the theme after initial page load cause glitches (so it should probably be disabled or moved being another setting)
  • still need to figure out what to do with inline styles used for the loading screen

@pgayvallet
Copy link
Contributor

pgayvallet commented Dec 18, 2023

So, the PR is ready: #173044, however during development of the PR we discovered this issue: #173529 which seems to be a blocker for this feature.

@legrego
Copy link
Member

legrego commented Dec 18, 2023

Not a blocker, but it would be great for us to have a plan for #155945 before merging, too. Otherwise, we'll have a confusing feature gap between the two systems.

@pgayvallet
Copy link
Contributor

@legrego I created #173895 to discuss how to close this feature gap.

pgayvallet added a commit that referenced this issue Feb 20, 2024
## Summary

Fix #89340

Implements a third option, `system`, for the `theme:darkMode`
uiSettings, which will follow the system's theme preference (light/dark)
when Kibana loads.


https://github.com/elastic/kibana/assets/1532934/82078697-8bf5-41df-add1-4ecfed6e1dea

**Note: system theme refresh still requires the user to reload Kibana -
please see the next section for the reasons if you're interested**


## How theming works in Kibana, again?

This is an excellent question, thanks for asking. And the answer is,
"well, it's complicated".

We have multiples sources of "themed" styles in Kibana, ordered from
"best" to "worse":

#### 1. the EUI/JSS Theming

It was initially implemented in
#117368. All react applications
and react mountpoints are supposed to be wrapped by a
`KibanaThemeProvider` that bridges core's `theme$` values to the
`EuiProvider`.


https://github.com/elastic/kibana/blob/477505a2dd86d8f103f3aef16b3b4b76dff0b27a/packages/core/theme/core-theme-browser-internal/src/core_theme_provider.tsx#L11

This one was already dynamic and just works perfectly. If
`core.theme.theme$` changes, the new values is received by the theme
provider, which automatically changes the styles accordingly, creating
this sexy "it just works" effect:


https://github.com/elastic/kibana/assets/1532934/f3e61ca7-f3ed-4c37-aa46-76ee68c1a628

If everything theme-related was using this approach, dynamic theme
reload would have been possible. However, Kibana has a lot of legacy, so
as you can imagine, it wasn't that easy.

So, **don't get false hopes** (as I did when I tried it...) from this
video. Dynamic theme swap **could not** be implemented in this PR. And
the reasons are just below.

#### 2. Per-theme css files


https://github.com/elastic/kibana/blob/6443b571642c453a9232a04297fddfb0a918c0dc/packages/core/rendering/core-rendering-server-internal/src/render_utils.ts#L40-L54

We have a bunch of dark/light variations of some css files that are
computed in the rendering service, server-side, to be injected into the
page template.

Of course, this doesn't play well with dynamic theming, given the UI
then doesn't know which css should be swapped, and which one should be
added instead.

However, porting the responsibilities of which theme css files to load
to the browser was achievable, and done in this PR. core's browser-side
`theme` provider now receives the list of theme files for both light and
dark theme (via the injected metadata), and inject them in the document
(instead of having them pre-injected in the page template by the
rendering service server-side).

So this one wasn't a blocker for dynamic theme reload.  

#### 3. Plugin styles 

This is where the problems start.

Plugins can ship their own styles too, by importing them from components
or from their entrypoint.

E.g this component


https://github.com/elastic/kibana/blob/f1dc1e1869566c1d61a08bb67eb3d4ac2f47834e/src/plugins/controls/public/control_group/component/control_group_component.tsx#L9

importing this file:


https://github.com/elastic/kibana/blob/bafb23580b18781e711575cd8c0e17a6e3f4f740/src/plugins/controls/public/control_group/control_group.scss#L107-L110

Which relies on a theme variable, `$euiFormBackgroundColor`

So how does it works under the hood? How is that
`$euiFormBackgroundColor` variable interpolated? Using which theme?

Well, technically, how the styles are effectively loaded differs
slightly between dev and production (different webpack loaders/config),
but in both cases, it depends on the `__kbnThemeTag__` variable that is
injected to the global scope by the `bootstrap.js` script.

This `__kbnThemeTag__` tag (apparently) **can** be modified after page
load. However it doesn't magically reload everything, so styles already
loaded for one theme will not reload. If a component and its imported
styles were already compiled / injected, then they won't reload

As a short video is better than 3 blocks of text, just see:


https://github.com/elastic/kibana/assets/1532934/3087ffd6-80d8-42bf-ab17-691ec408ea6f

That was the first blocker for supporting "dynamic" reloads of the
system theme.

#### 4. Static inline styles

Last but not least, we have some static style injected in the template,
that also depend on the current theme.


https://github.com/elastic/kibana/blob/6443b571642c453a9232a04297fddfb0a918c0dc/packages/core/rendering/core-rendering-server-internal/src/views/styles.tsx#L52-L54

Of course this plays very badly with dynamic theming. And worth noting,
those styles are used by the "Loading Elastic" screen, where Core (and
therefore Core's theming service) isn't loaded yet, which made the
things even more complicated.

This was the second blocker for supporting "dynamic" reloads of the
system theme.

#### 5. `euiThemeVars`

Actually TIL (not that I was asking for it)

We're exposing the EUI theme variable via the `euiThemeVars` of the
`@kbn/ui-theme` package:

E.g.


https://github.com/elastic/kibana/blob/c7e785383a87f7e18509c601a5c089c755ac028e/src/plugins/chart_expressions/expression_metric/public/components/metric_vis.tsx#L41


https://github.com/elastic/kibana/blob/c7e785383a87f7e18509c601a5c089c755ac028e/src/plugins/chart_expressions/expression_metric/public/components/metric_vis.tsx#L50

So I did my best, and made it that this export was a proxy, and that
Core's theme service was dynamically swapping the target of the proxy
depending on the system's theme...


https://github.com/elastic/kibana/blob/b0a0017811d7402f76709af7ab1b8e12334e64a5/packages/kbn-ui-theme/src/theme.ts#L30-L42

Unfortunately it doesn't fully work for dynamic system theme switch,
given modules/code can keep references of the property directly (e.g.
the snippet a few lines on top), and there's nothing to dynamically
reload components when the proxy changes.

So yet another blocker for dynamic theme switch. 


## Release Notes

Add a new option, `system`, to the `theme:darkMode` Kibana advanced
setting, that can be used to have Kibana's theme follow the system's
(light or dark)

---------

Co-authored-by: kibanamachine <[email protected]>
fkanout pushed a commit to fkanout/kibana that referenced this issue Mar 4, 2024
)

## Summary

Fix elastic#89340

Implements a third option, `system`, for the `theme:darkMode`
uiSettings, which will follow the system's theme preference (light/dark)
when Kibana loads.


https://github.com/elastic/kibana/assets/1532934/82078697-8bf5-41df-add1-4ecfed6e1dea

**Note: system theme refresh still requires the user to reload Kibana -
please see the next section for the reasons if you're interested**


## How theming works in Kibana, again?

This is an excellent question, thanks for asking. And the answer is,
"well, it's complicated".

We have multiples sources of "themed" styles in Kibana, ordered from
"best" to "worse":

#### 1. the EUI/JSS Theming

It was initially implemented in
elastic#117368. All react applications
and react mountpoints are supposed to be wrapped by a
`KibanaThemeProvider` that bridges core's `theme$` values to the
`EuiProvider`.


https://github.com/elastic/kibana/blob/477505a2dd86d8f103f3aef16b3b4b76dff0b27a/packages/core/theme/core-theme-browser-internal/src/core_theme_provider.tsx#L11

This one was already dynamic and just works perfectly. If
`core.theme.theme$` changes, the new values is received by the theme
provider, which automatically changes the styles accordingly, creating
this sexy "it just works" effect:


https://github.com/elastic/kibana/assets/1532934/f3e61ca7-f3ed-4c37-aa46-76ee68c1a628

If everything theme-related was using this approach, dynamic theme
reload would have been possible. However, Kibana has a lot of legacy, so
as you can imagine, it wasn't that easy.

So, **don't get false hopes** (as I did when I tried it...) from this
video. Dynamic theme swap **could not** be implemented in this PR. And
the reasons are just below.

#### 2. Per-theme css files


https://github.com/elastic/kibana/blob/6443b571642c453a9232a04297fddfb0a918c0dc/packages/core/rendering/core-rendering-server-internal/src/render_utils.ts#L40-L54

We have a bunch of dark/light variations of some css files that are
computed in the rendering service, server-side, to be injected into the
page template.

Of course, this doesn't play well with dynamic theming, given the UI
then doesn't know which css should be swapped, and which one should be
added instead.

However, porting the responsibilities of which theme css files to load
to the browser was achievable, and done in this PR. core's browser-side
`theme` provider now receives the list of theme files for both light and
dark theme (via the injected metadata), and inject them in the document
(instead of having them pre-injected in the page template by the
rendering service server-side).

So this one wasn't a blocker for dynamic theme reload.  

#### 3. Plugin styles 

This is where the problems start.

Plugins can ship their own styles too, by importing them from components
or from their entrypoint.

E.g this component


https://github.com/elastic/kibana/blob/f1dc1e1869566c1d61a08bb67eb3d4ac2f47834e/src/plugins/controls/public/control_group/component/control_group_component.tsx#L9

importing this file:


https://github.com/elastic/kibana/blob/bafb23580b18781e711575cd8c0e17a6e3f4f740/src/plugins/controls/public/control_group/control_group.scss#L107-L110

Which relies on a theme variable, `$euiFormBackgroundColor`

So how does it works under the hood? How is that
`$euiFormBackgroundColor` variable interpolated? Using which theme?

Well, technically, how the styles are effectively loaded differs
slightly between dev and production (different webpack loaders/config),
but in both cases, it depends on the `__kbnThemeTag__` variable that is
injected to the global scope by the `bootstrap.js` script.

This `__kbnThemeTag__` tag (apparently) **can** be modified after page
load. However it doesn't magically reload everything, so styles already
loaded for one theme will not reload. If a component and its imported
styles were already compiled / injected, then they won't reload

As a short video is better than 3 blocks of text, just see:


https://github.com/elastic/kibana/assets/1532934/3087ffd6-80d8-42bf-ab17-691ec408ea6f

That was the first blocker for supporting "dynamic" reloads of the
system theme.

#### 4. Static inline styles

Last but not least, we have some static style injected in the template,
that also depend on the current theme.


https://github.com/elastic/kibana/blob/6443b571642c453a9232a04297fddfb0a918c0dc/packages/core/rendering/core-rendering-server-internal/src/views/styles.tsx#L52-L54

Of course this plays very badly with dynamic theming. And worth noting,
those styles are used by the "Loading Elastic" screen, where Core (and
therefore Core's theming service) isn't loaded yet, which made the
things even more complicated.

This was the second blocker for supporting "dynamic" reloads of the
system theme.

#### 5. `euiThemeVars`

Actually TIL (not that I was asking for it)

We're exposing the EUI theme variable via the `euiThemeVars` of the
`@kbn/ui-theme` package:

E.g.


https://github.com/elastic/kibana/blob/c7e785383a87f7e18509c601a5c089c755ac028e/src/plugins/chart_expressions/expression_metric/public/components/metric_vis.tsx#L41


https://github.com/elastic/kibana/blob/c7e785383a87f7e18509c601a5c089c755ac028e/src/plugins/chart_expressions/expression_metric/public/components/metric_vis.tsx#L50

So I did my best, and made it that this export was a proxy, and that
Core's theme service was dynamically swapping the target of the proxy
depending on the system's theme...


https://github.com/elastic/kibana/blob/b0a0017811d7402f76709af7ab1b8e12334e64a5/packages/kbn-ui-theme/src/theme.ts#L30-L42

Unfortunately it doesn't fully work for dynamic system theme switch,
given modules/code can keep references of the property directly (e.g.
the snippet a few lines on top), and there's nothing to dynamically
reload components when the proxy changes.

So yet another blocker for dynamic theme switch. 


## Release Notes

Add a new option, `system`, to the `theme:darkMode` Kibana advanced
setting, that can be used to have Kibana's theme follow the system's
(light or dark)

---------

Co-authored-by: kibanamachine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants