[DataGrid] Fix first row flickering with autoHeight enabled#14235
[DataGrid] Fix first row flickering with autoHeight enabled#14235kenanyusuf merged 8 commits intomui:masterfrom
Conversation
|
Deploy preview: https://deploy-preview-14235--material-ui-x.netlify.app/ Updated pages: |
kenanyusuf
added
type: bug
packages/x-data-grid/src/hooks/features/virtualization/useGridVirtualization.tsx
Outdated
Show resolved
Hide resolved
kenanyusuf
force-pushed
the
fix-auto-height-row-flicker
branch
from
108a2ef to
b6555c2
Compare
packages/x-data-grid/src/hooks/features/virtualization/useGridVirtualScroller.tsx
Outdated
Show resolved
Hide resolved
packages/x-data-grid/src/hooks/features/virtualization/useGridVirtualization.tsx
Outdated
Show resolved
Hide resolved
packages/x-data-grid/src/hooks/features/virtualization/useGridVirtualization.tsx
Outdated
Show resolved
Hide resolved
…Virtualization.tsx Co-authored-by: Bilal Shafi <bilalshafidev@gmail.com> Signed-off-by: Kenan Yusuf <kenan.m.yusuf@gmail.com>
| ...state.virtualization, | ||
| enabled, | ||
| enabledForColumns: enabled, | ||
| enabledForRows: enabled && !props.autoHeight, |
There was a problem hiding this comment.
I'm not sure how to feel about putting the !prop.autoheight logic in the state setter. I would have maybe gone for keeping the state setter as simple as possible. But I also don't find a satisfying place to put it, so I don't know.
There was a problem hiding this comment.
Could we move the !autoheight somewhere it doesn't need to be repeated?
There was a problem hiding this comment.
No, feel free to ignore if there's no other good place.
| } = params || {}; | ||
|
|
||
| const firstColumnToRender = !hasVirtualization ? 0 : currentContext.firstColumnIndex; | ||
| const firstColumnToRender = currentContext.firstColumnIndex; |
There was a problem hiding this comment.
There was a bug here. hasVirtualization is using gridVirtualizationColumnEnabledSelector to get the column virtualization state. This was previously always true, even when the disableVirtualization prop was passed to DataGrid. The 0 here meant that pinned column headers were duplicated were not being removed from the middle section of columns.
|
Thank you guys for bashing this bug ! Our users will be happy to have this fixed 🙏 !! |
5 participants
Fixes the first row flickering when a new, editable row is added to a grid with autoHeight enabled.
Fixes #13270
The problem
1. User adds a new row
2. The new row gets rendered in the correct position, but the original row is hidden
This happens because the newly added editable cell calls
.focus()when it mounts. The virtual scroller has ascrollevent listener that listens to scroll events in order to recalculate which rows are in view to render. The browser tries to scroll the editable cell into view, and triggers the scroll event callback.The scroll event handler receives an
event.scrollTopposition of52px, and decides that the first row does not need rendering because based on this value, it is out of view. This is what leads to the temporary state of a hidden first row.Why is this only a problem with autoHeight enabled?
When
autoHeightis not enabled, the scroll event listener only fires when the virtual scroller content actually becomes scrollable. This means the editable cell moves into focus and there is no visible flicker.With
autoHeightenabled, the virtual scroller content temporarily overflows when a new row is added whilst it recalculates the virtual scroller content height.3. The virtual scroller recalculates and re-renders both rows in the correct position
The solution
Disable row virtualization when
autoHeightis enabled. Row virtualization has no performance gains whenautoHeightis enabled as the list of rows never becomes scrollable. This in turn fixes the issue of the virtual scroller miscalculating which elements are in view.