[Lens] fix breaking color picker when value is incorrect (#133796) (#134033)

* [Lens] fix breaking color picker when value is incorrect

* positive test

* [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix'

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* fix when removing

* refactor

* deeper refactoring

* remove unused

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
(cherry picked from commit 1c651ed9bf)

Co-authored-by: Marta Bondyra <4283304+mbondyra@users.noreply.github.com>

x-pack/plugins/lens/public/xy_visualization/annotations/helpers.tsx

@@ -12,6 +12,7 @@ import {
   defaultAnnotationRangeColor,
   isRangeAnnotation,
 } from '@kbn/event-annotation-plugin/public';
+import { EventAnnotationConfig } from '@kbn/event-annotation-plugin/common';
 import { layerTypes } from '../../../common';
 import type { FramePublicAPI, Visualization } from '../../types';
 import { isHorizontalChart } from '../state_helpers';
@@ -168,17 +169,16 @@ export const setAnnotationsDimension: Visualization<XYState>['setDimension'] = (
   };
 };
 
-export const getAnnotationsAccessorColorConfig = (layer: XYAnnotationLayerConfig) => {
-  return layer.annotations.map((annotation) => {
-    return {
-      columnId: annotation.id,
-      triggerIcon: annotation.isHidden ? ('invisible' as const) : ('color' as const),
-      color:
-        annotation?.color ||
-        (isRangeAnnotation(annotation) ? defaultAnnotationRangeColor : defaultAnnotationColor),
-    };
-  });
-};
+export const getSingleColorAnnotationConfig = (annotation: EventAnnotationConfig) => ({
+  columnId: annotation.id,
+  triggerIcon: annotation.isHidden ? ('invisible' as const) : ('color' as const),
+  color:
+    annotation?.color ||
+    (isRangeAnnotation(annotation) ? defaultAnnotationRangeColor : defaultAnnotationColor),
+});
+
+export const getAnnotationsAccessorColorConfig = (layer: XYAnnotationLayerConfig) =>
+  layer.annotations.map((annotation) => getSingleColorAnnotationConfig(annotation));
 
 export const getAnnotationsConfiguration = ({
   state,

x-pack/plugins/lens/public/xy_visualization/color_assignment.ts

@@ -9,12 +9,20 @@ import { uniq, mapValues } from 'lodash';
 import type { PaletteOutput, PaletteRegistry } from '@kbn/coloring';
 import type { Datatable } from '@kbn/expressions-plugin';
 import { euiLightVars } from '@kbn/ui-theme';
+import {
+  defaultAnnotationColor,
+  defaultAnnotationRangeColor,
+  isRangeAnnotation,
+} from '@kbn/event-annotation-plugin/public';
 import type { AccessorConfig, FramePublicAPI } from '../types';
 import { getColumnToLabelMap } from './state_helpers';
 import { FormatFactory } from '../../common';
 import { isDataLayer, isReferenceLayer, isAnnotationsLayer } from './visualization_helpers';
 import { getAnnotationsAccessorColorConfig } from './annotations/helpers';
-import { getReferenceLineAccessorColorConfig } from './reference_line_helpers';
+import {
+  getReferenceLineAccessorColorConfig,
+  getSingleColorConfig,
+} from './reference_line_helpers';
 import { XYDataLayerConfig, XYLayerConfig } from './types';
 
 const isPrimitive = (value: unknown): boolean => value != null && typeof value !== 'object';
@@ -96,7 +104,67 @@ export function getColorAssignments(
   });
 }
 
-export function getAccessorColorConfig(
+function getDisabledConfig(accessor: string) {
+  return {
+    columnId: accessor as string,
+    triggerIcon: 'disabled' as const,
+  };
+}
+
+export function getAssignedColorConfig(
+  layer: XYLayerConfig,
+  accessor: string,
+  colorAssignments: ColorAssignments,
+  frame: Pick<FramePublicAPI, 'datasourceLayers'>,
+  paletteService: PaletteRegistry
+): AccessorConfig {
+  if (isReferenceLayer(layer)) {
+    return getSingleColorConfig(accessor);
+  }
+  if (isAnnotationsLayer(layer)) {
+    const annotation = layer.annotations.find((a) => a.id === accessor);
+    return {
+      columnId: accessor,
+      triggerIcon: annotation?.isHidden ? ('invisible' as const) : ('color' as const),
+      color: isRangeAnnotation(annotation) ? defaultAnnotationRangeColor : defaultAnnotationColor,
+    };
+  }
+  const layerContainsSplits = isDataLayer(layer) && !layer.collapseFn && layer.splitAccessor;
+  const currentPalette: PaletteOutput = layer.palette || { type: 'palette', name: 'default' };
+  const totalSeriesCount = colorAssignments[currentPalette.name]?.totalSeriesCount;
+
+  if (layerContainsSplits) {
+    return getDisabledConfig(accessor);
+  }
+
+  const columnToLabel = getColumnToLabelMap(layer, frame.datasourceLayers[layer.layerId]);
+  const rank = colorAssignments[currentPalette.name].getRank(
+    layer,
+    columnToLabel[accessor] || accessor,
+    accessor
+  );
+  const assignedColor =
+    totalSeriesCount != null
+      ? paletteService.get(currentPalette.name).getCategoricalColor(
+          [
+            {
+              name: columnToLabel[accessor] || accessor,
+              rankAtDepth: rank,
+              totalSeriesAtDepth: totalSeriesCount,
+            },
+          ],
+          { maxDepth: 1, totalSeries: totalSeriesCount },
+          currentPalette.params
+        )
+      : undefined;
+  return {
+    columnId: accessor as string,
+    triggerIcon: assignedColor ? 'color' : 'disabled',
+    color: assignedColor ?? undefined,
+  };
+}
+
+export function getAccessorColorConfigs(
   colorAssignments: ColorAssignments,
   frame: Pick<FramePublicAPI, 'datasourceLayers'>,
   layer: XYLayerConfig,
@@ -109,42 +177,18 @@ export function getAccessorColorConfig(
     return getAnnotationsAccessorColorConfig(layer);
   }
   const layerContainsSplits = !layer.collapseFn && layer.splitAccessor;
-  const currentPalette: PaletteOutput = layer.palette || { type: 'palette', name: 'default' };
-  const totalSeriesCount = colorAssignments[currentPalette.name]?.totalSeriesCount;
   return layer.accessors.map((accessor) => {
-    const currentYConfig = layer.yConfig?.find((yConfig) => yConfig.forAccessor === accessor);
     if (layerContainsSplits) {
+      return getDisabledConfig(accessor);
+    }
+    const currentYConfig = layer.yConfig?.find((yConfig) => yConfig.forAccessor === accessor);
+    if (currentYConfig?.color) {
       return {
         columnId: accessor as string,
-        triggerIcon: 'disabled',
+        triggerIcon: 'color',
+        color: currentYConfig.color,
       };
     }
-
-    const columnToLabel = getColumnToLabelMap(layer, frame.datasourceLayers[layer.layerId]);
-    const rank = colorAssignments[currentPalette.name].getRank(
-      layer,
-      columnToLabel[accessor] || accessor,
-      accessor
-    );
-    const customColor =
-      currentYConfig?.color ||
-      (totalSeriesCount != null
-        ? paletteService.get(currentPalette.name).getCategoricalColor(
-            [
-              {
-                name: columnToLabel[accessor] || accessor,
-                rankAtDepth: rank,
-                totalSeriesAtDepth: totalSeriesCount,
-              },
-            ],
-            { maxDepth: 1, totalSeries: totalSeriesCount },
-            currentPalette.params
-          )
-        : undefined);
-    return {
-      columnId: accessor as string,
-      triggerIcon: customColor ? 'color' : 'disabled',
-      color: customColor ?? undefined,
-    };
+    return getAssignedColorConfig(layer, accessor, colorAssignments, frame, paletteService);
   });
 }

x-pack/plugins/lens/public/xy_visualization/reference_line_helpers.tsx

@@ -370,7 +370,7 @@ export const setReferenceDimension: Visualization<XYState>['setDimension'] = ({
   };
 };
 
-const getSingleColorConfig = (id: string, color = defaultReferenceLineColor) => ({
+export const getSingleColorConfig = (id: string, color = defaultReferenceLineColor) => ({
   columnId: id,
   triggerIcon: 'color' as const,
   color,

x-pack/plugins/lens/public/xy_visualization/visualization.tsx

@@ -31,7 +31,7 @@ import { State, visualizationTypes, XYSuggestion, XYLayerConfig, XYDataLayerConf
 import { layerTypes } from '../../common';
 import { isHorizontalChart } from './state_helpers';
 import { toExpression, toPreviewExpression, getSortedAccessors } from './to_expression';
-import { getAccessorColorConfig, getColorAssignments } from './color_assignment';
+import { getAccessorColorConfigs, getColorAssignments } from './color_assignment';
 import { getColumnToLabelMap } from './state_helpers';
 import {
   getGroupsAvailableInData,
@@ -699,7 +699,7 @@ const getMappedAccessors = ({
       { tables: frame.activeData },
       fieldFormats.deserialize
     );
-    mappedAccessors = getAccessorColorConfig(
+    mappedAccessors = getAccessorColorConfigs(
       colorAssignments,
       frame,
       {

x-pack/plugins/lens/public/xy_visualization/xy_config_panel/annotations_config_panel/annotations_panel.tsx

@@ -346,6 +346,7 @@ export const AnnotationsPanel = (
 
         <ColorPicker
           {...props}
+          overwriteColor={currentAnnotation?.color}
           defaultColor={isRange ? defaultAnnotationRangeColor : defaultAnnotationColor}
           showAlpha={isRange}
           setConfig={setAnnotations}

x-pack/plugins/lens/public/xy_visualization/xy_config_panel/color_picker.tsx

@@ -5,7 +5,7 @@
  * 2.0.
  */
 
-import React, { useEffect, useMemo, useState } from 'react';
+import React, { useEffect, useRef, useState } from 'react';
 import chroma from 'chroma-js';
 import { i18n } from '@kbn/i18n';
 import {
@@ -16,15 +16,7 @@ import {
   EuiIcon,
   euiPaletteColorBlind,
 } from '@elastic/eui';
-import type { PaletteRegistry } from '@kbn/coloring';
-import type { VisualizationDimensionEditorProps } from '../../types';
-import { State } from '../types';
-import { FormatFactory } from '../../../common';
-import { getSeriesColor } from '../state_helpers';
-import { getAccessorColorConfig, getColorAssignments } from '../color_assignment';
-import { getSortedAccessors } from '../to_expression';
 import { TooltipWrapper } from '../../shared_components';
-import { getDataLayers, isDataLayer } from '../visualization_helpers';
 
 const tooltipContent = {
   auto: i18n.translate('xpack.lens.configPanel.color.tooltip.auto', {
@@ -39,81 +31,72 @@ const tooltipContent = {
   }),
 };
 
+// copied from coloring package
+function isValidPonyfill(colorString: string) {
+  // we're using an old version of chroma without the valid function
+  try {
+    chroma(colorString);
+    return true;
+  } catch (e) {
+    return false;
+  }
+}
+
+export function isValidColor(colorString?: string) {
+  // chroma can handle also hex values with alpha channel/transparency
+  // chroma accepts also hex without #, so test for it
+  return (
+    colorString && colorString !== '' && /^#/.test(colorString) && isValidPonyfill(colorString)
+  );
+}
+
+const getColorAlpha = (color?: string | null) =>
+  (color && isValidColor(color) && chroma(color)?.alpha()) || 1;
+
 export const ColorPicker = ({
-  state,
-  layerId,
-  accessor,
-  frame,
-  formatFactory,
-  paletteService,
   label,
   disableHelpTooltip,
   disabled,
   setConfig,
-  showAlpha,
   defaultColor,
-}: VisualizationDimensionEditorProps<State> & {
-  formatFactory: FormatFactory;
-  paletteService: PaletteRegistry;
+  overwriteColor,
+  showAlpha,
+}: {
+  overwriteColor?: string | null;
+  defaultColor?: string | null;
+  setConfig: (config: { color?: string }) => void;
   label?: string;
   disableHelpTooltip?: boolean;
   disabled?: boolean;
-  setConfig: (config: { color?: string }) => void;
   showAlpha?: boolean;
-  defaultColor?: string;
 }) => {
-  const index = state.layers.findIndex((l) => l.layerId === layerId);
-  const layer = state.layers[index];
-
-  const overwriteColor = getSeriesColor(layer, accessor);
-  const currentColor = useMemo(() => {
-    if (overwriteColor || !frame.activeData) return overwriteColor;
-    if (defaultColor) {
-      return defaultColor;
-    }
-    if (isDataLayer(layer)) {
-      const sortedAccessors: string[] = getSortedAccessors(
-        frame.datasourceLayers[layer.layerId] ?? layer.accessors,
-        layer
-      );
-      const colorAssignments = getColorAssignments(
-        getDataLayers(state.layers),
-        { tables: frame.activeData ?? {} },
-        formatFactory
-      );
-      const mappedAccessors = getAccessorColorConfig(
-        colorAssignments,
-        frame,
-        {
-          ...layer,
-          accessors: sortedAccessors.filter((sorted) => layer.accessors.includes(sorted)),
-        },
-        paletteService
-      );
-      return mappedAccessors.find((a) => a.columnId === accessor)?.color || null;
-    }
-  }, [
-    overwriteColor,
-    frame,
-    paletteService,
-    state.layers,
-    accessor,
-    formatFactory,
-    layer,
-    defaultColor,
-  ]);
-
-  const [color, setColor] = useState(currentColor);
+  const [color, setColor] = useState(overwriteColor || defaultColor);
+  const [hexColor, setHexColor] = useState(overwriteColor || defaultColor);
+  const [currentColorAlpha, setCurrentColorAlpha] = useState(getColorAlpha(color));
+  const unflushedChanges = useRef(false);
 
   useEffect(() => {
-    setColor(currentColor);
-  }, [currentColor]);
+    //  only the changes from outside the color picker should be applied
+    if (!unflushedChanges.current) {
+      // something external changed the color that is currently selected (switching from annotation line to annotation range)
+      if (overwriteColor && hexColor && overwriteColor !== hexColor) {
+        setColor(overwriteColor || defaultColor);
+        setCurrentColorAlpha(getColorAlpha(overwriteColor));
+      }
+    }
+    unflushedChanges.current = false;
+  }, [hexColor, overwriteColor, defaultColor]);
 
   const handleColor: EuiColorPickerProps['onChange'] = (text, output) => {
     setColor(text);
-    if (output.isValid || text === '') {
-      const newColor = text === '' ? undefined : output.hex;
-      setConfig({ color: newColor });
+    unflushedChanges.current = true;
+    if (output.isValid) {
+      setHexColor(output.hex);
+      setCurrentColorAlpha(chroma(output.hex)?.alpha() || 1);
+      setConfig({ color: output.hex });
+    }
+    if (text === '') {
+      setConfig({ color: undefined });
     }
   };
 
@@ -123,8 +106,6 @@ export const ColorPicker = ({
       defaultMessage: 'Series color',
     });
 
-  const currentColorAlpha = color ? chroma(color).alpha() : 1;
-
   const colorPicker = (
     <EuiColorPicker
       fullWidth
@@ -132,11 +113,14 @@ export const ColorPicker = ({
       compressed
       isClearable={Boolean(overwriteColor)}
       onChange={handleColor}
-      color={disabled ? '' : color || currentColor}
+      color={disabled ? '' : color}
       disabled={disabled}
-      placeholder={i18n.translate('xpack.lens.xyChart.seriesColor.auto', {
-        defaultMessage: 'Auto',
-      })}
+      placeholder={
+        defaultColor?.toUpperCase() ||
+        i18n.translate('xpack.lens.xyChart.seriesColor.auto', {
+          defaultMessage: 'Auto',
+        })
+      }
       aria-label={inputLabel}
       showAlpha={showAlpha}
       swatches={
@@ -162,7 +146,6 @@ export const ColorPicker = ({
             {inputLabel}
             {!disableHelpTooltip && (
               <>
-                {''}
                 <EuiIcon
                   type="questionInCircle"
                   color="subdued"

x-pack/plugins/lens/public/xy_visualization/xy_config_panel/dimension_editor.tsx

@@ -5,7 +5,7 @@
  * 2.0.
  */
 
-import React, { useCallback } from 'react';
+import React, { useCallback, useMemo } from 'react';
 import { i18n } from '@kbn/i18n';
 import { EuiButtonGroup, EuiFormRow, htmlIdGenerator } from '@elastic/eui';
 import type { PaletteRegistry } from '@kbn/coloring';
@@ -13,13 +13,15 @@ import { YAxisMode, ExtendedYConfig } from '@kbn/expression-xy-plugin/common';
 import type { VisualizationDimensionEditorProps } from '../../types';
 import { State, XYState, XYDataLayerConfig } from '../types';
 import { FormatFactory } from '../../../common';
-import { isHorizontalChart } from '../state_helpers';
+import { getSeriesColor, isHorizontalChart } from '../state_helpers';
 import { ColorPicker } from './color_picker';
 import { PalettePicker, useDebouncedValue } from '../../shared_components';
-import { isAnnotationsLayer, isReferenceLayer } from '../visualization_helpers';
+import { getDataLayers, isAnnotationsLayer, isReferenceLayer } from '../visualization_helpers';
 import { ReferenceLinePanel } from './reference_line_config_panel';
 import { AnnotationsPanel } from './annotations_config_panel';
 import { CollapseSetting } from '../../shared_components/collapse_setting';
+import { getSortedAccessors } from '../to_expression';
+import { getColorAssignments, getAssignedColorConfig } from '../color_assignment';
 
 type UnwrapArray<T> = T extends Array<infer P> ? P : T;
 
@@ -44,6 +46,25 @@ export function DimensionEditor(
     formatFactory: FormatFactory;
     paletteService: PaletteRegistry;
   }
+) {
+  const { state, layerId } = props;
+  const index = state.layers.findIndex((l) => l.layerId === layerId);
+  const layer = state.layers[index];
+  if (isAnnotationsLayer(layer)) {
+    return <AnnotationsPanel {...props} />;
+  }
+
+  if (isReferenceLayer(layer)) {
+    return <ReferenceLinePanel {...props} />;
+  }
+  return <DataDimensionEditor {...props} />;
+}
+
+export function DataDimensionEditor(
+  props: VisualizationDimensionEditorProps<State> & {
+    formatFactory: FormatFactory;
+    paletteService: PaletteRegistry;
+  }
 ) {
   const { state, setState, layerId, accessor } = props;
   const index = state.layers.findIndex((l) => l.layerId === layerId);
@@ -79,13 +100,30 @@ export function DimensionEditor(
     [accessor, index, localState, layer, setLocalState]
   );
 
-  if (isAnnotationsLayer(layer)) {
-    return <AnnotationsPanel {...props} />;
-  }
+  const overwriteColor = getSeriesColor(layer, accessor);
+  const assignedColor = useMemo(() => {
+    const sortedAccessors: string[] = getSortedAccessors(
+      props.frame.datasourceLayers[layer.layerId] ?? layer.accessors,
+      layer
+    );
+    const colorAssignments = getColorAssignments(
+      getDataLayers(state.layers),
+      { tables: props.frame.activeData ?? {} },
+      props.formatFactory
+    );
 
-  if (isReferenceLayer(layer)) {
-    return <ReferenceLinePanel {...props} />;
-  }
+    return getAssignedColorConfig(
+      {
+        ...layer,
+        accessors: sortedAccessors.filter((sorted) => layer.accessors.includes(sorted)),
+      },
+      accessor,
+      colorAssignments,
+      props.frame,
+
+      props.paletteService
+    ).color;
+  }, [props.frame, props.paletteService, state.layers, accessor, props.formatFactory, layer]);
 
   const localLayer: XYDataLayerConfig = layer;
   if (props.groupId === 'breakdown') {
@@ -114,6 +152,8 @@ export function DimensionEditor(
     <>
       <ColorPicker
         {...props}
+        overwriteColor={overwriteColor}
+        defaultColor={assignedColor}
         disabled={Boolean(!localLayer.collapseFn && localLayer.splitAccessor)}
         setConfig={setConfig}
       />

x-pack/plugins/lens/public/xy_visualization/xy_config_panel/reference_line_config_panel/reference_line_panel.tsx

@@ -94,6 +94,7 @@ export const ReferenceLinePanel = (
       <FillSetting isHorizontal={isHorizontal} setConfig={setConfig} currentConfig={localConfig} />
       <ColorPicker
         {...props}
+        overwriteColor={localConfig?.color}
         defaultColor={defaultReferenceLineColor}
         setConfig={setConfig}
         disableHelpTooltip

x-pack/plugins/lens/public/xy_visualization/xy_config_panel/xy_config_panel.test.tsx

@@ -18,6 +18,16 @@ import { createMockFramePublicAPI, createMockDatasource } from '../../mocks';
 import { chartPluginMock } from '@kbn/charts-plugin/public/mocks';
 import { EuiColorPicker } from '@elastic/eui';
 import { layerTypes } from '../../../common';
+import { act } from 'react-dom/test-utils';
+
+jest.mock('lodash', () => {
+  const original = jest.requireActual('lodash');
+
+  return {
+    ...original,
+    debounce: (fn: unknown) => fn,
+  };
+});
 
 describe('XY Config panels', () => {
   let frame: FramePublicAPI;
@@ -343,5 +353,67 @@ describe('XY Config panels', () => {
 
       expect(component.find(EuiColorPicker).prop('color')).toEqual('red');
     });
+    test('does not apply incorrect color', () => {
+      const setState = jest.fn();
+      const state = {
+        ...testState(),
+        layers: [
+          {
+            seriesType: 'bar',
+            layerType: layerTypes.DATA,
+            layerId: 'first',
+            splitAccessor: undefined,
+            xAccessor: 'foo',
+            accessors: ['bar'],
+            yConfig: [{ forAccessor: 'bar', color: 'red' }],
+          },
+        ],
+      } as XYState;
+
+      const component = mount(
+        <DimensionEditor
+          layerId={state.layers[0].layerId}
+          frame={{
+            ...frame,
+            activeData: {
+              first: {
+                type: 'datatable',
+                columns: [],
+                rows: [{ bar: 123 }],
+              },
+            },
+          }}
+          setState={setState}
+          accessor="bar"
+          groupId="left"
+          state={state}
+          formatFactory={jest.fn()}
+          paletteService={chartPluginMock.createPaletteRegistry()}
+          panelRef={React.createRef()}
+        />
+      );
+
+      act(() => {
+        component
+          .find('input[data-test-subj="euiColorPickerAnchor indexPattern-dimension-colorPicker"]')
+          .simulate('change', {
+            target: { value: 'INCORRECT_COLOR' },
+          });
+      });
+      component.update();
+      expect(component.find(EuiColorPicker).prop('color')).toEqual('INCORRECT_COLOR');
+      expect(setState).not.toHaveBeenCalled();
+
+      act(() => {
+        component
+          .find('input[data-test-subj="euiColorPickerAnchor indexPattern-dimension-colorPicker"]')
+          .simulate('change', {
+            target: { value: '666666' },
+          });
+      });
+      component.update();
+      expect(component.find(EuiColorPicker).prop('color')).toEqual('666666');
+      expect(setState).toHaveBeenCalled();
+    });
   });
 });