-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[draft] Scrolling improvements exploration #9171
Conversation
Netlify deploy previewNetlify deploy preview: https://deploy-preview-9171--material-ui-x.netlify.app/ Updated pagesThese are the results for the performance tests:
|
Below is a video demo of this exploration. The visualization indicates in yellow the screen, in the red selection the blocks we want rendered, in blue/green the blocks currently rendered, where blue means queued for rendering by react, and green means it has been rendered to the DOM. You can also play directly with the component here: https://deploy-preview-9171--material-ui-x.netlify.app/x/react-data-grid/performance/ (there's some raw CSS in there for the prototype, I've only tested in dark mode) The good parts1. Sticky headerIt stays in sync, there is no flickering as we scroll horizontally, feels great, I think we should do it. 2. Dynamic rendered range based on current scroll directionHaving more cells around prevents to an extent white areas when scrolling, and the dynamic range allows to have more cells without paying the cost for cells the user won't see. The implementation could be improved but it's nice to have, and it really diminishes empty areas. 3. Fake scrollbarsWe get control, which is good. I've inspected AG-grid, they seem to be using this technique. I remember google spreadsheets doing it as well. We should use it to improve the scroll-with-scrollbar experience. 4. Using background-image on the container to render the cell bordersThis is nice because it avoids completely empty white areas. This is only possible if the dimensions are known, so no 5. R* trees are cool.title The bad parts6. Rendering outside of reactRendering with 7. Text selection doesn't play nice with tilesI'm not ready to give up on the R*-trees, but it's clear that rows need to be rendered inside a single DOM element to provide good text selection UX. scroll-concept.webm |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
overflow-x: hidden; | ||
} | ||
.fake-scroll-vertical-content { | ||
display: inline-block; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
function afterAnimationFrame(fn: Function) { | ||
requestAnimationFrame(() => { | ||
setImmediate(fn) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of using setImmediate
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The afterAnimationFrame
function is there to allow starting work right after the browser has painted a frame.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aaah, so requestAnimationFrame
executes before the paint, and then you use setImmediate
to schedule a microtask that will be run after the paint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I've lifted the setImmeditate
implementation from React's code, they use MessageChannel
ports which seems to be the best method for an immediate callback. I think it's a task, not a microtask, possibly because a microtask would run after the RAF handlers but before the paint.
const root = ReactDOM.createRoot(element) | ||
root.render( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, I think this is the first time I see multiple React roots being used like this.
I wonder how would we update <Block/>
components?
I created this demo to see if I can share context between multiple React roots, and it didn't work:
https://codesandbox.io/s/epic-framework-y9cmmx?file=/src/App.tsx
Am I missing something here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to update blocks with this implementation, they need mounting & unmounting only. The reactive updates of the grid would be handled by our existing mechanism with useGridSelector
, which can work even across roots.
However, it's true that custom user context doesn't work across roots.
An alternative here could be to use portals instead of roots.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, it's true that custom user context doesn't work across roots.
I guess roots are a no-go then - they need to have access to the theme context among others:
https://codesandbox.io/s/eager-jennings-yxcjcz?file=/src/App.tsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative here could be to use portals instead of roots.
Is it possible to imperatively mount/unmount portals though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess roots are a no-go then - they need to have access to the theme context among others:
Depends if we accept to support only some contexts. It's easy to manually re-wrap the subroots with more contexts such as the theme one, but there is no general solution to support all contexts afaik.
Is it possible to imperatively mount/unmount portals though?
Not sure, probably not :|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's easy to manually re-wrap the subroots with more contexts such as the theme one, but there is no general solution to support all contexts afaik.
Yeah, that would work for the MUI theme context.
But it won't be possible to use other component libraries that rely on context in cell renderer (Material UI could be used with other component libraries)
<React.Fragment> | ||
{instance.rows.slice(rowIndex, rowIndex + BLOCK_ROW_SIZE).map((row, i) => | ||
<div className='row' key={i}> | ||
{instance.columns.slice(columnIndex, columnIndex + BLOCK_COL_SIZE).map((column, j) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is interesting, the column header is not virtualized here.
I was wondering how would this approach scale with a large number of columns.
I tried to apply this idea to our current virtualization engine in #9589
Here is a demo with 1000 columns: https://deploy-preview-9589--material-ui-x.netlify.app/x/react-data-grid/virtualization/#column-virtualization
- The initial render is laggy since we need to render 1000 columns at once.
- The horizontal scrolling feels pretty good - the column header stays in sync with the content
- Interactions with column header are super slow - e.g. keyboard navigation, column menu, sorting.
This is probably due to the fact that we rerender the whole column header on state change. We could optimize it with React.memo and only rerender a single cell at once, but this could be a bottleneck.
Overall, a non-virtualized column header feels risky to me.
What do you think?
Did you explore the idea of column header virtualization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't implement it to keep things simple, but there isn't any issue with doing header virtualization and sticky headers.
However, we might want to disable header virtualization under a certain threshold (something like 100?). Rendering 100 column headers upfront isn't an issue, and we avoid some costs when scrolling.
This is an experimental branch to explore some of the rendering & scheduling ideas discussed in #9001 in a reimplementation of the core DataGrid virtualization logic.
Nothing to see yet.See last comment for results & demo link.Architectural differences
position: sticky
, so they're never out of sync with the body of the grid. We also get scrolling-on-headers for free.ReactDOM.createRoot(...)
on each new tile, this hopefully will avoid the react diffing cost on scrolling, which is the place where we need to save as many cycles as possible.background-image
cell/row borders