[DataGrid] Fix scroll jumping #14929
Conversation
romgrk
added
the
component: data grid
|
Deploy preview: https://deploy-preview-14929--material-ui-x.netlify.app/ |
There was a problem hiding this comment.
As I'm fixing/refactoring the 'auto' height code, I'm seeing stuff that could be removed. I see that there is a .resized flag that is never used, except in unstable_setRowHeight which is itself not used afaict. It was added in #3751. @cherniavskii, can we remove this method and its associated data?
There was a problem hiding this comment.
It's a programmatic API that developers can use. I'm not sure if it's actually being used though, it could be useful for implementing row resizing.
I prefer keeping it if it's not causing issues.
There was a problem hiding this comment.
I don't like much keeping code that's unused, it makes reading & understanding the rest of the file more tedious. For example, the isResized condition increases the cyclomatic complexity of the calculateRowProcessedSizes function.
But should we consider removing the unstable_ prefix from those methods, if we're going to keep them?
There was a problem hiding this comment.
Also note that the unstable_setRowHeight method is marked as @ignore - do not document in addition to unstable_, thus why I suggested removing it.
There was a problem hiding this comment.
unstable_setRowHeight method is marked as
@ignore - do not document
Fair enough, then I'm OK with removing it 👍🏻
There was a problem hiding this comment.
I think deleting code is my favourite part of programming.
There was a problem hiding this comment.
How about fixing a regression caused by a deleted code 😆
romgrk
force-pushed
the
fix-scroll-jump
branch
from
b4083c4
to
58d6d96
Compare
| const resizeObserver = useLazyRef( | ||
| () => | ||
| new ResizeObserver((entries) => { | ||
| for (let i = 0; i < entries.length; i += 1) { | ||
| const entry = entries[i]; | ||
| const height = | ||
| entry.borderBoxSize && entry.borderBoxSize.length > 0 | ||
| ? entry.borderBoxSize[0].blockSize | ||
| : entry.contentRect.height; | ||
| const rowId = (entry.target as any).__mui_id; | ||
| apiRef.current.unstable_storeRowHeightMeasurement(rowId, height); | ||
| } | ||
| if (!isHeightMetaValid.current) { | ||
| apiRef.current.requestPipeProcessorsApplication('rowHeight'); | ||
| } | ||
| }), | ||
| ).current; |
There was a problem hiding this comment.
I moved this here because ResizeObserver was designed to be one-listener-many-elements, the performance cost of using one per element is illustrated by the chromium authors here.
This also simplifies the logic as the N resize notifications come at once, so the whole debounce logic is not needed anymore. Debounce was the problem with scroll jumping. We had the data to avoid jumps, but we weren't using it because the debounce was keeping the invalidated data in use.
| // HACK: rowHeight trails behind the most up-to-date value just enough to | ||
| // mess the initial rowsMeta hydration :/ | ||
| const baseRowHeight = gridDimensionsSelector(apiRef.current.state).rowHeight; |
There was a problem hiding this comment.
@cherniavskii The recent change you made to pull rowHeight from useGridSelector coupled with the changes of this PR created this issue :| It's a symptom of this.
| rowsHeightLookup.current = {}; | ||
| const resetRowHeights: GridRowsMetaApi['resetRowHeights'] = () => { | ||
| heightCache.clear(); |
There was a problem hiding this comment.
We used Object.create(null) when creating the lookup but not when resetting it here. Using Map for lookups avoids the issues with {} (prototype injection, and presence of Object methods), and is also more performant.
| React.useEffect(() => { | ||
| useEnhancedEffect(() => { |
There was a problem hiding this comment.
This was causing subtle bugs, because some of our hook effects use useEnhancedEffect (aka React.useLayoutEffect), they were running before the API methods were updated, so they were calling outdated useCallback instances.
|
The argos changes all look positive to me, they show the grid reacting to |
There was a problem hiding this comment.
Sidenote but the detail panels code is terribly unefficient. IIUC, it calls getDetailPanel(Height|Content) for all the rows of the grid, regardless if they're displayed or not. For large datasets, the performance must be horrible.
Closes #14802
Before: https://codesandbox.io/p/sandbox/thirsty-sun-qcstlj