(superset) 02/03: fix(echarts): account for non-legend chart padding

[jl...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

sadpandajoe pushed a commit to branch fix-plain-legend-overlap-timeseries
in repository https://gitbox.apache.org/repos/asf/superset.git

commit 07717dc89ef77258cddd20662be4a102883bb142
Author: sadpandajoe <jc...@gmail.com>
AuthorDate: Tue Aug 18 22:47:31 2026 +0000

    fix(echarts): account for non-legend chart padding
---
 RCA.md                                             | 13 +++++--
 .../src/Timeseries/transformProps.ts               | 16 +++++++++
 .../plugin-chart-echarts/src/utils/series.ts       | 12 +++----
 .../test/Timeseries/transformProps.test.ts         | 40 ++++++++++++++++++++++
 4 files changed, 72 insertions(+), 9 deletions(-)

diff --git a/RCA.md b/RCA.md
index 2f08d431c46..b93969d4257 100644
--- a/RCA.md
+++ b/RCA.md
@@ -39,8 +39,14 @@
 - **Verified:**
   `superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts` replaces
   the horizontal 40% cap with an 80px minimum plot-space floor. It reserves the
-  full estimated legend margin whenever that leaves at least 80px for the plot;
-  for larger legends, it reserves all available height except that floor.
+  full estimated legend margin whenever that leaves at least 80px of genuinely
+  available height for the plot; for larger legends, it reserves the available
+  height except that floor.
+- **Verified:**
+  `superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts`
+  calculates the top and bottom padding required without a legend, including
+  zoom controls and axis-title offsets, and excludes those fixed reservations
+  before applying the plot-space floor.
 - **Verified:** The horizontal path continues to return `LegendType.Plain`; no
   Plain-to-Scroll fallback was added.
 - **Verified:**
@@ -48,6 +54,9 @@
   a 40-item, 800-by-400 regression case whose estimated eleven rows require a
   260px margin, and updates the small-chart overflow expectation to preserve an
   80px plot area.
+- **Verified:** A transform-level regression test covers a 200px-tall zoomable
+  chart with a many-item top Plain legend and verifies that the resulting grid
+  retains 80px of plot height after both top and bottom padding.
 - **Verified:** Vertical Plain legend layout is unchanged.
 
 ## Latent Bugs Found
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 43e9a2f431b..e8daba62ae0 100644
--- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
+++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
@@ -1122,6 +1122,18 @@ export default function transformProps(
     name,
     icon: 'roundRect',
   }));
+  const nonLegendPadding = getPadding(
+    false,
+    legendOrientation,
+    addYAxisLabelOffset,
+    zoomable,
+    undefined,
+    addXAxisLabelOffset,
+    yAxisTitlePosition,
+    convertInteger(yAxisTitleMargin),
+    convertInteger(xAxisTitleMargin),
+    isHorizontal,
+  );
   const getLegendLayout = (candidateLegendMargin?: string | number | null) => {
     const padding = getPadding(
       showLegend,
@@ -1137,6 +1149,10 @@ export default function transformProps(
     );
 
     return resolveLegendLayout({
+      availableHeight: Math.max(
+        height - nonLegendPadding.top - nonLegendPadding.bottom,
+        0,
+      ),
       availableWidth:
         legendOrientation === LegendOrientation.Top ||
         legendOrientation === LegendOrientation.Bottom
diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts b/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
index 8e90cf1eca3..cae2e11535a 100644
--- a/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
+++ b/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
@@ -60,6 +60,7 @@ const DEFAULT_LEGEND_ICON_WIDTH = 25;
 const LEGEND_ICON_LABEL_GAP = 5;
 const LEGEND_HORIZONTAL_SIDE_GUTTER = 16;
 const LEGEND_HORIZONTAL_ROW_HEIGHT = 24;
+// Smallest plot height that still preserves the chart's visible structure.
 const MIN_HORIZONTAL_LEGEND_PLOT_SPACE = 80;
 const MAX_LEGEND_MARGIN_RATIO = 0.4;
 const LEGEND_VERTICAL_SIDE_GUTTER = 16;
@@ -269,13 +270,10 @@ function getHorizontalPlainLegendLayout({
   const requiredMargin =
     defaultLegendPadding[orientation] +
     Math.max(0, rowsForMargin - 1) * LEGEND_HORIZONTAL_ROW_HEIGHT;
-  const boundedMargin =
-    availableHeight > 0
-      ? Math.min(
-          requiredMargin,
-          Math.max(availableHeight - MIN_HORIZONTAL_LEGEND_PLOT_SPACE, 0),
-        )
-      : requiredMargin;
+  const boundedMargin = Math.min(
+    requiredMargin,
+    Math.max(availableHeight - MIN_HORIZONTAL_LEGEND_PLOT_SPACE, 0),
+  );
 
   return {
     effectiveMargin: Math.max(currentMargin, boundedMargin),
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 ff44aa7d3b6..7968bc580c6 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
@@ -37,6 +37,7 @@ import {
 import { GenericDataType } from '@apache-superset/core/common';
 import { supersetTheme } from '@apache-superset/core/theme';
 import type { SeriesOption } from 'echarts';
+import type { GridComponentOption } from 'echarts/components';
 import transformProps from '../../src/Timeseries/transformProps';
 import {
   EchartsTimeseriesSeriesType,
@@ -1035,6 +1036,45 @@ test('honors an explicit List selection for zoomable top legends even when toolb
   expect((transformed.echartOptions.legend as any).type).toBe(LegendType.Plain);
 });
 
+test('preserves plot space for a small zoomable chart with a many-item Plain legend', () => {
+  const chartHeight = 200;
+  const manyLegendValues = Object.fromEntries(
+    Array.from({ length: 40 }, (_, index) => [
+      `Country ${index + 1}, Product`,
+      index + 1,
+    ]),
+  );
+  const chartProps = createTestChartProps({
+    height: chartHeight,
+    formData: {
+      ...formData,
+      legendType: LegendType.Plain,
+      legendOrientation: LegendOrientation.Top,
+      showLegend: true,
+      zoomable: true,
+      yAxisTitleMargin: 0,
+      yAxisTitlePosition: 'Left',
+    },
+    queriesData: [
+      createTestQueryData(
+        createTestData([manyLegendValues], { intervalMs: 300000000 }),
+      ),
+    ],
+  });
+
+  const transformed = transformProps(chartProps);
+  const grid = transformed.echartOptions.grid as GridComponentOption;
+
+  expect((transformed.echartOptions.legend as { type?: LegendType }).type).toBe(
+    LegendType.Plain,
+  );
+  expect(typeof grid.top).toBe('number');
+  expect(typeof grid.bottom).toBe('number');
+  expect(
+    chartHeight - (Number(grid.top) + Number(grid.bottom)),
+  ).toBeGreaterThanOrEqual(80);
+});
+
 test('honors user-selected plain legend type for top orientation when space allows (#39540)', () => {
   // Regression test for issue #39540: switching the legend type control from
   // scroll to plain must reach the rendered ECharts config. Horizontal legends