From 02ffb52f4a2ef95ab35b57dd8600ca450e05b7ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Pedro=20Alves=20Barbosa?= <93401463+joaopedroab@users.noreply.github.com> Date: Sun, 22 Mar 2026 18:59:15 -0300 Subject: [PATCH] fix(table): improve conditional formatting text contrast (#38705) --- superset-frontend/package-lock.json | 3 +- .../superset-ui-chart-controls/package.json | 3 +- .../superset-ui-chart-controls/src/types.ts | 5 + .../src/utils/getColorFormatters.ts | 63 ++ .../test/utils/getColorFormatters.test.ts | 54 ++ .../src/styles/index.tsx | 11 + .../src/utils/getCellStyle.ts | 33 +- .../src/utils/useColDefs.ts | 26 +- .../test/utils/useColDefs.test.ts | 743 +++++++++++++++++- .../src/PivotTableChart.tsx | 6 + .../src/react-pivottable/TableRenderers.tsx | 78 +- .../react-pivottable/tableRenders.test.tsx | 505 +++++++++++- .../plugin-chart-table/src/TableChart.tsx | 17 +- .../test/TableChart.test.tsx | 233 +++++- 14 files changed, 1698 insertions(+), 82 deletions(-) diff --git a/superset-frontend/package-lock.json b/superset-frontend/package-lock.json index c6c51ac052d..03b2084934c 100644 --- a/superset-frontend/package-lock.json +++ b/superset-frontend/package-lock.json @@ -51108,7 +51108,8 @@ "dependencies": { "@apache-superset/core": "*", "@types/react": "*", - "lodash": "^4.17.23" + "lodash": "^4.17.23", + "tinycolor2": "*" }, "peerDependencies": { "@ant-design/icons": "^5.2.6", diff --git a/superset-frontend/packages/superset-ui-chart-controls/package.json b/superset-frontend/packages/superset-ui-chart-controls/package.json index a9eb07107a1..69c89099a05 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/package.json +++ b/superset-frontend/packages/superset-ui-chart-controls/package.json @@ -26,7 +26,8 @@ "dependencies": { "@apache-superset/core": "*", "@types/react": "*", - "lodash": "^4.17.23" + "lodash": "^4.17.23", + "tinycolor2": "*" }, "peerDependencies": { "@ant-design/icons": "^5.2.6", diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/types.ts b/superset-frontend/packages/superset-ui-chart-controls/src/types.ts index 1cc9743f4cf..6344222d594 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/types.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/src/types.ts @@ -507,6 +507,11 @@ export type ColorFormatters = { ) => string | undefined; }[]; +export type ResolvedColorFormatterResult = { + backgroundColor?: string; + color?: string; +}; + export default {}; export function isColumnMeta(column: AnyDict): column is ColumnMeta { diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts b/superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts index 5869472de9b..10b5c996d6f 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts @@ -20,11 +20,13 @@ import memoizeOne from 'memoize-one'; import { isString, isBoolean } from 'lodash'; import { isBlank } from '@apache-superset/core/utils'; import { addAlpha, DataRecord } from '@superset-ui/core'; +import tinycolor from 'tinycolor2'; import { ColorFormatters, Comparator, ConditionalFormattingConfig, MultipleValueComparators, + ResolvedColorFormatterResult, } from '../types'; export const round = (num: number, precision = 0) => @@ -33,6 +35,11 @@ export const round = (num: number, precision = 0) => const MIN_OPACITY_BOUNDED = 0.05; const MIN_OPACITY_UNBOUNDED = 0; const MAX_OPACITY = 1; +const READABLE_TEXT_COLORS = [ + { r: 0, g: 0, b: 0 }, + { r: 255, g: 255, b: 255 }, +]; + export const getOpacity = ( value: number | string | boolean | null, cutoffPoint: number | string, @@ -325,3 +332,59 @@ export const getColorFormatters = memoizeOne( [], ) ?? [], ); + +export const getReadableTextColor = ( + backgroundColor: string | undefined, + surfaceColor: string, +): string | undefined => { + if (!backgroundColor) { + return undefined; + } + + const background = tinycolor(backgroundColor); + const surface = tinycolor(surfaceColor); + + if (!background.isValid() || !surface.isValid()) { + return undefined; + } + + const { r: bgR, g: bgG, b: bgB, a: bgAlpha } = background.toRgb(); + const { r: surfaceR, g: surfaceG, b: surfaceB } = surface.toRgb(); + const alpha = bgAlpha ?? 1; + + const compositeColor = tinycolor({ + r: bgR * alpha + surfaceR * (1 - alpha), + g: bgG * alpha + surfaceG * (1 - alpha), + b: bgB * alpha + surfaceB * (1 - alpha), + }); + + return tinycolor + .mostReadable(compositeColor, READABLE_TEXT_COLORS, { + includeFallbackColors: true, + level: 'AA', + size: 'small', + }) + .toRgbString(); +}; + +export const getNormalizedTextColor = ( + color: string | undefined, +): string | undefined => { + if (!color) { + return undefined; + } + + const parsedColor = tinycolor(color); + if (!parsedColor.isValid()) { + return color; + } + + return parsedColor.setAlpha(1).toRgbString(); +}; + +export const getTextColorForBackground = ( + result: ResolvedColorFormatterResult, + surfaceColor: string, +): string | undefined => + getNormalizedTextColor(result.color) ?? + getReadableTextColor(result.backgroundColor, surfaceColor); diff --git a/superset-frontend/packages/superset-ui-chart-controls/test/utils/getColorFormatters.test.ts b/superset-frontend/packages/superset-ui-chart-controls/test/utils/getColorFormatters.test.ts index 1ed3f031d70..7b437f68f63 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/test/utils/getColorFormatters.test.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/test/utils/getColorFormatters.test.ts @@ -24,6 +24,11 @@ import { getColorFormatters, getColorFunction, } from '../../src'; +import { + getReadableTextColor, + getNormalizedTextColor, + getTextColorForBackground, +} from '../../src/utils/getColorFormatters'; configure(); const mockData = [ @@ -107,6 +112,55 @@ test('getColorFunction LESS_THAN', () => { expect(colorFunction(50)).toEqual('#FF0000FF'); }); +test('getReadableTextColor returns white for dark backgrounds', () => { + expect(getReadableTextColor('#111111', '#ffffff')).toBe('rgb(255, 255, 255)'); +}); + +test('getReadableTextColor returns black for light backgrounds', () => { + expect(getReadableTextColor('#f5f5f5', '#ffffff')).toBe('rgb(0, 0, 0)'); +}); + +test('getReadableTextColor blends alpha over the provided surface', () => { + expect(getReadableTextColor('rgba(0, 0, 0, 0.6)', '#ffffff')).toBe( + 'rgb(255, 255, 255)', + ); + expect(getReadableTextColor('rgba(255, 255, 255, 0.6)', '#000000')).toBe( + 'rgb(0, 0, 0)', + ); +}); + +test('getTextColorForBackground prefers explicit text color', () => { + expect( + getTextColorForBackground( + { backgroundColor: '#111111', color: '#ace1c4ff' }, + '#ffffff', + ), + ).toBe('rgb(172, 225, 196)'); +}); + +test('getNormalizedTextColor removes alpha from explicit text colors', () => { + expect(getNormalizedTextColor('#ace1c40d')).toBe('rgb(172, 225, 196)'); + expect(getNormalizedTextColor('rgba(172, 225, 196, 0.2)')).toBe( + 'rgb(172, 225, 196)', + ); +}); + +test('getTextColorForBackground normalizes explicit text color alpha', () => { + expect( + getTextColorForBackground( + { backgroundColor: '#111111', color: '#ace1c40d' }, + '#ffffff', + ), + ).toBe('rgb(172, 225, 196)'); +}); + +test('getTextColorForBackground falls back to adaptive contrast', () => { + expect( + getTextColorForBackground({ backgroundColor: '#111111' }, '#ffffff'), + ).toBe('rgb(255, 255, 255)'); + expect(getTextColorForBackground({}, '#ffffff')).toBeUndefined(); +}); + test('getColorFunction GREATER_OR_EQUAL', () => { const colorFunction = getColorFunction( { diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/styles/index.tsx b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/styles/index.tsx index 62e1fc35b79..7e5131bdf17 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/styles/index.tsx +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/styles/index.tsx @@ -309,6 +309,17 @@ export const StyledChartContainer = styled.div<{ height: auto; } + .ag-cell { + color: var(--ag-cell-value-color, inherit); + } + + .ag-row-hover .ag-cell { + color: var( + --ag-cell-value-hover-color, + var(--ag-cell-value-color, inherit) + ); + } + .ag-container { border-radius: 0px; border: var(--ag-wrapper-border); diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellStyle.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellStyle.ts index 1aab42b836f..ac5eb26b54a 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellStyle.ts +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellStyle.ts @@ -17,7 +17,11 @@ * under the License. */ -import { ColorFormatters } from '@superset-ui/chart-controls'; +import { + ColorFormatters, + getTextColorForBackground, + ObjectFormattingEnum, +} from '@superset-ui/chart-controls'; import { CellClassParams } from '@superset-ui/core/components/ThemedAgGridReact'; import { BasicColorFormatterType, InputColumn } from '../types'; @@ -29,6 +33,8 @@ type CellStyleParams = CellClassParams & { [Key: string]: BasicColorFormatterType; }[]; col: InputColumn; + cellSurfaceColor: string; + hoverCellSurfaceColor: string; }; const getCellStyle = (params: CellStyleParams) => { @@ -42,8 +48,11 @@ const getCellStyle = (params: CellStyleParams) => { columnColorFormatters, col, node, + cellSurfaceColor, + hoverCellSurfaceColor, } = params; let backgroundColor; + let color; if (hasColumnColorFormatters) { columnColorFormatters! .filter(formatter => { @@ -56,7 +65,16 @@ const getCellStyle = (params: CellStyleParams) => { const formatterResult = value || value === 0 ? formatter.getColorFromValue(value) : false; if (formatterResult) { - backgroundColor = formatterResult; + if ( + formatter.objectFormatting === ObjectFormattingEnum.TEXT_COLOR || + formatter.toTextColor + ) { + color = formatterResult; + } else if ( + formatter.objectFormatting !== ObjectFormattingEnum.CELL_BAR + ) { + backgroundColor = formatterResult; + } } }); } @@ -72,9 +90,20 @@ const getCellStyle = (params: CellStyleParams) => { const textAlign = col?.config?.horizontalAlign || (col?.isNumeric ? 'right' : 'left'); + const resolvedTextColor = getTextColorForBackground( + { backgroundColor, color }, + cellSurfaceColor, + ); + const hoverResolvedTextColor = getTextColorForBackground( + { backgroundColor, color }, + hoverCellSurfaceColor, + ); return { backgroundColor: backgroundColor || '', + color: '', + '--ag-cell-value-color': resolvedTextColor || '', + '--ag-cell-value-hover-color': hoverResolvedTextColor || '', textAlign, }; }; diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/useColDefs.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/useColDefs.ts index 84606c5013f..48f713aabbe 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/useColDefs.ts +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/useColDefs.ts @@ -21,6 +21,7 @@ import { ColDef } from '@superset-ui/core/components/ThemedAgGridReact'; import { useCallback, useMemo } from 'react'; import { DataRecord, DataRecordValue } from '@superset-ui/core'; import { GenericDataType } from '@apache-superset/core/common'; +import { useTheme } from '@apache-superset/core/theme'; import { ColorFormatters } from '@superset-ui/chart-controls'; import { extent as d3Extent, max as d3Max } from 'd3-array'; import { @@ -225,6 +226,7 @@ export const useColDefs = ({ alignPositiveNegative, slice_id, }: UseColDefsProps) => { + const theme = useTheme(); const getCommonColProps = useCallback( ( col: InputColumn, @@ -280,15 +282,30 @@ export const useColDefs = ({ headerName: getHeaderLabel(col), valueFormatter: p => valueFormatter(p, col), valueGetter: p => valueGetter(p, col), - cellStyle: p => - getCellStyle({ + cellStyle: p => { + const cellSurfaceColor = + p.node?.rowPinned != null + ? theme.colorBgBase + : p.rowIndex % 2 === 0 + ? theme.colorBgBase + : theme.colorFillQuaternary; + const hoverCellSurfaceColor = + p.node?.rowPinned != null + ? cellSurfaceColor + : theme.colorFillSecondary; + const cellStyleParams = { ...p, hasColumnColorFormatters, columnColorFormatters, hasBasicColorFormatters, basicColorFormatters, col, - }), + cellSurfaceColor, + hoverCellSurfaceColor, + } as Parameters[0]; + + return getCellStyle(cellStyleParams); + }, cellClass: p => getCellClass({ ...p, @@ -385,6 +402,9 @@ export const useColDefs = ({ allowRearrangeColumns, serverPagination, alignPositiveNegative, + theme.colorBgBase, + theme.colorFillSecondary, + theme.colorFillQuaternary, ], ); diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/useColDefs.test.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/useColDefs.test.ts index 9f2a44a82d0..40b4863a3ae 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/useColDefs.test.ts +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/useColDefs.test.ts @@ -18,9 +18,19 @@ */ import { renderHook } from '@testing-library/react-hooks'; import { GenericDataType } from '@apache-superset/core/common'; +import { + supersetTheme, + ThemeProvider, + type SupersetTheme, +} from '@apache-superset/core/theme'; +import { ObjectFormattingEnum } from '@superset-ui/chart-controls'; +import tinycolor from 'tinycolor2'; +import { createElement, type ComponentProps, ReactNode } from 'react'; import { useColDefs } from '../../src/utils/useColDefs'; import { InputColumn } from '../../src/types'; +type TestCellStyleFunc = (params: unknown) => unknown; + function makeColumn(overrides: Partial = {}): InputColumn { return { key: 'test_col', @@ -34,6 +44,81 @@ function makeColumn(overrides: Partial = {}): InputColumn { }; } +function getCellStyleFunction(cellStyle: unknown): TestCellStyleFunc { + expect(typeof cellStyle).toBe('function'); + return cellStyle as TestCellStyleFunc; +} + +function getCellStyleResult( + cellStyle: TestCellStyleFunc, + overrides: Record = {}, +) { + return cellStyle({ + value: 42, + colDef: { field: 'count' }, + rowIndex: 0, + node: {}, + ...overrides, + } as never); +} + +function getExpectedTextColor( + result: { backgroundColor?: string; color?: string }, + surfaceColor: string, +) { + if (result.color) { + const parsedColor = tinycolor(result.color); + return parsedColor.isValid() + ? parsedColor.setAlpha(1).toRgbString() + : result.color; + } + + if (!result.backgroundColor) { + return undefined; + } + + const background = tinycolor(result.backgroundColor); + const surface = tinycolor(surfaceColor); + if (!background.isValid() || !surface.isValid()) { + return undefined; + } + + const { r: bgR, g: bgG, b: bgB, a: bgAlpha } = background.toRgb(); + const { r: surfaceR, g: surfaceG, b: surfaceB } = surface.toRgb(); + const alpha = bgAlpha ?? 1; + + return tinycolor + .mostReadable( + tinycolor({ + r: bgR * alpha + surfaceR * (1 - alpha), + g: bgG * alpha + surfaceG * (1 - alpha), + b: bgB * alpha + surfaceB * (1 - alpha), + }), + [ + { r: 0, g: 0, b: 0 }, + { r: 255, g: 255, b: 255 }, + ], + { + includeFallbackColors: true, + level: 'AA', + size: 'small', + }, + ) + .toRgbString(); +} + +function makeThemeWrapper(theme: SupersetTheme) { + return function ThemeWrapper({ children }: { children?: ReactNode }) { + return createElement( + ThemeProvider, + { theme } as ComponentProps, + children, + ); + }; +} + +const defaultThemeWrapper = makeThemeWrapper(supersetTheme); + const defaultProps = { data: [{ test_col: 'value' }], serverPagination: false, @@ -58,12 +143,14 @@ test('boolean columns use agCheckboxCellRenderer', () => { dataType: GenericDataType.Boolean, }); - const { result } = renderHook(() => - useColDefs({ - ...defaultProps, - columns: [booleanCol], - data: [{ is_active: true }, { is_active: false }], - }), + const { result } = renderHook( + () => + useColDefs({ + ...defaultProps, + columns: [booleanCol], + data: [{ is_active: true }, { is_active: false }], + }), + { wrapper: defaultThemeWrapper }, ); const colDef = result.current[0]; @@ -79,12 +166,14 @@ test('string columns use custom TextCellRenderer', () => { dataType: GenericDataType.String, }); - const { result } = renderHook(() => - useColDefs({ - ...defaultProps, - columns: [stringCol], - data: [{ name: 'Alice' }], - }), + const { result } = renderHook( + () => + useColDefs({ + ...defaultProps, + columns: [stringCol], + data: [{ name: 'Alice' }], + }), + { wrapper: defaultThemeWrapper }, ); const colDef = result.current[0]; @@ -101,12 +190,14 @@ test('numeric columns use custom NumericCellRenderer', () => { isMetric: true, }); - const { result } = renderHook(() => - useColDefs({ - ...defaultProps, - columns: [numericCol], - data: [{ count: 42 }], - }), + const { result } = renderHook( + () => + useColDefs({ + ...defaultProps, + columns: [numericCol], + data: [{ count: 42 }], + }), + { wrapper: defaultThemeWrapper }, ); const colDef = result.current[0]; @@ -121,15 +212,619 @@ test('temporal columns use custom TextCellRenderer', () => { dataType: GenericDataType.Temporal, }); - const { result } = renderHook(() => - useColDefs({ - ...defaultProps, - columns: [temporalCol], - data: [{ created_at: '2024-01-01' }], - }), + const { result } = renderHook( + () => + useColDefs({ + ...defaultProps, + columns: [temporalCol], + data: [{ created_at: '2024-01-01' }], + }), + { wrapper: defaultThemeWrapper }, ); const colDef = result.current[0]; expect(colDef.cellRenderer).toBeInstanceOf(Function); expect(colDef.cellDataType).toBe('date'); }); + +test('cellStyle derives readable text color from dark background formatting', () => { + const numericCol = makeColumn({ + key: 'count', + label: 'Count', + dataType: GenericDataType.Numeric, + isNumeric: true, + isMetric: true, + }); + + const { result } = renderHook( + () => + useColDefs({ + ...defaultProps, + columns: [numericCol], + data: [{ count: 42 }], + columnColorFormatters: [ + { + column: 'count', + objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR, + getColorFromValue: (value: unknown) => + value === 42 ? '#111111' : undefined, + }, + ], + }), + { wrapper: defaultThemeWrapper }, + ); + + const colDef = result.current[0]; + const cellStyle = getCellStyleFunction(colDef.cellStyle); + expect( + cellStyle({ + value: 42, + colDef: { field: 'count' }, + rowIndex: 0, + node: {}, + } as never), + ).toMatchObject({ + backgroundColor: '#111111', + color: '', + '--ag-cell-value-color': 'rgb(255, 255, 255)', + textAlign: 'right', + }); +}); + +test('cellStyle keeps explicit text color over adaptive contrast', () => { + const numericCol = makeColumn({ + key: 'count', + label: 'Count', + dataType: GenericDataType.Numeric, + isNumeric: true, + isMetric: true, + }); + + const { result } = renderHook( + () => + useColDefs({ + ...defaultProps, + columns: [numericCol], + data: [{ count: 42 }], + columnColorFormatters: [ + { + column: 'count', + objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR, + getColorFromValue: (value: unknown) => + value === 42 ? '#111111' : undefined, + }, + { + column: 'count', + objectFormatting: ObjectFormattingEnum.TEXT_COLOR, + getColorFromValue: (value: unknown) => + value === 42 ? '#ace1c40d' : undefined, + }, + ], + }), + { wrapper: defaultThemeWrapper }, + ); + + const colDef = result.current[0]; + const cellStyle = getCellStyleFunction(colDef.cellStyle); + expect( + cellStyle({ + value: 42, + colDef: { field: 'count' }, + rowIndex: 0, + node: {}, + } as never), + ).toMatchObject({ + backgroundColor: '#111111', + color: '', + '--ag-cell-value-color': 'rgb(172, 225, 196)', + textAlign: 'right', + }); +}); + +test('cellStyle treats legacy toTextColor formatters as text color', () => { + const numericCol = makeColumn({ + key: 'count', + label: 'Count', + dataType: GenericDataType.Numeric, + isNumeric: true, + isMetric: true, + }); + + const { result } = renderHook( + () => + useColDefs({ + ...defaultProps, + columns: [numericCol], + data: [{ count: 42 }], + columnColorFormatters: [ + { + column: 'count', + objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR, + getColorFromValue: (value: unknown) => + value === 42 ? '#111111' : undefined, + }, + { + column: 'count', + toTextColor: true, + getColorFromValue: (value: unknown) => + value === 42 ? '#ace1c40d' : undefined, + }, + ], + }), + { wrapper: defaultThemeWrapper }, + ); + + const colDef = result.current[0]; + const cellStyle = getCellStyleFunction(colDef.cellStyle); + expect(getCellStyleResult(cellStyle)).toMatchObject({ + backgroundColor: '#111111', + color: '', + '--ag-cell-value-color': 'rgb(172, 225, 196)', + textAlign: 'right', + }); +}); + +test('cellStyle uses caller-provided surface color for adaptive contrast', () => { + const numericCol = makeColumn({ + key: 'count', + label: 'Count', + dataType: GenericDataType.Numeric, + isNumeric: true, + isMetric: true, + }); + const backgroundColor = 'rgba(0, 0, 0, 0.4)'; + + const { result: lightResult } = renderHook( + () => + useColDefs({ + ...defaultProps, + columns: [numericCol], + data: [{ count: 42 }], + columnColorFormatters: [ + { + column: 'count', + objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR, + getColorFromValue: (value: unknown) => + value === 42 ? backgroundColor : undefined, + }, + ], + }), + { + wrapper: makeThemeWrapper({ + ...supersetTheme, + colorBgBase: '#ffffff', + }), + }, + ); + + const { result: darkResult } = renderHook( + () => + useColDefs({ + ...defaultProps, + columns: [numericCol], + data: [{ count: 42 }], + columnColorFormatters: [ + { + column: 'count', + objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR, + getColorFromValue: (value: unknown) => + value === 42 ? backgroundColor : undefined, + }, + ], + }), + { + wrapper: makeThemeWrapper({ + ...supersetTheme, + colorBgBase: '#000000', + }), + }, + ); + + const lightCellStyle = getCellStyleFunction(lightResult.current[0].cellStyle); + const darkCellStyle = getCellStyleFunction(darkResult.current[0].cellStyle); + + expect(getCellStyleResult(lightCellStyle)).toMatchObject({ + backgroundColor, + color: '', + '--ag-cell-value-color': getExpectedTextColor( + { backgroundColor }, + '#ffffff', + ), + }); + expect(getCellStyleResult(darkCellStyle)).toMatchObject({ + backgroundColor, + color: '', + '--ag-cell-value-color': getExpectedTextColor( + { backgroundColor }, + '#000000', + ), + }); + expect(getCellStyleResult(lightCellStyle)).not.toEqual( + getCellStyleResult(darkCellStyle), + ); +}); + +test('cellStyle uses striped odd-row surface for adaptive contrast', () => { + const numericCol = makeColumn({ + key: 'count', + label: 'Count', + dataType: GenericDataType.Numeric, + isNumeric: true, + isMetric: true, + }); + const backgroundColor = 'rgba(0, 0, 0, 0.4)'; + + const { result } = renderHook( + () => + useColDefs({ + ...defaultProps, + columns: [numericCol], + data: [{ count: 42 }, { count: 43 }], + columnColorFormatters: [ + { + column: 'count', + objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR, + getColorFromValue: (value: unknown) => + typeof value === 'number' ? backgroundColor : undefined, + }, + ], + }), + { + wrapper: makeThemeWrapper({ + ...supersetTheme, + colorBgBase: '#ffffff', + colorFillQuaternary: '#000000', + }), + }, + ); + + const cellStyle = getCellStyleFunction(result.current[0].cellStyle); + + expect( + getCellStyleResult(cellStyle, { + rowIndex: 0, + }), + ).toMatchObject({ + backgroundColor, + color: '', + '--ag-cell-value-color': getExpectedTextColor( + { backgroundColor }, + '#ffffff', + ), + }); + expect( + getCellStyleResult(cellStyle, { + rowIndex: 1, + }), + ).toMatchObject({ + backgroundColor, + color: '', + '--ag-cell-value-color': getExpectedTextColor( + { backgroundColor }, + '#000000', + ), + }); +}); + +test('cellStyle exposes hover-specific adaptive contrast for formatted cells', () => { + const numericCol = makeColumn({ + key: 'count', + label: 'Count', + dataType: GenericDataType.Numeric, + isNumeric: true, + isMetric: true, + }); + const backgroundColor = 'rgba(0, 0, 0, 0.4)'; + + const { result } = renderHook( + () => + useColDefs({ + ...defaultProps, + columns: [numericCol], + data: [{ count: 42 }], + columnColorFormatters: [ + { + column: 'count', + objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR, + getColorFromValue: (value: unknown) => + value === 42 ? backgroundColor : undefined, + }, + ], + }), + { + wrapper: makeThemeWrapper({ + ...supersetTheme, + colorBgBase: '#ffffff', + colorFillSecondary: '#000000', + }), + }, + ); + + const cellStyle = getCellStyleFunction(result.current[0].cellStyle); + expect(getCellStyleResult(cellStyle)).toMatchObject({ + backgroundColor, + color: '', + '--ag-cell-value-color': getExpectedTextColor( + { backgroundColor }, + '#ffffff', + ), + '--ag-cell-value-hover-color': getExpectedTextColor( + { backgroundColor }, + '#000000', + ), + }); +}); + +test('cellStyle resets inline text color variables when no formatter matches', () => { + const numericCol = makeColumn({ + key: 'count', + label: 'Count', + dataType: GenericDataType.Numeric, + isNumeric: true, + isMetric: true, + }); + + const { result } = renderHook( + () => + useColDefs({ + ...defaultProps, + columns: [numericCol], + data: [{ count: 42 }], + columnColorFormatters: [ + { + column: 'count', + objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR, + getColorFromValue: () => undefined, + }, + ], + }), + { + wrapper: makeThemeWrapper({ + ...supersetTheme, + colorPrimaryText: '#123456', + }), + }, + ); + + const cellStyle = getCellStyleFunction(result.current[0].cellStyle); + const cellStyleResult = getCellStyleResult(cellStyle) as { + backgroundColor: string; + color?: string; + textAlign: string; + }; + expect(cellStyleResult).toMatchObject({ + backgroundColor: '', + color: '', + '--ag-cell-value-color': '', + '--ag-cell-value-hover-color': '', + textAlign: 'right', + }); +}); + +test('cellStyle preserves invalid explicit text color', () => { + const numericCol = makeColumn({ + key: 'count', + label: 'Count', + dataType: GenericDataType.Numeric, + isNumeric: true, + isMetric: true, + }); + + const { result } = renderHook( + () => + useColDefs({ + ...defaultProps, + columns: [numericCol], + data: [{ count: 42 }], + columnColorFormatters: [ + { + column: 'count', + objectFormatting: ObjectFormattingEnum.TEXT_COLOR, + getColorFromValue: (value: unknown) => + value === 42 ? 'not-a-color' : undefined, + }, + ], + }), + { wrapper: defaultThemeWrapper }, + ); + + const cellStyle = getCellStyleFunction(result.current[0].cellStyle); + expect(getCellStyleResult(cellStyle)).toMatchObject({ + backgroundColor: '', + color: '', + '--ag-cell-value-color': 'not-a-color', + '--ag-cell-value-hover-color': 'not-a-color', + }); +}); + +test('cellStyle ignores cell-bar formatters for text and background resolution', () => { + const numericCol = makeColumn({ + key: 'count', + label: 'Count', + dataType: GenericDataType.Numeric, + isNumeric: true, + isMetric: true, + }); + + const { result } = renderHook( + () => + useColDefs({ + ...defaultProps, + columns: [numericCol], + data: [{ count: 42 }], + columnColorFormatters: [ + { + column: 'count', + objectFormatting: ObjectFormattingEnum.CELL_BAR, + getColorFromValue: (value: unknown) => + value === 42 ? '#11111199' : undefined, + }, + ], + }), + { + wrapper: makeThemeWrapper({ + ...supersetTheme, + colorPrimaryText: '#654321', + }), + }, + ); + + const cellStyle = getCellStyleFunction(result.current[0].cellStyle); + const cellStyleResult = getCellStyleResult(cellStyle) as { + backgroundColor: string; + color?: string; + }; + expect(cellStyleResult).toMatchObject({ + backgroundColor: '', + color: '', + '--ag-cell-value-color': '', + '--ag-cell-value-hover-color': '', + }); +}); + +test('cellStyle lets basic color formatters override column formatter background', () => { + const numericCol = makeColumn({ + key: 'count', + label: 'Count', + dataType: GenericDataType.Numeric, + isNumeric: true, + isMetric: true, + metricName: 'sum__count', + }); + + const { result } = renderHook( + () => + useColDefs({ + ...defaultProps, + columns: [numericCol], + data: [{ count: 42 }], + isUsingTimeComparison: true, + columnColorFormatters: [ + { + column: 'count', + objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR, + getColorFromValue: (value: unknown) => + value === 42 ? '#111111' : undefined, + }, + ], + basicColorFormatters: [ + { + sum__count: { + backgroundColor: '#abcdef', + arrowColor: 'Green', + mainArrow: 'up', + }, + }, + ], + }), + { wrapper: defaultThemeWrapper }, + ); + + const cellStyle = getCellStyleFunction(result.current[0].cellStyle); + expect(getCellStyleResult(cellStyle)).toMatchObject({ + backgroundColor: '#abcdef', + color: '', + '--ag-cell-value-color': getExpectedTextColor( + { backgroundColor: '#abcdef' }, + '#ffffff', + ), + }); +}); + +test('cellStyle ignores basic color formatters for pinned bottom rows', () => { + const numericCol = makeColumn({ + key: 'count', + label: 'Count', + dataType: GenericDataType.Numeric, + isNumeric: true, + isMetric: true, + metricName: 'sum__count', + }); + + const { result } = renderHook( + () => + useColDefs({ + ...defaultProps, + columns: [numericCol], + data: [{ count: 42 }], + isUsingTimeComparison: true, + basicColorFormatters: [ + { + sum__count: { + backgroundColor: '#abcdef', + arrowColor: 'Green', + mainArrow: 'up', + }, + }, + ], + }), + { wrapper: defaultThemeWrapper }, + ); + + const cellStyle = getCellStyleFunction(result.current[0].cellStyle); + expect( + getCellStyleResult(cellStyle, { + node: { rowPinned: 'bottom' }, + }), + ).toMatchObject({ + backgroundColor: '', + }); +}); + +test('cellStyle defaults non-numeric columns to left alignment', () => { + const stringCol = makeColumn({ + key: 'name', + label: 'Name', + dataType: GenericDataType.String, + }); + + const { result } = renderHook( + () => + useColDefs({ + ...defaultProps, + columns: [stringCol], + data: [{ name: 'Alice' }], + }), + { wrapper: defaultThemeWrapper }, + ); + + const cellStyle = getCellStyleFunction(result.current[0].cellStyle); + expect( + cellStyle({ + value: 'Alice', + colDef: { field: 'name' }, + rowIndex: 0, + node: {}, + } as never), + ).toMatchObject({ + textAlign: 'left', + }); +}); + +test('cellStyle respects explicit horizontal alignment overrides', () => { + const numericCol = makeColumn({ + key: 'count', + label: 'Count', + dataType: GenericDataType.Numeric, + isNumeric: true, + isMetric: true, + config: { + horizontalAlign: 'center', + }, + }); + + const { result } = renderHook( + () => + useColDefs({ + ...defaultProps, + columns: [numericCol], + data: [{ count: 42 }], + }), + { wrapper: defaultThemeWrapper }, + ); + + const cellStyle = getCellStyleFunction(result.current[0].cellStyle); + expect(getCellStyleResult(cellStyle)).toMatchObject({ + textAlign: 'center', + }); +}); diff --git a/superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx b/superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx index 425c3598117..ef1dbb9a998 100644 --- a/superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx +++ b/superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx @@ -576,6 +576,9 @@ export default function PivotTableChart(props: PivotTableProps) { omittedHighlightHeaderGroups: [METRIC_KEY], cellColorFormatters: { [METRIC_KEY]: metricColorFormatters }, dateFormatters, + cellBackgroundColor: theme.colorBgBase, + cellTextColor: theme.colorPrimaryText, + activeHeaderBackgroundColor: theme.colorPrimaryBg, }), [ colTotals, @@ -586,6 +589,9 @@ export default function PivotTableChart(props: PivotTableProps) { rowTotals, rowSubTotals, selectedFilters, + theme.colorBgBase, + theme.colorPrimaryBg, + theme.colorPrimaryText, toggleFilter, ], ); diff --git a/superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx b/superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx index cc1d8fbe2df..cbf7e1619df 100644 --- a/superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx +++ b/superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx @@ -20,18 +20,20 @@ import { Component, ReactNode, MouseEvent } from 'react'; import { safeHtmlSpan } from '@superset-ui/core'; import { t } from '@apache-superset/core/translation'; +import { supersetTheme } from '@apache-superset/core/theme'; import PropTypes from 'prop-types'; import { FaSort } from 'react-icons/fa'; import { FaSortDown as FaSortDesc } from 'react-icons/fa'; import { FaSortUp as FaSortAsc } from 'react-icons/fa'; +import { + ColorFormatters, + getTextColorForBackground, + ObjectFormattingEnum, + ResolvedColorFormatterResult, +} from '@superset-ui/chart-controls'; import { PivotData, flatKey } from './utilities'; import { Styles } from './Styles'; -interface CellColorFormatter { - column: string; - getColorFromValue(value: unknown): string | undefined; -} - type ClickCallback = ( e: MouseEvent, value: unknown, @@ -59,8 +61,11 @@ interface TableOptions { highlightHeaderCellsOnHover?: boolean; omittedHighlightHeaderGroups?: string[]; highlightedHeaderCells?: Record; - cellColorFormatters?: Record; + cellColorFormatters?: Record; dateFormatters?: Record string) | undefined>; + cellBackgroundColor?: string; + cellTextColor?: string; + activeHeaderBackgroundColor?: string; } interface SubtotalDisplay { @@ -174,14 +179,19 @@ function displayHeaderCell( ); } -function getCellColor( +export function getCellColor( keys: string[], aggValue: string | number | null, - cellColorFormatters: Record | undefined, -): { backgroundColor: string | undefined } { + cellColorFormatters: Record | undefined, + cellBackgroundColor = supersetTheme.colorBgBase, +): ResolvedColorFormatterResult { if (!cellColorFormatters) return { backgroundColor: undefined }; let backgroundColor: string | undefined; + let color: string | undefined; + const isTextColorFormatter = (formatter: ColorFormatters[number]) => + formatter.objectFormatting === ObjectFormattingEnum.TEXT_COLOR || + formatter.toTextColor; for (const cellColorFormatter of Object.values(cellColorFormatters)) { if (!Array.isArray(cellColorFormatter)) continue; @@ -191,14 +201,26 @@ function getCellColor( if (formatter.column === key) { const result = formatter.getColorFromValue(aggValue); if (result) { - backgroundColor = result; + if (isTextColorFormatter(formatter)) { + color = result; + } else if ( + formatter.objectFormatting !== ObjectFormattingEnum.CELL_BAR + ) { + backgroundColor = result; + } } } } } } - return { backgroundColor }; + return { + backgroundColor, + color: getTextColorForBackground( + { backgroundColor, color }, + cellBackgroundColor, + ), + }; } interface HierarchicalNode { @@ -746,6 +768,8 @@ export class TableRenderer extends Component< highlightedHeaderCells, cellColorFormatters, dateFormatters, + cellBackgroundColor = supersetTheme.colorBgBase, + activeHeaderBackgroundColor = supersetTheme.colorPrimaryBg, } = this.props.tableOptions; if (!visibleColKeys || !colAttrSpans) { @@ -812,6 +836,7 @@ export class TableRenderer extends Component< ) { colLabelClass += ' active'; } + const isActiveHeader = colLabelClass.includes('active'); const maxRowIndex = pivotSettings.maxRowVisible!; const mColVisible = pivotSettings.maxColVisible!; const visibleSortIcon = mColVisible - 1 === attrIdx; @@ -844,12 +869,16 @@ export class TableRenderer extends Component< }; const headerCellFormattedValue = dateFormatters?.[attrName]?.(colKey[attrIdx]) ?? colKey[attrIdx]; - const { backgroundColor } = getCellColor( + const { backgroundColor, color } = getCellColor( [attrName], headerCellFormattedValue, cellColorFormatters, + isActiveHeader ? activeHeaderBackgroundColor : cellBackgroundColor, ); - const style = { backgroundColor }; + const style = { + backgroundColor, + ...(color ? { color } : {}), + }; attrValueCells.push( 0) { const flatRowKey = flatKey(rowKey.slice(0, i + 1)); @@ -1080,12 +1113,16 @@ export class TableRenderer extends Component< const headerCellFormattedValue = dateFormatters?.[rowAttrs[i]]?.(r) ?? r; - const { backgroundColor } = getCellColor( + const { backgroundColor, color } = getCellColor( [rowAttrs[i]], headerCellFormattedValue, cellColorFormatters, + isActiveHeader ? activeHeaderBackgroundColor : cellBackgroundColor, ); - const style = { backgroundColor }; + const style = { + backgroundColor, + ...(color ? { color } : {}), + }; return ( [2]; +type RenderTableRowSettings = Parameters[2]; +type TableRendererStateStub = TableRenderer['state']; +type CachedBasePivotSettings = NonNullable< + TableRenderer['cachedBasePivotSettings'] +>; + const columnIndex = 0; const visibleColKeys = [['col1'], ['col2']]; -const pivotData = { - subtotals: { - rowEnabled: true, - rowPartialOnTop: false, - }, -} as any; const maxRowIndex = 2; const mockProps = { @@ -56,6 +67,97 @@ const mockProps = { colPartialOnTop: false, }; +const toPivotData = (value: Partial): PivotData => + value as unknown as PivotData; + +const toCachedBasePivotSettings = ( + value: Partial, +): CachedBasePivotSettings => value as unknown as CachedBasePivotSettings; + +const createActiveHeaderTableOptions = ( + activeHeaderBackgroundColor: string, + overrides: Record = {}, +): TableRenderer['props']['tableOptions'] => + ({ + ...overrides, + activeHeaderBackgroundColor, + }) as unknown as TableRenderer['props']['tableOptions']; + +const createPivotDataStub = ( + aggregatorValue = 200, + isSubtotal = false, +): PivotData => + toPivotData({ + getAggregator: () => ({ + push: jest.fn(), + value: () => aggregatorValue, + format: (value: number) => String(value), + isSubtotal, + }), + }); + +const pivotData = toPivotData({ + subtotals: { + rowEnabled: true, + rowPartialOnTop: false, + }, +}); + +const createColHeaderRowSettings = ( + overrides: Partial = {}, +): RenderColHeaderRowSettings => + ({ + rowAttrs: [], + colAttrs: ['region'], + visibleColKeys: [['EMEA']], + colAttrSpans: [[1]], + colKeys: [['EMEA']], + colSubtotalDisplay: { + displayOnTop: false, + enabled: false, + hideOnExpand: false, + }, + rowSubtotalDisplay: { + displayOnTop: false, + enabled: false, + hideOnExpand: false, + }, + maxColVisible: 1, + maxRowVisible: 0, + pivotData: createPivotDataStub(), + namesMapping: {}, + allowRenderHtml: false, + arrowExpanded: null, + arrowCollapsed: null, + rowTotals: false, + colTotals: false, + ...overrides, + }) as RenderColHeaderRowSettings; + +const createTableRowSettings = ( + overrides: Partial = {}, +): RenderTableRowSettings => + ({ + rowAttrs: ['metric'], + colAttrs: [], + rowAttrSpans: [[1]], + visibleColKeys: [[]], + pivotData: createPivotDataStub(), + rowTotals: false, + rowSubtotalDisplay: { + displayOnTop: false, + enabled: false, + hideOnExpand: false, + }, + arrowExpanded: null, + arrowCollapsed: null, + cellCallbacks: {}, + rowTotalCallbacks: {}, + namesMapping: {}, + allowRenderHtml: false, + ...overrides, + }) as RenderTableRowSettings; + beforeEach(() => { tableRenderer = new TableRenderer(mockProps); @@ -65,24 +167,24 @@ beforeEach(() => { tableRenderer.getAggregatedData = mockGetAggregatedData; tableRenderer.sortAndCacheData = mockSortAndCacheData; - tableRenderer.cachedBasePivotSettings = { - pivotData: { + tableRenderer.cachedBasePivotSettings = toCachedBasePivotSettings({ + pivotData: toPivotData({ subtotals: { rowEnabled: true, rowPartialOnTop: false, colEnabled: false, colPartialOnTop: false, }, - }, + }), rowKeys: [['A'], ['B'], ['C']], - } as any; + }); tableRenderer.state = { sortingOrder: [], activeSortColumn: null, collapsedRows: {}, collapsedCols: {}, - } as any; + } as TableRendererStateStub; }); const mockGroups = { @@ -103,13 +205,15 @@ const mockGroups = { }, }; -const createMockPivotData = (rowData: Record) => - ({ +const createMockPivotData = (rowData: Record): PivotData => + toPivotData({ rowKeys: Object.keys(rowData).map(key => key.split('.')), - getAggregator: (rowKey: string[], colName: string) => ({ + getAggregator: (rowKey: string[]) => ({ + push: jest.fn(), value: () => rowData[rowKey.join('.')], + format: (value: number) => String(value), }), - }) as unknown as PivotData; + }); test('should set initial ascending sort when no active sort column', () => { mockGetAggregatedData.mockReturnValue({ @@ -211,15 +315,15 @@ test('should check second call in sequence', () => { activeSortColumn: 0, collapsedRows: {}, collapsedCols: {}, - } as any; + } as TableRendererStateStub; tableRenderer.sortData(columnIndex, visibleColKeys, pivotData, maxRowIndex); tableRenderer.state = { sortingOrder: ['asc' as never], - activeSortColumn: 0 as any, + activeSortColumn: 0, collapsedRows: {}, collapsedCols: {}, - } as any; + } as TableRendererStateStub; tableRenderer.sortData(columnIndex, visibleColKeys, pivotData, maxRowIndex); expect(mockSortAndCacheData).toHaveBeenCalledTimes(2); @@ -324,22 +428,23 @@ test('should sort hierarchical data in ascending order', () => { test('should calculate groups from pivot data', () => { tableRenderer = new TableRenderer(mockProps); const mockAggregator = (value: number) => ({ + push: jest.fn(), value: () => value, format: jest.fn(), isSubtotal: false, }); - const mockPivotData = { + const mockPivotData = toPivotData({ rowKeys: [['A'], ['B'], ['C']], getAggregator: jest .fn() .mockReturnValueOnce(mockAggregator(30)) .mockReturnValueOnce(mockAggregator(10)) .mockReturnValueOnce(mockAggregator(20)), - }; + }); const result = tableRenderer.getAggregatedData( - mockPivotData as any, + mockPivotData, ['col1'], false, ); @@ -589,3 +694,359 @@ test('values ​​from the 3rd level of the hierarchy with a subtotal at the to }, }); }); + +test('getCellColor derives readable text from the winning background', () => { + expect( + getCellColor( + ['revenue'], + 200, + { + metric: [ + { + column: 'revenue', + objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR, + getColorFromValue: (value: unknown) => + value === 200 ? '#111111' : undefined, + }, + ], + }, + '#ffffff', + ), + ).toEqual({ + backgroundColor: '#111111', + color: 'rgb(255, 255, 255)', + }); +}); + +test('getCellColor keeps explicit text color over adaptive contrast', () => { + expect( + getCellColor( + ['revenue'], + 200, + { + metric: [ + { + column: 'revenue', + objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR, + getColorFromValue: (value: unknown) => + value === 200 ? '#111111' : undefined, + }, + { + column: 'revenue', + objectFormatting: ObjectFormattingEnum.TEXT_COLOR, + getColorFromValue: (value: unknown) => + value === 200 ? '#ace1c40d' : undefined, + }, + ], + }, + '#ffffff', + ), + ).toEqual({ + backgroundColor: '#111111', + color: 'rgb(172, 225, 196)', + }); +}); + +test('getCellColor treats legacy toTextColor formatters as text color', () => { + expect( + getCellColor( + ['revenue'], + 200, + { + metric: [ + { + column: 'revenue', + getColorFromValue: (value: unknown) => + value === 200 ? '#111111' : undefined, + }, + { + column: 'revenue', + toTextColor: true, + getColorFromValue: (value: unknown) => + value === 200 ? '#ace1c40d' : undefined, + }, + ], + }, + '#ffffff', + ), + ).toEqual({ + backgroundColor: '#111111', + color: 'rgb(172, 225, 196)', + }); +}); + +test('getCellColor ignores cell-bar rules when resolving text color', () => { + expect( + getCellColor( + ['revenue'], + 200, + { + metric: [ + { + column: 'revenue', + objectFormatting: ObjectFormattingEnum.CELL_BAR, + getColorFromValue: (value: unknown) => + value === 200 ? '#11111199' : undefined, + }, + ], + }, + '#ffffff', + ), + ).toEqual({ + backgroundColor: undefined, + color: undefined, + }); +}); + +test('renderTableRow keeps subtotal background and readable text in sync', () => { + tableRenderer = new TableRenderer({ + ...mockProps, + tableOptions: { + cellColorFormatters: { + metric: [ + { + column: 'revenue', + objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR, + getColorFromValue: (value: unknown) => + value === 200 ? '#111111' : undefined, + }, + ], + }, + cellBackgroundColor: '#ffffff', + cellTextColor: '#000000', + }, + }); + + const row = tableRenderer.renderTableRow( + ['revenue'], + 0, + createTableRowSettings({ + pivotData: createPivotDataStub(200, true), + }), + ) as ReactElement; + + const cells = Children.toArray(row.props.children); + const valueCell = cells.find( + child => isValidElement(child) && child.props.className === 'pvtVal', + ) as ReactElement; + + expect(valueCell.props.style).toEqual({ + backgroundColor: '#111111', + color: 'rgb(255, 255, 255)', + fontWeight: 'bold', + }); +}); + +test('renderColAttrsHeader applies readable text color to formatted headers', () => { + tableRenderer = new TableRenderer({ + ...mockProps, + tableOptions: { + cellColorFormatters: { + metric: [ + { + column: 'region', + objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR, + getColorFromValue: (value: unknown) => + value === 'EMEA' ? '#111111' : undefined, + }, + ], + }, + cellBackgroundColor: '#ffffff', + cellTextColor: '#000000', + }, + }); + + const row = tableRenderer.renderColHeaderRow( + 'region', + 0, + createColHeaderRowSettings(), + ) as ReactElement; + + const cells = Children.toArray(row.props.children); + const headerCell = cells.find( + child => isValidElement(child) && child.props.className === 'pvtColLabel', + ) as ReactElement; + + expect(headerCell.props.style).toEqual({ + backgroundColor: '#111111', + color: 'rgb(255, 255, 255)', + }); +}); + +test('renderColAttrsHeader uses active header surface for adaptive contrast', () => { + const activeHeaderBackgroundColor = '#102a43'; + tableRenderer = new TableRenderer({ + ...mockProps, + tableOptions: createActiveHeaderTableOptions(activeHeaderBackgroundColor, { + highlightedHeaderCells: { + region: ['EMEA'], + }, + cellColorFormatters: { + metric: [ + { + column: 'region', + objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR, + getColorFromValue: (value: unknown) => + value === 'EMEA' ? 'rgba(0, 0, 0, 0.4)' : undefined, + }, + ], + }, + cellBackgroundColor: '#ffffff', + cellTextColor: '#000000', + }), + }); + + const row = tableRenderer.renderColHeaderRow( + 'region', + 0, + createColHeaderRowSettings(), + ) as ReactElement; + + const cells = Children.toArray(row.props.children); + const headerCell = cells.find( + child => + isValidElement(child) && child.props.className === 'pvtColLabel active', + ) as ReactElement; + + expect(headerCell.props.style).toEqual({ + backgroundColor: 'rgba(0, 0, 0, 0.4)', + color: getTextColorForBackground( + { backgroundColor: 'rgba(0, 0, 0, 0.4)' }, + activeHeaderBackgroundColor, + ), + }); +}); + +test('renderColHeaderRow preserves default header text color without formatting', () => { + tableRenderer = new TableRenderer({ + ...mockProps, + tableOptions: { + cellColorFormatters: { metric: [] }, + cellBackgroundColor: '#ffffff', + cellTextColor: '#ff00aa', + }, + }); + + const row = tableRenderer.renderColHeaderRow( + 'region', + 0, + createColHeaderRowSettings(), + ) as ReactElement; + + const cells = Children.toArray(row.props.children); + const headerCell = cells.find( + child => isValidElement(child) && child.props.className === 'pvtColLabel', + ) as ReactElement; + + expect(headerCell.props.style).toEqual({ + backgroundColor: undefined, + color: undefined, + }); +}); + +test('renderTableRow preserves default row-header text color without formatting', () => { + tableRenderer = new TableRenderer({ + ...mockProps, + tableOptions: { + cellColorFormatters: { metric: [] }, + cellBackgroundColor: '#ffffff', + cellTextColor: '#ff00aa', + }, + }); + + const row = tableRenderer.renderTableRow( + ['revenue'], + 0, + createTableRowSettings(), + ) as ReactElement; + + const cells = Children.toArray(row.props.children); + const headerCell = cells.find( + child => isValidElement(child) && child.props.className === 'pvtRowLabel', + ) as ReactElement; + + expect(headerCell.props.style).toEqual({ + backgroundColor: undefined, + color: undefined, + }); +}); + +test('renderTableRow applies readable text color to formatted row headers', () => { + tableRenderer = new TableRenderer({ + ...mockProps, + tableOptions: { + cellColorFormatters: { + metric: [ + { + column: 'metric', + objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR, + getColorFromValue: (value: unknown) => + value === 'revenue' ? '#111111' : undefined, + }, + ], + }, + cellBackgroundColor: '#ffffff', + cellTextColor: '#000000', + }, + }); + + const row = tableRenderer.renderTableRow( + ['revenue'], + 0, + createTableRowSettings(), + ) as ReactElement; + + const cells = Children.toArray(row.props.children); + const headerCell = cells.find( + child => isValidElement(child) && child.props.className === 'pvtRowLabel', + ) as ReactElement; + + expect(headerCell.props.style).toEqual({ + backgroundColor: '#111111', + color: 'rgb(255, 255, 255)', + }); +}); + +test('renderTableRow uses active header surface for adaptive contrast', () => { + const activeHeaderBackgroundColor = '#102a43'; + tableRenderer = new TableRenderer({ + ...mockProps, + tableOptions: createActiveHeaderTableOptions(activeHeaderBackgroundColor, { + highlightedHeaderCells: { + metric: ['revenue'], + }, + cellColorFormatters: { + metric: [ + { + column: 'metric', + objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR, + getColorFromValue: (value: unknown) => + value === 'revenue' ? 'rgba(0, 0, 0, 0.4)' : undefined, + }, + ], + }, + cellBackgroundColor: '#ffffff', + cellTextColor: '#000000', + }), + }); + + const row = tableRenderer.renderTableRow( + ['revenue'], + 0, + createTableRowSettings(), + ) as ReactElement; + + const cells = Children.toArray(row.props.children); + const headerCell = cells.find( + child => + isValidElement(child) && child.props.className === 'pvtRowLabel active', + ) as ReactElement; + + expect(headerCell.props.style).toEqual({ + backgroundColor: 'rgba(0, 0, 0, 0.4)', + color: getTextColorForBackground( + { backgroundColor: 'rgba(0, 0, 0, 0.4)' }, + activeHeaderBackgroundColor, + ), + }); +}); diff --git a/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx b/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx index 02be074bf65..02b0a8520a6 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx +++ b/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx @@ -75,6 +75,7 @@ import { import { isEmpty, debounce, isEqual } from 'lodash'; import { ColorFormatters, + getTextColorForBackground, ObjectFormattingEnum, ColorSchemeEnum, } from '@superset-ui/chart-controls'; @@ -944,9 +945,11 @@ export default function TableChart( if (!formatterResult) return; if ( - formatter.objectFormatting === ObjectFormattingEnum.TEXT_COLOR + formatter.objectFormatting === + ObjectFormattingEnum.TEXT_COLOR || + formatter.toTextColor ) { - color = formatterResult.slice(0, -2); + color = formatterResult; } else if ( formatter.objectFormatting === ObjectFormattingEnum.CELL_BAR ) { @@ -997,8 +1000,13 @@ export default function TableChart( ? basicColorColumnFormatters[row.index][column.key]?.mainArrow : ''; } + const rowSurfaceColor = + row.index % 2 === 0 ? theme.colorBgLayout : theme.colorBgBase; + const resolvedTextColor = getTextColorForBackground( + { backgroundColor, color }, + rowSurfaceColor, + ); const StyledCell = styled.td` - color: ${color ? `${color}FF` : theme.colorText}; text-align: ${sharedStyle.textAlign}; white-space: ${value instanceof Date ? 'nowrap' : undefined}; position: relative; @@ -1097,6 +1105,9 @@ export default function TableChart( : '', isActiveFilterValue(key, value) ? ' dt-is-active-filter' : '', ].join(' '), + style: resolvedTextColor + ? ({ color: resolvedTextColor } as CSSProperties) + : undefined, tabIndex: 0, }; if (html) { diff --git a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx index f3963b0477d..7b702bb24a6 100644 --- a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx +++ b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx @@ -17,7 +17,11 @@ * under the License. */ import '@testing-library/jest-dom'; -import { ObjectFormattingEnum } from '@superset-ui/chart-controls'; +import { + getTextColorForBackground, + ObjectFormattingEnum, +} from '@superset-ui/chart-controls'; +import { supersetTheme } from '@apache-superset/core/theme'; import { render, screen, @@ -670,6 +674,52 @@ describe('plugin-chart-table', () => { ); expect(getComputedStyle(screen.getByText('N/A')).background).toBe(''); }); + + test('preserves muted null styling when no formatter resolves text color', () => { + const dataWithEmptyCell = cloneDeep(testData.advanced.queriesData[0]); + dataWithEmptyCell.data.push({ + __timestamp: null, + name: 'Noah', + sum__num: null, + '%pct_nice': 0.643, + 'abc.com': 'bazzinga', + }); + + render( + ProviderWrapper({ + children: ( + + ), + }), + ); + + const noahRow = screen.getByText('Noah').closest('tr'); + expect(noahRow).not.toBeNull(); + + const nullCell = noahRow?.querySelector('td.dt-is-null'); + expect(nullCell).not.toBeNull(); + expect((nullCell as HTMLElement).style.color).toBe(''); + expect(getComputedStyle(nullCell as Element).color).toBe( + 'rgba(0, 0, 0, 0.45)', + ); + }); + test('should display original label in grouped headers', () => { const props = transformProps(testData.comparison); @@ -1351,11 +1401,9 @@ describe('plugin-chart-table', () => { ); expect(getComputedStyle(screen.getByTitle('2467063')).color).toBe( - 'rgba(172, 225, 196, 1)', - ); - expect(getComputedStyle(screen.getByTitle('2467')).color).toBe( - 'rgba(0, 0, 0, 0.88)', + 'rgb(172, 225, 196)', ); + expect((screen.getByTitle('2467') as HTMLElement).style.color).toBe(''); }); test('display text color using column color formatter for entire row', () => { @@ -1385,13 +1433,182 @@ describe('plugin-chart-table', () => { ); expect(getComputedStyle(screen.getByText('Michael')).color).toBe( - 'rgba(172, 225, 196, 1)', + 'rgb(172, 225, 196)', ); expect(getComputedStyle(screen.getByTitle('2467063')).color).toBe( - 'rgba(172, 225, 196, 1)', + 'rgb(172, 225, 196)', ); expect(getComputedStyle(screen.getByTitle('0.123456')).color).toBe( - 'rgba(172, 225, 196, 1)', + 'rgb(172, 225, 196)', + ); + }); + + test('derive readable text color from dark background formatting', () => { + render( + ProviderWrapper({ + children: ( + ', + targetValue: 2467, + useGradient: false, + }, + ], + }, + })} + /> + ), + }), + ); + + expect(getComputedStyle(screen.getByTitle('2467063')).background).toBe( + 'rgb(17, 17, 17)', + ); + expect(getComputedStyle(screen.getByTitle('2467063')).color).toBe( + 'rgb(255, 255, 255)', + ); + }); + + test('keep explicit text color over adaptive contrast', () => { + render( + ProviderWrapper({ + children: ( + ', + targetValue: 2467, + useGradient: false, + }, + { + colorScheme: '#ACE1C4', + column: 'sum__num', + operator: '>', + targetValue: 2467, + objectFormatting: ObjectFormattingEnum.TEXT_COLOR, + }, + ], + }, + })} + /> + ), + }), + ); + + expect(getComputedStyle(screen.getByTitle('2467063')).background).toBe( + 'rgb(17, 17, 17)', + ); + expect(getComputedStyle(screen.getByTitle('2467063')).color).toBe( + 'rgb(172, 225, 196)', + ); + }); + + test('support legacy toTextColor formatters', () => { + render( + ProviderWrapper({ + children: ( + ', + targetValue: 2467, + useGradient: false, + }, + { + colorScheme: '#ACE1C4', + column: 'sum__num', + operator: '>', + targetValue: 2467, + toTextColor: true, + }, + ], + }, + })} + /> + ), + }), + ); + + expect(getComputedStyle(screen.getByTitle('2467063')).background).toBe( + 'rgb(17, 17, 17)', + ); + expect(getComputedStyle(screen.getByTitle('2467063')).color).toBe( + 'rgb(172, 225, 196)', + ); + }); + + test('use striped row surface when deriving adaptive text color', () => { + const backgroundColor = Array.from( + { length: 0xff }, + (_, index) => `#000000${(index + 1).toString(16).padStart(2, '0')}`, + ).find(candidate => { + const baseColor = getTextColorForBackground( + { backgroundColor: candidate }, + supersetTheme.colorBgBase, + ); + const layoutColor = getTextColorForBackground( + { backgroundColor: candidate }, + supersetTheme.colorBgLayout, + ); + return baseColor !== layoutColor; + }); + + expect(backgroundColor).toBeDefined(); + + render( + ProviderWrapper({ + children: ( + ', + targetValue: 2000, + useGradient: false, + }, + ], + }, + })} + /> + ), + }), + ); + + expect(getComputedStyle(screen.getByTitle('2467063')).color).toBe( + getTextColorForBackground( + { backgroundColor }, + supersetTheme.colorBgLayout, + ), + ); + expect(getComputedStyle(screen.getByTitle('2467')).color).toBe( + getTextColorForBackground( + { backgroundColor }, + supersetTheme.colorBgBase, + ), ); });