From ba50bdff237e28dfd18f0792ffaf60d0c8ca9cae Mon Sep 17 00:00:00 2001 From: Joe Li Date: Tue, 1 Sep 2026 13:23:17 -0700 Subject: [PATCH] fix(echarts): prevent x-axis time labels from overlapping (#43669) Co-authored-by: Claude Sonnet 5 --- .../src/MixedTimeseries/transformProps.ts | 50 ++---- .../src/Timeseries/transformProps.ts | 56 +++---- .../plugin-chart-echarts/src/constants.ts | 6 + .../src/utils/formatters.ts | 150 ++++++++++++++++++ .../MixedTimeseries/transformProps.test.ts | 52 ++++++ .../test/Timeseries/transformProps.test.ts | 38 +++++ .../test/Timeseries/transformers.test.ts | 49 ++++++ 7 files changed, 333 insertions(+), 68 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts index 64cdc7b287a..1855b19a202 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts @@ -103,7 +103,9 @@ import { import { TIMEGRAIN_TO_TIMESTAMP, TIMESERIES_CONSTANTS } from '../constants'; import { getDefaultTooltip } from '../utils/tooltip'; import { + createSpacedXAxisFormatter, getTooltipTimeFormatter, + getXAxisDomain, getXAxisFormatter, getYAxisFormatter, } from '../utils/formatters'; @@ -664,44 +666,26 @@ export default function transformProps( ? getXAxisFormatter(xAxisTimeFormat, resolvedTimeGrain) : String; + // hideOverlap must stay off so the forced boundary label from showMaxLabel + // is never suppressed (#39899). The formatter itself dedupes consecutive + // identical labels and thins out labels that would otherwise visually + // collide, since hideOverlap can no longer do that for us. const showMaxLabel = xAxisType === AxisType.Time && xAxisLabelRotation === 0 && !!resolvedTimeGrain; const deduplicatedFormatter = showMaxLabel - ? (() => { - let lastLabel: string | undefined; - let lastValue: number | undefined; - const wrapper = (value: number | string) => { - // ECharts formats the labels in repeated ascending passes. Reset the - // dedup state when the sequence restarts so a forced boundary label - // (e.g. the min date) isn't blanked by the previous pass's last label - // when both format identically (e.g. a May-to-May range). - if ( - typeof value === 'number' && - lastValue !== undefined && - value <= lastValue - ) { - lastLabel = undefined; - } - if (typeof value === 'number') { - lastValue = value; - } - const label = - typeof xAxisFormatter === 'function' - ? (xAxisFormatter as Function)(value) - : String(value); - if (label === lastLabel) { - return ''; - } - lastLabel = label; - return label; - }; - if (typeof xAxisFormatter === 'function' && 'id' in xAxisFormatter) { - (wrapper as any).id = (xAxisFormatter as any).id; - } - return wrapper; - })() + ? createSpacedXAxisFormatter( + xAxisFormatter, + ...getXAxisDomain( + [ + rebasedDataA as Record[], + rebasedDataB as Record[], + ], + xAxisLabel, + ), + Math.max(width - 2 * TIMESERIES_CONSTANTS.gridOffsetLeft, 0), + ) : xAxisFormatter; const yAxisTitleMarginPx = convertInteger(yAxisTitleMargin); diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts index 4882dfff5e1..169a7563128 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts @@ -126,8 +126,11 @@ import { } from '../constants'; import { getDefaultTooltip } from '../utils/tooltip'; import { + createDedupXAxisFormatter, + createSpacedXAxisFormatter, getPercentFormatter, getTooltipTimeFormatter, + getXAxisDomain, getXAxisFormatter, getYAxisFormatter, } from '../utils/formatters'; @@ -1213,46 +1216,29 @@ export default function transformProps( // When showMaxLabel is true, ECharts may render a label at the axis // boundary that formats identically to the last data-point tick (e.g. - // "2005" appears twice with Year grain). Wrap the formatter to suppress - // consecutive duplicate labels. + // "2005" appears twice with Year grain), and hideOverlap must stay off so + // that forced boundary label is never suppressed (#39899). Wrap the + // formatter to suppress consecutive duplicate labels and to thin out + // labels that would otherwise visually collide, since hideOverlap can no + // longer do that for us. The spacing estimate assumes the axis runs along + // the bottom of the chart (pixel width, character width); a horizontal + // orientation chart puts the time axis on the side instead, so it falls + // back to dedup-only there. const showMaxLabel = xAxisType === AxisType.Time && xAxisLabelRotation === 0 && !!resolvedTimeGrain; const deduplicatedFormatter = showMaxLabel - ? (() => { - let lastLabel: string | undefined; - let lastValue: number | undefined; - const wrapper = (value: number | string) => { - // ECharts formats the labels in repeated ascending passes. Reset the - // dedup state when the sequence restarts so a forced boundary label - // (e.g. the min date) isn't blanked by the previous pass's last label - // when both format identically (e.g. a May-to-May range). - if ( - typeof value === 'number' && - lastValue !== undefined && - value <= lastValue - ) { - lastLabel = undefined; - } - if (typeof value === 'number') { - lastValue = value; - } - const label = - typeof xAxisFormatter === 'function' - ? (xAxisFormatter as Function)(value) - : String(value); - if (label === lastLabel) { - return ''; - } - lastLabel = label; - return label; - }; - if (typeof xAxisFormatter === 'function' && 'id' in xAxisFormatter) { - (wrapper as any).id = (xAxisFormatter as any).id; - } - return wrapper; - })() + ? isHorizontal + ? createDedupXAxisFormatter(xAxisFormatter) + : createSpacedXAxisFormatter( + xAxisFormatter, + ...getXAxisDomain( + [rebasedData as Record[]], + xAxisLabel, + ), + Math.max(width - 2 * TIMESERIES_CONSTANTS.gridOffsetLeft, 0), + ) : xAxisFormatter; const temporalTickValues = resolveTemporalTickValues( diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/constants.ts b/superset-frontend/plugins/plugin-chart-echarts/src/constants.ts index 2b94967ae00..7454da43b92 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/constants.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/constants.ts @@ -52,6 +52,12 @@ export const TIMESERIES_CONSTANTS = { microChartHeight: 60, // One y-axis tick per this many pixels of chart height yAxisPixelsPerTick: 80, + // Rough average glyph width (px) used to estimate whether adjacent x-axis + // time labels would visually collide, since the real rendered width isn't + // known until ECharts lays out the axis. + xAxisLabelCharWidthPx: 7, + // Minimum gap (px) to keep between adjacent x-axis time labels. + xAxisLabelMinGapPx: 8, }; export enum OpacityEnum { diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts b/superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts index 485bc2407ac..5fe4dd19560 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts @@ -24,6 +24,7 @@ import { getTimeFormatter, isSavedMetric, NumberFormats, + NumberFormatter, QueryFormMetric, SMART_DATE_DETAILED_ID, SMART_DATE_ID, @@ -32,6 +33,7 @@ import { TimeGranularity, ValueFormatter, } from '@superset-ui/core'; +import { TIMESERIES_CONSTANTS } from '../constants'; export const getSmartDateDetailedFormatter = () => getTimeFormatter(SMART_DATE_DETAILED_ID); @@ -213,3 +215,151 @@ export function getXAxisFormatter( } return String; } + +type XAxisFormatterFn = + | TimeFormatter + | NumberFormatter + | StringConstructor + | ((value: number | string) => string); + +/** + * Wraps an x-axis time formatter so that consecutive ticks that format to + * identical text are blanked (e.g. the boundary label forced by + * showMaxLabel duplicating the last real tick). + * + * Use this instead of createSpacedXAxisFormatter when the axis geometry + * doesn't match the spacing model's horizontal-plot assumptions, e.g. a + * horizontal orientation chart, where the time axis runs vertically along + * the side of the chart rather than along the bottom. + */ +export function createDedupXAxisFormatter( + xAxisFormatter: XAxisFormatterFn | undefined, +): (value: number | string) => string { + let lastLabel: string | undefined; + let lastValue: number | undefined; + const wrapper = (value: number | string) => { + // ECharts formats the labels in repeated ascending passes. Reset the + // dedup state when the sequence restarts so a forced boundary label + // (e.g. the min date) isn't blanked by the previous pass's last label + // when both format identically (e.g. a May-to-May range). + if ( + typeof value === 'number' && + lastValue !== undefined && + value <= lastValue + ) { + lastLabel = undefined; + } + if (typeof value === 'number') { + lastValue = value; + } + const label = + typeof xAxisFormatter === 'function' + ? (xAxisFormatter as Function)(value) + : String(value); + if (label === lastLabel) { + return ''; + } + lastLabel = label; + return label; + }; + if (typeof xAxisFormatter === 'function' && 'id' in xAxisFormatter) { + (wrapper as { id?: unknown }).id = (xAxisFormatter as { id?: unknown }).id; + } + return wrapper; +} + +/** + * Wraps an x-axis time formatter so that: + * - consecutive ticks that format to identical text are blanked (e.g. the + * boundary label forced by showMaxLabel duplicating the last real tick). + * - ticks that would render close enough to visually collide with the + * previously shown label are blanked, since disabling ECharts' + * `hideOverlap` (required to keep the forced boundary label visible, see + * #39899) also disables its native overlap suppression for every other + * label on the axis. + * + * The forced axis boundary labels (domainMin/domainMax) are never blanked by + * the spacing check so they stay visible regardless of density. + */ +export function createSpacedXAxisFormatter( + xAxisFormatter: XAxisFormatterFn | undefined, + domainMin: number | undefined, + domainMax: number | undefined, + plotWidthPx: number, +): (value: number | string) => string { + const pixelsPerMs = + domainMin !== undefined && domainMax !== undefined && domainMax > domainMin + ? plotWidthPx / (domainMax - domainMin) + : undefined; + let lastLabel: string | undefined; + let lastValue: number | undefined; + let lastShownValue: number | undefined; + const wrapper = (value: number | string) => { + // ECharts formats the labels in repeated ascending passes. Reset the + // dedup/spacing state when the sequence restarts so a forced boundary + // label (e.g. the min date) isn't blanked by the previous pass's state + // when both format identically (e.g. a May-to-May range). + if ( + typeof value === 'number' && + lastValue !== undefined && + value <= lastValue + ) { + lastLabel = undefined; + lastShownValue = undefined; + } + if (typeof value === 'number') { + lastValue = value; + } + const label = + typeof xAxisFormatter === 'function' + ? (xAxisFormatter as Function)(value) + : String(value); + if (label === lastLabel) { + return ''; + } + const isBoundary = + typeof value === 'number' && (value === domainMin || value === domainMax); + if ( + !isBoundary && + typeof value === 'number' && + pixelsPerMs !== undefined && + lastShownValue !== undefined && + (value - lastShownValue) * pixelsPerMs < + label.length * TIMESERIES_CONSTANTS.xAxisLabelCharWidthPx + + TIMESERIES_CONSTANTS.xAxisLabelMinGapPx + ) { + return ''; + } + lastLabel = label; + if (typeof value === 'number') { + lastShownValue = value; + } + return label; + }; + if (typeof xAxisFormatter === 'function' && 'id' in xAxisFormatter) { + (wrapper as { id?: unknown }).id = (xAxisFormatter as { id?: unknown }).id; + } + return wrapper; +} + +/** + * Computes the [min, max] of a temporal x-axis column across one or more + * data record arrays, for use with createSpacedXAxisFormatter. + */ +export function getXAxisDomain( + dataRecordArrays: Record[][], + xAxisCol: string, +): [number | undefined, number | undefined] { + let domainMin: number | undefined; + let domainMax: number | undefined; + dataRecordArrays.forEach(records => { + records.forEach(record => { + const value = record[xAxisCol]; + if (typeof value === 'number') { + if (domainMin === undefined || value < domainMin) domainMin = value; + if (domainMax === undefined || value > domainMax) domainMax = value; + } + }); + }); + return [domainMin, domainMax]; +} diff --git a/superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts b/superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts index 2288ff19386..be425d16d5f 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts @@ -1379,6 +1379,58 @@ test('#39899 - x-axis dates do not overlap and last label stays visible at 0° r expect(axisLabel.hideOverlap).toBe(false); }); +test('#39899 - closely spaced x-axis time labels do not visually overlap (mixed)', () => { + const startTime = Date.UTC(2026, 0, 1); + const data = Array.from({ length: 20 }, (_, i) => ({ + __timestamp: startTime + i * 60 * 1000, + sum__num: i, + })); + const queryData = createTestQueryData(data, { + colnames: ['__timestamp', 'sum__num'], + coltypes: [GenericDataType.Temporal, GenericDataType.Numeric], + label_map: { __timestamp: ['__timestamp'], sum__num: ['sum__num'] }, + }); + + const chartProps = createEchartsTimeseriesTestChartProps< + EchartsMixedTimeseriesFormData, + EchartsMixedTimeseriesProps + >({ + ...MIXED_TIMESERIES_CHART_PROPS_DEFAULTS, + width: 300, + height: 400, + defaultQueriesData: [queryData, queryData], + formData: { + ...formData, + x_axis: '__timestamp', + xAxisTimeFormat: '%Y-%m-%d %H:%M:%S', + metrics: ['sum__num'], + metricsB: ['sum__num'], + groupby: [], + groupbyB: [], + xAxisLabelRotation: 0, + timeGrainSqla: TimeGranularity.MINUTE, + }, + queriesData: [queryData, queryData], + }); + + const { echartOptions } = transformProps(chartProps); + const { axisLabel } = echartOptions.xAxis as Record; + const labels = data.map(({ __timestamp }) => + axisLabel.formatter(__timestamp), + ); + + // hideOverlap must stay off so ECharts' own collision detection can never + // suppress the forced boundary label (#39899 must not regress). + expect(axisLabel.hideOverlap).toBe(false); + // The formatter itself must thin out labels that are too close together to + // render legibly in the available width. + expect(labels.filter(label => label === '').length).toBeGreaterThan(0); + // The first and last labels are the forced axis boundaries and must always + // stay visible. + expect(labels[0]).not.toBe(''); + expect(labels[labels.length - 1]).not.toBe(''); +}); + test('regression #37921: multi-metric Query A with groupby does not duplicate first metric in series names', () => { // Regression test for https://github.com/apache/superset/issues/37921 // ("Residual" follow-up to #37055). diff --git a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts index a24ff415857..45d782ff458 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts @@ -3132,6 +3132,44 @@ test('applies gridlines to the value axis after a horizontal orientation swaps i expect((echartOptions.xAxis as any).splitLine.show).toBe(false); }); +test('#39899 - horizontal orientation does not over-thin the time axis labels', () => { + // The spacing formatter estimates label collisions using horizontal plot + // geometry (width, 7px/char). A horizontal chart swaps the time axis onto + // the side of the chart, where that geometry no longer applies, so the + // spacing formatter must not be used there. + const monthData = Array.from({ length: 24 }, (_, i) => ({ + __timestamp: Date.UTC(2020, i, 1), + sales: i, + })); + const { echartOptions } = transformProps( + createTestChartProps({ + formData: { + granularity_sqla: 'ds', + timeGrainSqla: TimeGranularity.MONTH, + xAxisTimeFormat: '%Y-%m', + seriesType: EchartsTimeseriesSeriesType.Bar, + orientation: OrientationType.Horizontal, + }, + width: 800, + queriesData: [ + createTestQueryData(monthData, { + colnames: ['__timestamp', 'sales'], + coltypes: [GenericDataType.Temporal, GenericDataType.Numeric], + }), + ], + }), + ); + // Horizontal swaps the axes, so the time axis ends up as yAxis. + const { axisLabel } = echartOptions.yAxis as Record; + const labels = monthData.map(({ __timestamp }) => + axisLabel.formatter(__timestamp), + ); + + // Every month is a distinct label, so none should be blanked by the + // spacing/dedup formatter on a horizontal chart. + expect(labels.filter(label => label === '')).toHaveLength(0); +}); + test('boundary label alignment is dropped when the orientation moves the time axis to the side', () => { // The alignments position labels against the left and right edges of a // bottom axis. A horizontal chart swaps the axes, so applying them there diff --git a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts index ba49f4b3845..13a8638f180 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts @@ -834,6 +834,55 @@ test('#39899 - x-axis dates do not overlap and last label stays visible at 0° r expect(axisLabel.hideOverlap).toBe(false); }); +test('#39899 - closely spaced x-axis time labels do not visually overlap', () => { + const formData = { + colorScheme: 'bnbColors', + datasource: '3__table', + granularity_sqla: 'ds', + timeGrainSqla: TimeGranularity.MINUTE, + x_axis_time_format: '%Y-%m-%d %H:%M:%S', + metric: 'sum__num', + viz_type: 'my_viz', + }; + const startTime = new Date('2026-01-01T00:00:00Z').getTime(); + const data = Array.from({ length: 20 }, (_, i) => ({ + sum__num: i, + __timestamp: startTime + i * 60 * 1000, + })); + const chartProps = new ChartProps({ + formData, + width: 300, + height: 400, + queriesData: [ + { + data, + colnames: ['sum__num', '__timestamp'], + coltypes: [GenericDataType.Numeric, GenericDataType.Temporal], + }, + ], + theme: supersetTheme, + }); + + const result = transformProps( + chartProps as unknown as EchartsTimeseriesChartProps, + ); + const { axisLabel } = result.echartOptions.xAxis as Record; + const labels = data.map(({ __timestamp }) => + axisLabel.formatter(__timestamp), + ); + + // hideOverlap must stay off so ECharts' own collision detection can never + // suppress the forced boundary label (#39899 must not regress). + expect(axisLabel.hideOverlap).toBe(false); + // The formatter itself must thin out labels that are too close together to + // render legibly in the available width. + expect(labels.filter(label => label === '').length).toBeGreaterThan(0); + // The first and last labels are the forced axis boundaries and must always + // stay visible. + expect(labels[0]).not.toBe(''); + expect(labels[labels.length - 1]).not.toBe(''); +}); + test('last x-axis date is visible and not cut off when rotated -45°', () => { const lastDataPointTimestamp = new Date('2026-12-01').getTime(); const result = transformProps(