Skip to content

Fix incremental layout with variable heights#6588

Merged
devongovett merged 1 commit into
mainfrom
fix-incremental-layout
Jun 21, 2024
Merged

Fix incremental layout with variable heights#6588
devongovett merged 1 commit into
mainfrom
fix-incremental-layout

Conversation

@devongovett
Copy link
Copy Markdown
Member

Reported by @rostero1 in #6518 (comment)

In ListLayout with variable row heights, sometimes a large empty space could appear at the end of the list. This was due to the validRect not being invalidated properly when the height of an item changed. When this occurs, all items after that item need to be repositioned.

For clarity, I've also renamed two properties:

  • lastValidRect is now called validRect - this is the rectangle that is currently valid. We can reuse previously laid out items within this rectangle.
  • validRect is now called requestedRect - this is the rectangle that has been requested for layout so far. After a layout pass, validRect will equal requestedRect.

@rspbot
Copy link
Copy Markdown

rspbot commented Jun 20, 2024

@rspbot
Copy link
Copy Markdown

rspbot commented Jun 20, 2024

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

Copy link
Copy Markdown
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rendering of the items looks good. I'm seeing interesting behavior when scrolling quickly, towards the end of the list, with variable height enabled. Although once all the items get rendered once, it doesn't reproduce. This is MacOS Chrome.

Screen.Recording.2024-06-21.at.2.42.16.PM.mov
Screen.Recording.2024-06-21.at.2.47.01.PM.mov

Copy link
Copy Markdown
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The behavior that @reidbarber saw is interesting, I suppose that is because we don't actually know the heights of the rows that haven't been actually rendered and thus are using the estimated row height to calculate a theoretical scroll height for the scrollable region? Then as more rows render as you scroll down, the actual total scroll height grows, meaning your mouse's drag position relative to the scrollbar is incrementally no longer in sync with the actual scroll position? Looks like we actually have the same problem in RSP TableView if overflowMode: wrap is applied with highly variable row heights.

@devongovett
Copy link
Copy Markdown
Member Author

devongovett commented Jun 21, 2024

Yeah exactly. That has always been the case. I don't think there is much we can do about it. This example is probably worse than normal because the estimatedRowHeight is quite inaccurate from the real row height so it bounces around more. Users should try to estimate as accurately as they can to avoid it.

@devongovett devongovett merged commit 4e3af37 into main Jun 21, 2024
@devongovett devongovett deleted the fix-incremental-layout branch June 21, 2024 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants