Fix invalid calculation of chart dimensions caused collapsing (#16717)

* Fix invalid calculation of chart dimensions caused collapsing

fix cap calculation for row bar chart

* fix typo

* add spec for null

frontend/src/metabase/visualizations/lib/RowRenderer.js

@@ -120,7 +120,6 @@ export default function rowRenderer(
     .ordering(d => d.index);
 
   const labelPadHorizontal = 5;
-  const labelPadVertical = 1;
   let labelsOutside = false;
 
   chart.on("renderlet.bar-labels", chart => {
@@ -159,25 +158,16 @@ export default function rowRenderer(
     chart.margins().bottom += axisLabelHeight;
   }
 
-  // cap number of rows to fit
-  const rects = chart.selectAll(".row rect")[0];
-  const containerHeight =
-    rects[rects.length - 1].getBoundingClientRect().bottom -
-    rects[0].getBoundingClientRect().top;
-  const maxTextHeight = Math.max(
-    ...chart
-      .selectAll("g.row text")[0]
-      .map(e => e.getBoundingClientRect().height),
-  );
-  const rowHeight = maxTextHeight + chart.gap() + labelPadVertical * 2;
-  const cap = Math.max(1, Math.floor(containerHeight / rowHeight));
+  const { top, bottom } = chart.margins();
+  const boundsHeight = chart.height() - top - bottom;
+  const rowHeight = (boundsHeight - chart.gap()) / group.size();
+  const barHeight = rowHeight - chart.gap();
+  const cap = Math.max(1, Math.floor(boundsHeight / ROW_MAX_HEIGHT));
   chart.cap(cap);
 
-  // assume all bars are same height?
-  const barHeight = chart.select("g.row")[0][0].getBoundingClientRect().height;
   if (barHeight > ROW_MAX_HEIGHT) {
-    const reasolableMaxGap = containerHeight / 3;
-    chart.gap(Math.min(barHeight / 2, reasolableMaxGap));
+    const reasonableMaxGap = boundsHeight / 3;
+    chart.gap(Math.min(barHeight / 2, reasonableMaxGap));
   }
 
   chart.render();

frontend/test/metabase/scenarios/visualizations/rows.cy.spec.js

@@ -8,27 +8,38 @@ describe("scenarios > visualizations > rows", () => {
 
   // Until we enable multi-browser support, this repro will be skipped by Cypress in CI
   // Issue was specific to Firefox only - it is still possible to test it locally
-  it(
-    "should not collapse rows when last value is 0 (metabase#14285)",
-    { browser: "firefox" },
-    () => {
-      cy.createNativeQuestion({
-        name: "14285",
-        native: {
-          query:
-            "with temp as (\n select 'a' col1, 25 col2\n union all \n select 'b', 10\n union all \n select 'c', 15\n union all \n select 'd', 0\n union all\n select 'e', 30\n union all \n select 'f', 35\n)\nselect * from temp\norder by 2 desc",
-          "template-tags": {},
-        },
-        display: "row",
-      }).then(({ body: { id: QUESTION_ID } }) => {
-        cy.visit(`/question/${QUESTION_ID}`);
-      });
+  ["0", "null"].forEach(testValue => {
+    it(
+      `should not collapse rows when last value is ${testValue} (metabase#14285)`,
+      { browser: "firefox" },
+      () => {
+        cy.createNativeQuestion({
+          name: "14285",
+          native: {
+            query: `
+              with temp as (
+                select 'a' col1, 25 col2 union all
+                select 'b', 10 union all
+                select 'c', 15 union all
+                select 'd', ${testValue} union all
+                select 'e', 30 union all
+                select 'f', 35
+              ) select * from temp
+              order by 2 desc
+            `,
+            "template-tags": {},
+          },
+          display: "row",
+        }).then(({ body: { id: QUESTION_ID } }) => {
+          cy.visit(`/question/${QUESTION_ID}`);
+        });
 
-      cy.get(".Visualization").within(() => {
-        ["a", "b", "c", "d", "e", "f"].forEach(letter => {
-          cy.findByText(letter);
+        cy.get(".Visualization").within(() => {
+          ["a", "b", "c", "d", "e", "f"].forEach(letter => {
+            cy.findByText(letter);
+          });
         });
-      });
-    },
-  );
+      },
+    );
+  });
 });