[EuiDataGrid] Use integer values for dimensions derived from integers

[kqualters-elastic]

Bug repro steps

• In any browser, set your browser zoom level to anything above 100%, e.g. 110%
• Go to https://eui.elastic.co/v80.0.0/#/tabular-content/data-grid
  • (NOTE: If you zoom in while on this page, you must refresh the page first so the page loads in at 110% zoom)
• Notice that the first example has a scrollbar (when it should not)
• Go to the pagination Rows per page in the bottom left corner of the grid, and select 50
• Notice that the datagrid does not dynamically update its height when it should, it stays stuck as scrolling

Issues affected by this bug

• [Security Solution] Single alert may be hidden due to table height kibana#159143
• [Security][RAM] Fix Cases > Alerts data grid height rendering in as 0 kibana#134268
• and a few others

Summary of problem

Under certain circumstances, such as when zoom is not 100%, OS level font/zoom levels are not default, child components having a transform actively applied, browser extensions, and probably a whole lot of other scenarios, getBoundingClientRect().height/width will return a decimal number. When these close but not exactly the same values are compared with a value derived from a static number in javascript, like with row height from https://github.com/elastic/eui/blob/main/src/components/datagrid/utils/row_heights.ts#L40, pixel math will break. I believe what's happening is this:

useFinalGridDimensions will calculate a height/width and apply it to the Grid component, this calculation is done by storing a height/width in hook state, and setting it to a getBoundingClientRect().height/width if those measured values do not match the value from useUnconstrainedHeight. This normally works, as useUnconstrainedHeight is first static 34 when row heights are default, and no scrollbar/header row/footer row has been rendered yet, state is set to 34, and both the EUI components and the grid rerender, and the process happens again. This time with the static 34, and a scrollbar of 16, then once more with a static 34 + 16 + 30 from the header, etc. With integer pixel values, everything is good.

Two things ultimately make the bug happen: the first is the pixel math and getBoundingClientRect resulting in decimal values, and the second is that the useEffect that updates the state will run on either height or width changing, and potentially update them both. Sometimes they both are updated, in other cases just the height or width, and when the bug triggers, the effect will typically only update the width, the unconstrainedHeight has a value of 50, but for some reason the container is measured as 49.9999997 or something, and so the container height is set to this lower value. From here, no matter what you do, add more rows, change the page etc, the Grid will keep the 49.9997 value as it's height, making the table not usable.

QA

• In any browser, set your browser zoom level to anything above 100%, e.g. 110%
• Go to https://eui.elastic.co/pr_6895/#/tabular-content/data-grid
  • (NOTE: If you zoom in while on this page, you must refresh the page first so the page loads in at 110% zoom)
• Confirm that the first example does not have a scrollbar
• Go to the pagination Rows per page in the bottom left corner of the grid, and select 50
• Confirm that the data grid does correctly update its height dynamically and adding more rows does not create a scrollbar

General checklist

• Checked in mobile / resizing screens
• Checked in Chrome, Safari, Edge, and Firefox
• [ ] Added or updated jest and cypress tests
  • There isn't really an automated test we can add here unfortunately as browser zoom level isn't something we can set w/ Cypress 😢
• A changelog entry exists and is marked appropriately

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_6895/

[cee-chen]

@kqualters-elastic Apologies for totally missing this PR until now - I literally just didn't see the GH notification go by 🤦

Let me know what you think about totally removing getBoundingClientRect() - if you see a reason not to, no worries, we can move ahead with this PR as-is!

I did a bit of logging and unfortunately unconstrainedHeight can also return a decimal float, so we need to round both values in order for zoom shenanignans to behave. 🤦

[cee-chen]

@kqualters-elastic So unfortunately I'm testing https://eui.elastic.co/pr_6895/#/tabular-content/data-grid on Edge/Safari/etc at non-100% zoom and heights are still off/incorrect for me compared to 100% zoom.

I think I have a fix locally after some playing around with behavior - let me push up what I have and get your thoughts on it.

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_6895/

[kqualters-elastic]

tested locally in kibana, everything lgtm from the ways I was previously able to reproduce it 👍

Couple thoughts: can easily write unit tests for all these looks by mocking the refs/getBoundingClientRect calls. Also think long term it would be a good idea to rework making all of these calculations in a series of useEffect/useState -> set a prop on a ref that causes the ref to resize, triggering the whole process over again. Same calculations could be done in fewer steps I think, because there are quite a few custom hooks that return a referentially different value every render, inline functions or variables defined in the render that change every time, etc etc that should either be memoized more aggressively, or the parent components that they are used in not forcing rerenders so casually.

[cee-chen]

Couple thoughts: can easily write unit tests for all these looks by mocking the refs/getBoundingClientRect calls.

I'm slightly marginal on writing unit tests for a browser-level bug/edge case; I'd personally rather know that the bug we're attempting to resolve is solved, and mocking getBoundingClientRect() doesn't really help us assert anything other than that Math.round works or that an if conditional worked.

That being said if you feel strongly about adding a test to this PR before merge definitely let me know, otherwise I think the lengthy dev comment (hopefully) suffices in terms of future devs not accidentally regressing the check or behavior.

Also think long term it would be a good idea to rework [...] memoiz[ing] more aggressively, or the parent components that they are used in not forcing rerenders so casually.

Totally agreed - EuiDataGrid is complex at best and a nightmareish behemoth at worst 😅 Just to add some context around the custom hooks, they were added in #5506 because literally everything was being handled all in 1-2 files and it was frankly super hard to read/grok, let alone write unit tests for (and back then, unit tests were even more missing than they are today). It's definitely still not perfect, but at least concerns are separated by file which should hopefully make it easier to isolate problem areas.

I'd be super happy to take PRs on any perf improvements or memoization cleanups you'd like to see happen - feel free to assign any datagrid PRs to me!

[cee-chen]

Feel free to shout if you think this fix needs a test written for it, otherwise I think we can merge this in and get it into Kibana next week 🤞

[kqualters-elastic]

was thinking about a test that mimics how this bug comes about, where the first few updates to the hook contain a ref with the same dimensions as useUnconstrainedHeight, and then on the fourth or fifth update have the hook mocked as an integer and the ref being slightly smaller, but agree that's not testing much. merge it imo