Feat (core): Make theme settings user-specific and user-configurable

[joshuarrrr]

Description

The purpose of this PR is to make theme settings (dark mode and theme version) settable by device, rather than cluster-wide. This allows different users (or users on different devices) to use different themes and light/dark mode. It also adds a new global nav element for global access to theming controls. These changes, however, can be toggled off in advanced settings if the previous behavior is desired/preferred.

This is an iterative step toward user-specific settings that can be saved/shared across devices, and it should unblock #4454.

The biggest change is that it's no longer possible for any server system or component to know the theme version or dark mode settings, because the source of truth for theme settings resides client-side (stored in localstorage).

Notes for reviewers:

1. Start with the new theming documentation/diagram, which provides some background for why the implementation looks like it does.
2. Look at core/server/rendering to see how server-defined templates and styles are made theme/mode agnostic
3. Then see how the theme-specific logic has been moved to a client-side script under legacy/ui/ui_render. Unlike the existing script. This startup script is required to provide the theming info needed by the loading/error screen prior to starting the react app. Theme info not required by the loading page stays in the existing bootstrap script, which also pulls in some of the conditional theming logic that can no longer be done in the templates.
4. See how the ui_settings service was updated (both server and client). We define a new property preferBrowserSetting which indicates settings that have a local storage source of truth. The public client then reads and writes those settings appropriately. Note that this means even editing theme settings from the Advanced Setting page will no longer update the cluster setting - it will also just be considered device-specific.
5. Finally see how the branding prop is injected in the bootstrap process and how the new header control is registered and defined.

Issues Resolved

Fixes #4462

Screenshot

user-theme-demo.mov

Testing the changes

See video above, but some scenarios to verify:

1. For new instance, user appearance menu should appear, and it should work to set either theme, color mode or both
2. When choosing "Use browser settings", the dark mode should match browser settings (after page reload)
3. For existing instances, user appearance menu should appear, but with default values that match what was previously set via advanced settings.
4. In advanced settings, theming settings are disabled with explanatory message
5. When the Enable user control setting is disabled, all theming behavior should work as previously (advanced settings controls enabled), with no user appearance menu.
6. Any changes made to theme settings in advanced settings menu should persist as defaults if Enable user control is enabled
7. Any callers of the client-side ui_settings_client will get the theme values that represent the state of those settings, not necessarily the settings value (for backwards compatibility). E.g. if user settings are enabled and set to "Use browser settings", consumers calling uiClient.get('theme:darkMode') should get back true if that was the browser setting at page load/application start.
8. Callers of the server-side ui_settings_client will always get server/saved object default values, which may not reflect app state

Check List

• All tests pass
  • yarn test:jest
  • yarn test:jest_integration
• New functionality includes testing.
• New functionality has been documented.
• Update CHANGELOG.md
• Commits are signed per the DCO using --signoff

[codecov]

Codecov Report

Attention: Patch coverage is 22.41379% with 90 lines in your changes are missing coverage. Please review.

Project coverage is 67.69%. Comparing base (fea8202) to head (ca2a00e).
Report is 3 commits behind head on main.

Files	Patch %	Lines
...dvanced_settings/public/header_user_theme_menu.tsx	2.43%	40 Missing ⚠️
src/core/public/ui_settings/ui_settings_client.ts	13.63%	16 Missing and 3 partials ⚠️
src/legacy/ui/ui_render/ui_render_mixin.js	0.00%	18 Missing ⚠️
src/core/server/rendering/rendering_service.tsx	50.00%	0 Missing and 3 partials ⚠️
src/plugins/advanced_settings/public/plugin.ts	0.00%	3 Missing ⚠️
.../advanced_settings/public/register_nav_control.tsx	0.00%	3 Missing ⚠️
...s/public/management_app/components/field/field.tsx	75.00%	1 Missing and 1 partial ⚠️
src/core/public/osd_bootstrap.ts	66.66%	0 Missing and 1 partial ⚠️
...gs/public/management_app/lib/to_editable_config.ts	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5652      +/-   ##
==========================================
- Coverage   67.79%   67.69%   -0.10%     
==========================================
  Files        3413     3414       +1     
  Lines       66755    66838      +83     
  Branches    10861    10872      +11     
==========================================
- Hits        45254    45247       -7     
- Misses      18857    18919      +62     
- Partials     2644     2672      +28     

