petyosi / react-virtuoso

The most powerful virtual list component for React
https://virtuoso.dev
MIT License
5.16k stars 299 forks source link

[BUG] Double scrollbar on table with useWindowScroll and horizontally scrollable container #998

Open mihkeleidast opened 10 months ago

mihkeleidast commented 10 months ago

Describe the bug Scenario:

Reproduction https://codesandbox.io/s/dreamy-hill-vvjgd2?file=/styles.css

To Reproduce Steps to reproduce the behavior:

  1. Open the sandbox (may need a scaled display or zooming, as it comes down to rounding errors)
  2. See double vertical scrollbar

Expected behavior There should be only a single scrollbar in either direction.

Screenshots

double vertical scrollbar ![image](https://github.com/petyosi/react-virtuoso/assets/1892091/272be4de-7b31-4c96-8fe6-eea2cb1278e4)

Desktop (please complete the following information):

Additional context I initially discovered this on an a scaled display. However, after playing around with some CSS, also reproduced it on a 100% scaled display.

This may happen because of row height rounding. E.g. when inspecting, some rows have data-known-size of 50, while they are actually rendered at 50.19px.

The scroller height is set at 50020px, while its scrollHeight is 50025px. The difference comes up just at about 26 (rows rendered) x 0.19px = 4.94px.

I know I can work around this by setting overfow-y: hidden; on the horizontal scroll container, but thought it's still best to report this.

petyosi commented 10 months ago

Hey @mihkeleidast ,

what you describe makes sense, thank you for sharing the workaround. I've gone back and forth on rounding/not-rounding floating item sizes, since both approaches have their tradeoffs. Leaving a trace of the last commit that touched that: https://github.com/petyosi/react-virtuoso/commit/9127413161580c0d1bb8e2301beb74e272b23a33.

I might give this some more tries at some point, but the ultimate culprit here is that you can't call scrollTo with a floating value.

mihkeleidast commented 10 months ago

I wonder if ceiling instead of simple rounding would work better for some calculations? Or, keeping track of the diff of rounded/non-rounded heights and adding that deviation to the scroller height as well?

petyosi commented 10 months ago

I don't know. It's quite the case to solve, maybe the rounding should come at the scrollTo/scrollBy calls. You have some experience with the internals of the library, and the commit actually includes a test that denotes the problems discovered when rounding was not applied. Happy to accept a PR.