[Datagrid] Fix rowHeightOption: auto datagrid calculation issue

[kqualters-elastic]

Summary

Resolves #8245

The useUnconstrainedHeight hook used by datagrid has I think a subtle bug in it around auto height rows, in cases where the row height is 'auto' for all the rows in the table, and when datagrid is aware of them early in the multiple renders it uses to settle on a height, the returned value of this hook, an integer representing the height of the datagrid if it were rendered completely in full is returned as a number based on just the header row height, as described in the issue above. The reason for this, I believe, is because RowHeightUtils.getRowHeightOption will return not always a number, but sometimes the string 'auto' as well. That causes the if at https://github.com/elastic/eui/blob/main/packages/eui/src/components/datagrid/utils/grid_height_width.ts#L137 to be true, and as this is the first time we are attempting to determine the height of this row, the heightsCache of the RowHeightUtils class (these really desperately need to be converted to hooks, and a lot of these problems will disappear) returns 0 for all rows when calling getRowHeight, but we increment the knownRowCount anyway, and the result is this bug. The fix in this PR seems to be the most minimal way to fix this, however I think refactoring the class based utils that revolve around EuiDataGridRowHeightOption/EuiDataGridRowHeightsOptions would be best long term, as now multiple unneeded re-renders are forced from callbacks to force checks like useUnconstrainedHeight to be correct, which is bad for performance, but also calculations that happen outside of the React lifecycle, like with the RowHeightUtils class, unexpectedly do not happen at all in some cases, leading to bugs like this. A well done custom hooks approach could solve both problems at once. I have a branch somewhere with EuiDataGridCell converted from a class based component to a functional one to enable this I think.

QA

Remove or strikethrough items that do not apply to your PR.

General checklist

• Browser QA
  • Checked in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Checked for accessibility including keyboard-only and screenreader modes
• Docs site QA
  • Added documentation
  • Props have proper autodocs (using @default if default values are missing) and playground toggles
  • Checked Code Sandbox works for any docs examples
• Code quality checklist
  • Added or updated jest and cypress tests
  • Updated visual regression tests
• Release checklist
  • A changelog entry exists and is marked appropriately.
  • If applicable, added the breaking change issue label (and filled out the breaking change checklist)
• Designer checklist
  • If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)

[kqualters-elastic]

If anyone is interested https://github.com/kqualters-elastic/eui/tree/row-height-debug has console.logs in the places where you can see the calculation fail, adding a height of 0 to the unconstrained height state, and then (incorrectly) incrementing the knownRowCount. For reproducing, happened immediately on my OSX laptop, on Windows desktop, I had to mess with the zoom level of the browser + the size of the window in order to trigger the bug.

[kqualters-elastic]

The failing test is https://github.com/elastic/eui/blob/main/packages/eui/src/components/datagrid/data_grid.spec.tsx#L66 and is probably a bad test IMO. The test is not measuring the unconstrained height from the useUnconstrainedHeight hook, but finalHeight from useFinalGridDimensions. That hook has a height state that would have to be destroyed, or set to 0 at some point, so that this ternary uses unconstrainedHeight

  const finalHeight = isFullScreen
    ? fullScreenHeight
    : height || unconstrainedHeight;

and I think the only reason it passes is because in one of the calculation loops, all heights are 0, similar to the bug in a way.

[acstll]