Flag	Coverage Δ
Linux_1	33.21% <5.26%> (+0.02%)	⬆️
Linux_2	?
Linux_3	?
Linux_4	?
Windows_1	33.26% <5.26%> (+0.04%)	⬆️
Windows_2	55.53% <25.42%> (-0.07%)	⬇️
Windows_3	45.24% <18.75%> (-0.02%)	⬇️
Windows_4	34.87% <8.24%> (-0.05%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[joshuarrrr]

For the failing tests:

     │1)    dashboard app
     │       using current data
     │         dashboard snapshots
     │           compare TSVB snapshot:
     │
     │      Error: expected 0.021522 to be below 0.02
     │       at Assertion.assert (packages/osd-expect/expect.js:111:11)
     │       at Assertion.lessThan.Assertion.below (packages/osd-expect/expect.js:347:8)
     │       at Function.lessThan (packages/osd-expect/expect.js:542:15)
     │       at Context.<anonymous> (test/functional/apps/dashboard/dashboard_snapshots.js:78:39)
     │       at processTicksAndRejections (node:internal/process/task_queues:95:5)
     │       at Object.apply (packages/osd-test/src/functional_test_runner/lib/mocha/wrap_function.js:95:16)

This looks like a flaky test to me - has it failed on other PRs? I don't see how theming changes could make this value round incorrectly. And the CI test group 3 passed on the Windows run.

│1)    visualize app
     │       
     │         OpenSearch Dashboards branding configuration
     │           should render home page
     │             in default mode
     │               with customized logo in header bar:
     │
     │      Error: expected 'https://opensearch.org/ASSETS/BRAND/SVG/LOGO/OPENSEARCH_LOGO_DARKMODE.SVG' to equal 'https://opensearch.org/ASSETS/BRAND/SVG/LOGO/OPENSEARCH_LOGO_DEFAULT.SVG'
     │       at Assertion.assert (packages/osd-expect/expect.js:111:11)
     │       at Assertion.equal (packages/osd-expect/expect.js:238:8)
     │       at GlobalNav.logoExistsOrFail (test/functional/services/global_nav.ts:90:44)
     │       at processTicksAndRejections (node:internal/process/task_queues:95:5)
     │       at Context.<anonymous> (test/functional/apps/visualize/_custom_branding.ts:128:11)
     │       at Object.apply (packages/osd-test/src/functional_test_runner/lib/mocha/wrap_function.js:95:16)

@AMoo-Miki Can you help me look at this? As far as I can tell the behavior of this PR is actually what we intend/want to happen, since merging #4702. So I think the test case itself is wrong, but it's puzzling that it has been passing in its current state. Just want your confirmation before updating the test case to match the behavior (where we always use the dark mode version of the logo since it's always on a dark background component)

[AMoo-Miki]

@joshuarrrr

     │1)    dashboard app
     │       using current data
     │         dashboard snapshots
     │           compare TSVB snapshot:
     │
     │      Error: expected 0.021522 to be below 0.02

This looks like a flaky test to me - has it failed on other PRs? I don't see how theming changes could make this value round incorrectly. And the CI test group 3 passed on the Windows run.

This is the result of visual diff. It is saying that too much has changed. The solution is to update the base image after validating that the visual changes are desired.

     │1)    visualize app
     │       
     │         OpenSearch Dashboards branding configuration
     │           should render home page
     │             in default mode
     │               with customized logo in header bar:
     │
     │      Error: expected 'https://opensearch.org/ASSETS/BRAND/SVG/LOGO/OPENSEARCH_LOGO_DARKMODE.SVG'
     |      to equal 'https://opensearch.org/ASSETS/BRAND/SVG/LOGO/OPENSEARCH_LOGO_DEFAULT.SVG'

As far as I can tell the behavior of this PR is actually what we intend/want to happen, since merging #4702. So I think the test case itself is wrong, but it's puzzling that it has been passing in its current state... where we always use the dark mode version of the logo since it's always on a dark background component

I too believe the test is wrong. The expanded header shows up with a dark background irrespective of the color scheme and because of that, we will always have a dark logo there.

OpenSearch-Dashboards/src/core/public/chrome/ui/header/header.tsx

Line 131 in c2c4f53

const expandedHeaderColorScheme: EuiHeaderProps['theme'] = 'dark';

A collapsed header uses the theme's background color which is why the next test passes.

[joshuarrrr]

A collapsed header uses the theme's background color which is why the next test passes.

Ah, right - this is the part I forgot. Let me manually retest to make sure I didn't miss something here.

[opensearch-trigger-bot]

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-5652-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 876224bd9018d5b1dc25f033734676d150f21fc4
# Push it to GitHub
git push --set-upstream origin backport/backport-5652-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-5652-to-2.x.

[seraphjiang]

do we have functional test automation for test coverage?