From ed41c424de75fcba8751a70b54a189957f979e97 Mon Sep 17 00:00:00 2001
From: Brian Ingles <brianingles@deephaven.io>
Date: Thu, 11 Jan 2024 13:25:58 -0600
Subject: [PATCH] feat: Improved preload variable handling (#1723)

- Preload now always applies default preload values before applying
cached ones. This ensures that cached preload values don't prevent new
defaults from being applied if / when we add new variables to the
preload list
- Default preload variables can now be passed in to ThemeUtils +
ThemeProvider. This will allow DHE to specify additional variables if
needed

resolves #1695 and part of #1679
---
 .../src/styleguide/SpectrumComparison.tsx     | 27 +++++--
 packages/components/src/theme/ThemeModel.ts   |  2 +-
 .../components/src/theme/ThemeProvider.tsx    | 15 +++-
 .../components/src/theme/ThemeUtils.test.ts   | 45 ++++++++---
 packages/components/src/theme/ThemeUtils.ts   | 79 ++++++++++++++-----
 5 files changed, 126 insertions(+), 42 deletions(-)

diff --git a/packages/code-studio/src/styleguide/SpectrumComparison.tsx b/packages/code-studio/src/styleguide/SpectrumComparison.tsx
index 7d23701411..454e82e8b7 100644
--- a/packages/code-studio/src/styleguide/SpectrumComparison.tsx
+++ b/packages/code-studio/src/styleguide/SpectrumComparison.tsx
@@ -24,6 +24,7 @@ import {
   RadioItem,
   Select,
 } from '@deephaven/components';
+import { EMPTY_FUNCTION } from '@deephaven/utils';
 import {
   SAMPLE_SECTION_E2E_IGNORE,
   SPECTRUM_COMPARISON_SAMPLES_ID,
@@ -73,7 +74,9 @@ export function SpectrumComparison(): JSX.Element {
 
             {buttons.map(([level, variant]) => (
               <Fragment key={level}>
-                <BootstrapButtonOld kind={level}>Button</BootstrapButtonOld>
+                <BootstrapButtonOld onClick={EMPTY_FUNCTION} kind={level}>
+                  Button
+                </BootstrapButtonOld>
 
                 <Button variant={variant} style="fill">
                   Button
@@ -81,7 +84,11 @@ export function SpectrumComparison(): JSX.Element {
               </Fragment>
             ))}
 
-            <BootstrapButtonOld kind="primary" disabled>
+            <BootstrapButtonOld
+              onClick={EMPTY_FUNCTION}
+              kind="primary"
+              disabled
+            >
               Disabled
             </BootstrapButtonOld>
             <Button variant="accent" style="fill" isDisabled>
@@ -98,14 +105,20 @@ export function SpectrumComparison(): JSX.Element {
 
             {buttons.map(([level, variant]) => (
               <Fragment key={level}>
-                <BootstrapButtonOld kind={level}>{level}</BootstrapButtonOld>
+                <BootstrapButtonOld onClick={EMPTY_FUNCTION} kind={level}>
+                  {level}
+                </BootstrapButtonOld>
                 <Button variant={variant} style="outline">
                   {variant}
                 </Button>
               </Fragment>
             ))}
 
-            <BootstrapButtonOld kind="secondary" disabled>
+            <BootstrapButtonOld
+              onClick={EMPTY_FUNCTION}
+              kind="secondary"
+              disabled
+            >
               Disabled
             </BootstrapButtonOld>
             <Button variant="primary" style="outline" isDisabled>
@@ -121,10 +134,12 @@ export function SpectrumComparison(): JSX.Element {
             <label>Bootstrap</label>
             <label>Spectrum</label>
 
-            <BootstrapButtonOld kind="inline">Inline</BootstrapButtonOld>
+            <BootstrapButtonOld onClick={EMPTY_FUNCTION} kind="inline">
+              Inline
+            </BootstrapButtonOld>
             <ActionButton>Action</ActionButton>
 
-            <BootstrapButtonOld kind="inline" disabled>
+            <BootstrapButtonOld onClick={EMPTY_FUNCTION} kind="inline" disabled>
               Disabled
             </BootstrapButtonOld>
             <ActionButton isDisabled>Disabled</ActionButton>
diff --git a/packages/components/src/theme/ThemeModel.ts b/packages/components/src/theme/ThemeModel.ts
index 0d024d57b6..2a961b10db 100644
--- a/packages/components/src/theme/ThemeModel.ts
+++ b/packages/components/src/theme/ThemeModel.ts
@@ -43,7 +43,7 @@ export const DEFAULT_LIGHT_THEME_KEY = 'default-light' satisfies BaseThemeKey;
 
 // Hex versions of some of the default dark theme color palette needed for
 // preload defaults.
-const DEFAULT_DARK_THEME_PALETTE = {
+export const DEFAULT_DARK_THEME_PALETTE = {
   blue: {
     500: '#2f5bc0',
     400: '#254ba4',
diff --git a/packages/components/src/theme/ThemeProvider.tsx b/packages/components/src/theme/ThemeProvider.tsx
index c051d4a208..19ad76a7d1 100644
--- a/packages/components/src/theme/ThemeProvider.tsx
+++ b/packages/components/src/theme/ThemeProvider.tsx
@@ -1,6 +1,10 @@
 import { createContext, ReactNode, useEffect, useMemo, useState } from 'react';
 import Log from '@deephaven/log';
-import { DEFAULT_DARK_THEME_KEY, ThemeData } from './ThemeModel';
+import {
+  DEFAULT_DARK_THEME_KEY,
+  DEFAULT_PRELOAD_DATA_VARIABLES,
+  ThemeData,
+} from './ThemeModel';
 import {
   calculatePreloadStyleContent,
   getActiveThemes,
@@ -30,11 +34,13 @@ export interface ThemeProviderProps {
    * tell the provider to activate the base themes.
    */
   themes: ThemeData[] | null;
+  defaultPreloadValues?: Record<string, string>;
   children: ReactNode;
 }
 
 export function ThemeProvider({
   themes: customThemes,
+  defaultPreloadValues = DEFAULT_PRELOAD_DATA_VARIABLES,
   children,
 }: ThemeProviderProps): JSX.Element | null {
   const baseThemes = useMemo(() => getDefaultBaseThemes(), []);
@@ -71,9 +77,10 @@ export function ThemeProvider({
 
       // Override fill color for certain inline SVGs (the originals are provided
       // by theme-svg.scss)
-      overrideSVGFillColors();
+      overrideSVGFillColors(defaultPreloadValues);
 
-      const preloadStyleContent = calculatePreloadStyleContent();
+      const preloadStyleContent =
+        calculatePreloadStyleContent(defaultPreloadValues);
 
       log.debug2('updateThemePreloadData:', {
         active: activeThemes.map(theme => theme.themeKey),
@@ -87,7 +94,7 @@ export function ThemeProvider({
         preloadStyleContent,
       });
     },
-    [activeThemes, selectedThemeKey, customThemes]
+    [activeThemes, selectedThemeKey, customThemes, defaultPreloadValues]
   );
 
   useEffect(() => {
diff --git a/packages/components/src/theme/ThemeUtils.test.ts b/packages/components/src/theme/ThemeUtils.test.ts
index c91c75e168..931571c1cb 100644
--- a/packages/components/src/theme/ThemeUtils.test.ts
+++ b/packages/components/src/theme/ThemeUtils.test.ts
@@ -59,9 +59,9 @@ describe('calculatePreloadStyleContent', () => {
   }
 
   it('should set defaults if css variables are not defined', () => {
-    expect(calculatePreloadStyleContent()).toEqual(
-      expectedContent(DEFAULT_PRELOAD_DATA_VARIABLES)
-    );
+    expect(
+      calculatePreloadStyleContent(DEFAULT_PRELOAD_DATA_VARIABLES)
+    ).toEqual(expectedContent(DEFAULT_PRELOAD_DATA_VARIABLES));
   });
 
   it('should resolve css variables', () => {
@@ -71,7 +71,9 @@ describe('calculatePreloadStyleContent', () => {
     );
     document.body.style.setProperty('--dh-color-bg', 'orange');
 
-    expect(calculatePreloadStyleContent()).toEqual(
+    expect(
+      calculatePreloadStyleContent(DEFAULT_PRELOAD_DATA_VARIABLES)
+    ).toEqual(
       expectedContent({
         ...DEFAULT_PRELOAD_DATA_VARIABLES,
         '--dh-color-loading-spinner-primary': 'pink',
@@ -96,7 +98,10 @@ describe.each([document.body, document.createElement('div')])(
     it('should return empty string if property does not exist and no default value exists', () => {
       asMock(computedStyle.getPropertyValue).mockReturnValue('');
 
-      const resolver = createCssVariableResolver(targetElement);
+      const resolver = createCssVariableResolver(
+        targetElement,
+        DEFAULT_PRELOAD_DATA_VARIABLES
+      );
 
       expect(getComputedStyle).toHaveBeenCalledWith(targetElement);
 
@@ -113,7 +118,10 @@ describe.each([document.body, document.createElement('div')])(
       (key, value) => {
         asMock(computedStyle.getPropertyValue).mockReturnValue('');
 
-        const resolver = createCssVariableResolver(targetElement);
+        const resolver = createCssVariableResolver(
+          targetElement,
+          DEFAULT_PRELOAD_DATA_VARIABLES
+        );
 
         expect(getComputedStyle).toHaveBeenCalledWith(targetElement);
 
@@ -126,7 +134,10 @@ describe.each([document.body, document.createElement('div')])(
         name => `resolved:${name}`
       );
 
-      const resolver = createCssVariableResolver(targetElement);
+      const resolver = createCssVariableResolver(
+        targetElement,
+        DEFAULT_PRELOAD_DATA_VARIABLES
+      );
 
       expect(getComputedStyle).toHaveBeenCalledWith(targetElement);
 
@@ -387,7 +398,7 @@ describe('overrideSVGFillColors', () => {
           : 'red'
       );
 
-      overrideSVGFillColors();
+      overrideSVGFillColors(DEFAULT_PRELOAD_DATA_VARIABLES);
 
       expect(getComputedStyle).toHaveBeenCalledWith(document.body);
       expect(document.body.style.removeProperty).toHaveBeenCalledWith(key);
@@ -418,12 +429,22 @@ describe('preloadTheme', () => {
 
     preloadTheme();
 
-    const styleEl = document.querySelector('style');
+    const [styleElDefaults, styleElPrevious] =
+      document.querySelectorAll('style');
 
-    expect(styleEl).not.toBeNull();
-    expect(styleEl?.innerHTML).toEqual(
-      preloadData?.preloadStyleContent ?? calculatePreloadStyleContent()
+    expect(styleElDefaults).not.toBeNull();
+    expect(styleElDefaults?.innerHTML).toEqual(
+      calculatePreloadStyleContent(DEFAULT_PRELOAD_DATA_VARIABLES)
     );
+
+    if (preloadData?.preloadStyleContent == null) {
+      expect(styleElPrevious).toBeUndefined();
+    } else {
+      expect(styleElPrevious).toBeDefined();
+      expect(styleElPrevious?.innerHTML).toEqual(
+        preloadData?.preloadStyleContent
+      );
+    }
   });
 });
 
diff --git a/packages/components/src/theme/ThemeUtils.ts b/packages/components/src/theme/ThemeUtils.ts
index 6211e1ecfa..4b9cc44073 100644
--- a/packages/components/src/theme/ThemeUtils.ts
+++ b/packages/components/src/theme/ThemeUtils.ts
@@ -31,13 +31,19 @@ export type VarExpressionResolver = (varExpression: string) => string;
  * happens before themes are fully loaded so that we can style things like the
  * loading spinner and background color which are shown to the user early on in
  * the app lifecycle.
+ * @defaultPreloadValues Default values to use if a preload variable is not set.
  */
-export function calculatePreloadStyleContent(): CssVariableStyleContent {
-  const resolveVar = createCssVariableResolver(document.body);
+export function calculatePreloadStyleContent(
+  defaultPreloadValues: Record<string, string>
+): CssVariableStyleContent {
+  const resolveVar = createCssVariableResolver(
+    document.body,
+    defaultPreloadValues
+  );
 
   // Calculate the current preload variables. If the variable is not set, use
   // the default value.
-  const pairs = Object.keys(DEFAULT_PRELOAD_DATA_VARIABLES).map(
+  const pairs = Object.keys(defaultPreloadValues).map(
     key => `${key}:${resolveVar(key as ThemePreloadColorVariable)}`
   );
 
@@ -47,12 +53,14 @@ export function calculatePreloadStyleContent(): CssVariableStyleContent {
 /**
  * Create a resolver function for calculating the value of a css variable based
  * on a given element's computed style. If the variable resolves to '', we check
- * DEFAULT_PRELOAD_DATA_VARIABLES for a default value, and if one does not exist,
+ * `defaultValues` for a default value, and if one does not exist,
  * return ''.
  * @param el Element to resolve css variables against
+ * @param defaultValues Default values to use if a variable is not set.
  */
 export function createCssVariableResolver(
-  el: Element
+  el: Element,
+  defaultValues: Record<string, string>
 ): (varName: ThemeCssVariableName) => string {
   const computedStyle = getComputedStyle(el);
 
@@ -67,12 +75,25 @@ export function createCssVariableResolver(
       return value;
     }
 
-    return (
-      DEFAULT_PRELOAD_DATA_VARIABLES[varName as ThemePreloadColorVariable] ?? ''
-    );
+    return defaultValues[varName as ThemePreloadColorVariable] ?? '';
   };
 }
 
+/**
+ * Create a style tag containing preload css variables and add to the head.
+ * @param id The id of the style tag
+ * @param preloadStyleContent The css variable content to add to the style tag
+ */
+export function createPreloadStyleElement(
+  id: `theme-preload-${string}`,
+  preloadStyleContent: CssVariableStyleContent
+): void {
+  const style = document.createElement('style');
+  style.id = id;
+  style.innerHTML = preloadStyleContent;
+  document.head.appendChild(style);
+}
+
 /**
  * Extracts all css variable expressions from the given record and returns
  * a set of unique expressions.
@@ -378,18 +399,35 @@ export function getThemeKey(pluginName: string, themeName: string): string {
 
 /**
  * Preload minimal theme variables from the cache.
+ * @defaultPreloadValues Optional default values to use if a preload variable is not set.
  */
-export function preloadTheme(): void {
-  const preloadStyleContent =
-    getThemePreloadData()?.preloadStyleContent ??
-    calculatePreloadStyleContent();
+export function preloadTheme(
+  defaultPreloadValues: Record<string, string> = DEFAULT_PRELOAD_DATA_VARIABLES
+): void {
+  const previousPreloadStyleContent =
+    getThemePreloadData()?.preloadStyleContent;
+
+  const defaultPreloadStyleContent =
+    calculatePreloadStyleContent(defaultPreloadValues);
+
+  log.debug('Preloading theme content:', {
+    defaultPreloadStyleContent,
+    previousPreloadStyleContent,
+  });
 
-  log.debug('Preloading theme content:', `'${preloadStyleContent}'`);
+  createPreloadStyleElement(
+    'theme-preload-defaults',
+    defaultPreloadStyleContent
+  );
 
-  const style = document.createElement('style');
-  style.id = 'theme-preload';
-  style.innerHTML = preloadStyleContent;
-  document.head.appendChild(style);
+  // Any preload variables that were saved by last theme load should override
+  // the defaults
+  if (previousPreloadStyleContent != null) {
+    createPreloadStyleElement(
+      'theme-preload-previous',
+      previousPreloadStyleContent
+    );
+  }
 }
 
 /**
@@ -406,9 +444,12 @@ export function preloadTheme(): void {
  * just change the background color instead of relying on this util, but this
  * is not always possible. e.g. <select> elements don't support pseudo elements,
  * so there's not a good way to set icons via masks.
+ * @param defaultValues Default values to use if a variable is not set.
  */
-export function overrideSVGFillColors(): void {
-  const resolveVar = createCssVariableResolver(document.body);
+export function overrideSVGFillColors(
+  defaultValues: Record<string, string>
+): void {
+  const resolveVar = createCssVariableResolver(document.body, defaultValues);
 
   Object.entries(SVG_ICON_MANUAL_COLOR_MAP).forEach(([key, value]) => {
     // Clear any previous override so that our variables get resolved against the