I can confirm this fixes the original bug where the data grid collapses to 0 (#8245)

I also found a hardcoded height in Storybook, namely in EuiDataGrid / rowHeightsOptions (prop) / Auto, that would be no longer needed:

eui/packages/eui/src/components/datagrid/data_grid_row_heights.stories.tsx

Line 48 in ffd0cbc

height={419} // Required by webkit browsers to not render with 0 height. TODO: Investigate why, this is likely a bug

---

@kqualters-elastic thanks a lot for opening this and sharing your thoughts!

Regarding the test mentioned above, I still need to run it and understand it 🙏 …but I'm guessing here (with the bug fixed) it's failing, and it was passing before but actually asserting the wrong behaviour, correct? — what would you do about it in relation to this PR?

[acstll]

Locally for me computes a new unconstrained height when switching to auto height is passing, but accounts for a horizontal scrollbar is failing… 🤔

[kqualters-elastic]

Locally both are passing for me 😂

[acstll]

Locally both are passing for me 😂

for me as well now haha, the one with the horizontal scrollbar needs a particular system setting while running locally!

This 1-liner fix looks good for me!

I would only create a new issue to investigate/address:

• the flaky logic in useFinalGridDimensions regarding height
• check possible issues with calculated height with autoBelowLineCount enabled
• a possible refactor of class utils, as suggested above
• remove the hard-coded height in data_grid_row_heights.stories.tsx

Let's wait for the team 🙂

[acstll]

This 1-liner fix looks good for me!

after seeing how cypress fails for React 16 and 17, I think this needs another look.

I can confirm the computes a new unconstrained height when switching to auto height test fails for React 16 and 17 (it does not always does in Open mode!).

main

On main, it passes for all 3 React versions and the height is the expected, though there's a small detail that changes (below):

React version	assertion	visually correct?
18	✅ expected 256 to be above 0 ⚠️	yes
17	✅ expected 256 to be above 184	yes
16	✅ expected 256 to be above 184	yes

this branch

On this branch, it passes for React 18 only, and the height is not the expected:

React version	assertion	visually correct?
18	✅ expected 184 to be above 0	no
17	✅ expected 184 to be above 102 (flaky)	no
16	❌ expected 184 to be above 184 (flaky)	no

---

I guess we need to figure out whether the test needs some adjustment or the fix is not good enough?

[tkajtoch]

I'm working on getting these tests fixed, and if there are no other blockers, we should be able to get it merged this week

[logeekal]

I'm working on getting these tests fixed, and if there are no other blockers, we should be able to get it merged this week

Hey @tkajtoch ,

Let us know if you need any assistance from our side in getting these tests fixed.

[mbudge]

The only way to prevent bugs in the SIEM is if someone opens the alerts page in all supported web-browsers before doing a new release.

This was someone all customers were raising at the engineering all hands in Prague last year.

[logeekal]

The only way to prevent bugs in the SIEM is if someone opens the alerts page in all supported web-browsers before doing a new release.

Thanks @mbudge for your revert. We can do the testing but wanted to confirm if anymore changes are required.

One problem with this kind of bug is it is not 100% reproducible so that always leaves room for error. But Security solution will be happy to test it.

[mbudge]

The only way to prevent bugs in the SIEM is if someone opens the alerts page in all supported web-browsers before doing a new release.

Thanks @mbudge for your revert. We can do the testing but wanted to confirm if anymore changes are required.

One problem with this kind of bug is it is not 100% reproducible so that always leaves room for error. But Security solution will be happy to test it.

We have at least 3 different users working in 3 different work environments on 2 browsers affected. Pretty sure someone manually checking the page is working before allowing a new release would mitigate the risk. Automated testing is good but has it's limitations.

[tkajtoch]

To speed things up, I'm going to comment out the flaky tests in this PR and address them separately. It seems that the fix works flawlessly for real users

[kibanamachine]

Preview staging links for this PR:

• Docs site: https://eui.elastic.co/pr_8251/
• Storybook: https://eui.elastic.co/pr_8251/storybook
• New docs: https://eui.elastic.co/pr_8251/new-docs

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: 0460288

History

• 💔 Build #3316 failed 87173cd
• 💔 Build #3307 failed 6793850
• 💔 Build #3278 failed 309e3bb

cc @tkajtoch

[tkajtoch]

Tested locally in EUI and Kibana on main and eui-theme/borealis. Everything works as expected. LGTM

[mbudge]

Event rendered view is still broken in chrome in 8.16.4.

[weronikaolejniczak]

@mbudge I would appreciate more information like the testing path, a video 🙏🏻 I'm following what's said in the issue: #8245 I checked out 8.16 and ran Kibana. This DataGrid has an old picker and when I select "Auto-fit" rows (which is the equivalent to "Auto" in the new picker version) it seems to work perfectly (even without having cherry-picked the commit from this PR):

Screen.Recording.2025-02-13.at.18.04.15.mov

I even checked the EUI version that 8.16 uses - v97.0.0:

Screen.Recording.2025-02-13.at.17.21.16.mov

So this must've been an issue introduced later in the DataGrid.

[kqualters-elastic]

@weronikaolejniczak the fun part about this and all related bugs, is that it only happens in some circumstances for some users, as the browser introduces floating point rounding related errors in some situations, that then make their way into the datagrid's code paths when we use getBoundingClientRect and the like. This bug I never ran into on osx using defaults, but saw right away on windows. Was able to reproduce on osx eventually by changing my browser zoom level to 90%/110%, and/or filtering the table down to 1 entry, and switching the row height type. If neither of those work feel free to slack me and I can help, there are a few more ways I know of that can possibly cause this.

[mbudge]

Refresh the page and select event rendered view without clicking around.

The density option is not set by default.

Apparently the fix is being backported to 8.16.5