feat(charts): improve negative stacked bar label positioning and accessibility (#37405)

superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts

@@ -31,6 +31,7 @@ import {
   ValueFormatter,
 } from '@superset-ui/core';
 import { SupersetTheme, isThemeDark } from '@apache-superset/core/ui';
+import { getContrastingColor } from '@superset-ui/core';
 import type {
   CallbackDataParams,
   DefaultStatesMixin,
@@ -142,28 +143,39 @@ export const getBaselineSeriesForStream = (
   };
 };
 
-export function transformNegativeLabelsPosition(
+export function optimizeBarLabelPlacement(
   series: SeriesOption,
   isHorizontal: boolean,
 ): TimeseriesDataRecord[] {
   /*
-   * Adjusts label position for negative values in bar series
+   * Adjusts label position for all values in bar series
+   * Positions labels inside bars at appropriate edges to avoid axis overlap
    * @param series - Array of series options
    * @param isHorizontal - Whether chart is horizontal
-   * @returns data with adjusted label positions for negative values
+   * @returns data with adjusted label positions for all values
    */
   const transformValue = (value: any) => {
     const [xValue, yValue] = Array.isArray(value) ? value : [null, null];
     const axisValue = isHorizontal ? xValue : yValue;
 
-    return axisValue < 0
-      ? {
-          value,
-          label: {
-            position: 'outside',
-          },
-        }
-      : value;
+    if (axisValue === null || axisValue === undefined) {
+      return value;
+    }
+
+    // Use inside positioning for all bar charts to avoid axis overlap
+    const labelPosition =
+      axisValue < 0
+        ? isHorizontal
+          ? 'insideLeft'
+          : 'insideBottom'
+        : isHorizontal
+          ? 'insideRight'
+          : 'insideTop';
+
+    return {
+      value,
+      label: { position: labelPosition },
+    };
   };
 
   return (series.data as TimeseriesDataRecord[]).map(transformValue);
@@ -337,8 +349,10 @@ export function transformSeries(
 
   return {
     ...series,
-    ...(Array.isArray(data) && seriesType === 'bar' && !stack
-      ? { data: transformNegativeLabelsPosition(series, isHorizontal) }
+    ...(Array.isArray(data) && seriesType === 'bar'
+      ? {
+          data: optimizeBarLabelPlacement(series, isHorizontal),
+        }
       : null),
     connectNulls,
     queryIndex,
@@ -371,8 +385,11 @@ export function transformSeries(
     symbolSize: markerSize,
     label: {
       show: !!showValue,
-      position: isHorizontal ? 'right' : 'top',
-      color: theme?.colorText,
+      position: stack ? 'inside' : isHorizontal ? 'right' : 'top',
+      color:
+        stack || seriesType === 'bar'
+          ? getContrastingColor(String(itemStyle.color))
+          : theme?.colorText,
       textBorderWidth: 0,
       formatter: (params: any) => {
         // don't show confidence band value labels, as they're already visible on the tooltip

superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts

@@ -22,20 +22,24 @@ import { supersetTheme } from '@apache-superset/core/ui';
 import type { SeriesOption } from 'echarts';
 import { EchartsTimeseriesSeriesType } from '../../src';
 import { TIMESERIES_CONSTANTS } from '../../src/constants';
-import { LegendOrientation } from '../../src/types';
+import {
+  LegendOrientation,
+  EchartsTimeseriesChartProps,
+} from '../../src/types';
 import {
   transformSeries,
-  transformNegativeLabelsPosition,
+  optimizeBarLabelPlacement,
   getPadding,
 } from '../../src/Timeseries/transformers';
 import transformProps from '../../src/Timeseries/transformProps';
-import { EchartsTimeseriesChartProps } from '../../src/types';
 import * as seriesUtils from '../../src/utils/series';
 
-// Mock the colorScale function
-const mockColorScale = jest.fn(
-  (key: string, sliceId?: number) => `color-for-${key}-${sliceId}`,
-) as unknown as CategoricalColorScale;
+// Mock the colorScale function to return different colors based on key
+const mockColorScale = jest.fn((key: string) => {
+  if (key === 'test-key') return '#1f77b4'; // blue
+  if (key === 'series-key') return '#ff7f0e'; // orange
+  return '#2ca02c'; // green for any other key
+}) as unknown as CategoricalColorScale;
 
 describe('transformSeries', () => {
   const series = { name: 'test-series' };
@@ -49,7 +53,8 @@ describe('transformSeries', () => {
 
     const result = transformSeries(series, mockColorScale, 'test-key', opts);
 
-    expect((result as any)?.itemStyle.color).toBe('color-for-test-key-1');
+    expect(mockColorScale).toHaveBeenCalledWith('test-key', 1);
+    expect((result as any)?.itemStyle.color).toBe('#1f77b4');
   });
 
   test('should use seriesKey if timeShiftColor is not enabled', () => {
@@ -61,7 +66,8 @@ describe('transformSeries', () => {
 
     const result = transformSeries(series, mockColorScale, 'test-key', opts);
 
-    expect((result as any)?.itemStyle.color).toBe('color-for-series-key-2');
+    expect(mockColorScale).toHaveBeenCalledWith('series-key', 2);
+    expect((result as any)?.itemStyle.color).toBe('#ff7f0e');
   });
 
   test('should apply border styles for bar series with connectNulls', () => {
@@ -123,8 +129,8 @@ describe('transformSeries', () => {
   });
 });
 
-describe('transformNegativeLabelsPosition', () => {
-  test('label position bottom of negative value no Horizontal', () => {
+describe('optimizeBarLabelPlacement', () => {
+  test('label position for non-stacked vertical charts', () => {
     const isHorizontal = false;
     const series: SeriesOption = {
       data: [
@@ -137,15 +143,12 @@ describe('transformNegativeLabelsPosition', () => {
       type: EchartsTimeseriesSeriesType.Bar,
       stack: undefined,
     };
-    const result =
-      Array.isArray(series.data) && series.type === 'bar' && !series.stack
-        ? transformNegativeLabelsPosition(series, isHorizontal)
-        : series.data;
-    expect((result as any)[0].label).toBe(undefined);
-    expect((result as any)[1].label).toBe(undefined);
-    expect((result as any)[2].label.position).toBe('outside');
-    expect((result as any)[3].label.position).toBe('outside');
-    expect((result as any)[4].label).toBe(undefined);
+    const result = optimizeBarLabelPlacement(series, isHorizontal);
+    expect((result as any)[0].label.position).toBe('insideTop');
+    expect((result as any)[1].label.position).toBe('insideTop');
+    expect((result as any)[2].label.position).toBe('insideBottom');
+    expect((result as any)[3].label.position).toBe('insideBottom');
+    expect((result as any)[4].label.position).toBe('insideTop');
   });
 
   test('label position left of negative value is Horizontal', () => {
@@ -162,15 +165,12 @@ describe('transformNegativeLabelsPosition', () => {
       stack: undefined,
     };
 
-    const result =
-      Array.isArray(series.data) && series.type === 'bar' && !series.stack
-        ? transformNegativeLabelsPosition(series, isHorizontal)
-        : series.data;
-    expect((result as any)[0].label).toBe(undefined);
-    expect((result as any)[1].label.position).toBe('outside');
-    expect((result as any)[2].label).toBe(undefined);
-    expect((result as any)[3].label.position).toBe('outside');
-    expect((result as any)[4].label.position).toBe('outside');
+    const result = optimizeBarLabelPlacement(series, isHorizontal);
+    expect((result as any)[0].label.position).toBe('insideRight');
+    expect((result as any)[1].label.position).toBe('insideLeft');
+    expect((result as any)[2].label.position).toBe('insideRight');
+    expect((result as any)[3].label.position).toBe('insideLeft');
+    expect((result as any)[4].label.position).toBe('insideLeft');
   });
 
   test('label position to line type', () => {
@@ -192,7 +192,7 @@ describe('transformNegativeLabelsPosition', () => {
       !series.stack &&
       series.type !== 'line' &&
       series.type === 'bar'
-        ? transformNegativeLabelsPosition(series, isHorizontal)
+        ? optimizeBarLabelPlacement(series, isHorizontal)
         : series.data;
     expect((result as any)[0].label).toBe(undefined);
     expect((result as any)[1].label).toBe(undefined);
@@ -215,15 +215,34 @@ describe('transformNegativeLabelsPosition', () => {
       stack: 'obs',
     };
 
-    const result =
-      Array.isArray(series.data) && series.type === 'bar' && !series.stack
-        ? transformNegativeLabelsPosition(series, isHorizontal)
-        : series.data;
-    expect((result as any)[0].label).toBe(undefined);
-    expect((result as any)[1].label).toBe(undefined);
-    expect((result as any)[2].label).toBe(undefined);
-    expect((result as any)[3].label).toBe(undefined);
-    expect((result as any)[4].label).toBe(undefined);
+    const result = optimizeBarLabelPlacement(series, isHorizontal);
+    expect((result as any)[0].label.position).toBe('insideTop');
+    expect((result as any)[1].label.position).toBe('insideTop');
+    expect((result as any)[2].label.position).toBe('insideBottom');
+    expect((result as any)[3].label.position).toBe('insideBottom');
+    expect((result as any)[4].label.position).toBe('insideTop');
+  });
+
+  test('label position for horizontal stacked charts', () => {
+    const isHorizontal = true;
+    const series: SeriesOption = {
+      data: [
+        [1, 2020],
+        [-3, 2021],
+        [2, 2022],
+        [-4, 2023],
+        [-6, 2024],
+      ],
+      type: EchartsTimeseriesSeriesType.Bar,
+      stack: 'obs',
+    };
+
+    const result = optimizeBarLabelPlacement(series, isHorizontal);
+    expect((result as any)[0].label.position).toBe('insideRight');
+    expect((result as any)[1].label.position).toBe('insideLeft');
+    expect((result as any)[2].label.position).toBe('insideRight');
+    expect((result as any)[3].label.position).toBe('insideLeft');
+    expect((result as any)[4].label.position).toBe('insideLeft');
   });
 });