Skip to content
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

bug(cdk/scrolling): Append only mode together with FixedSizeVirtualScrollStrategy results in a jumping view #23577

Closed
philippspindler opened this issue Sep 13, 2021 · 2 comments · Fixed by #24153
Labels
area: cdk/scrolling P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent

Comments

@philippspindler
Copy link

Reproduction

See the example in the official material documentation
https://material.angular.io/cdk/scrolling/overview#cdk-virtual-scroll-append-only

  1. Scroll down
  2. Scrolling is delayed and the correct position is only set after a delay
Screen.Recording.2021-09-13.at.15.43.49.mov

Note: This effect becomes even more apparent when the rendering of an item is more expensive!

Expected Behavior

Virtual scrolling with appendOnly sets the correct position without jumping to the top.

Actual Behavior

The virtual scroll using appendOnly jumps for a certain time to the top of the scrolling container. After this period of time, the correct scroll position is set.

A possible solution

I was able to solve this issue by implementing a CustomScrollingStrategy that copy-pastes FixedSizeScrollingStrategy and adapts line 171.

- this._viewport.setRenderedContentOffset(this._itemSize * newRange.start);
+ this._viewport.setRenderedContentOffset(0, 'to-start');

I am missing the insights to be able to open a pull request fixing this issue respecting all different use cases of virtual scrolling with and without appendOnly. So I am open to any kind of help to fix this issue.

Environment

  • Angular: 12.2.5
  • CDK/Material: 12.2.5
  • Browser(s): Chrome
  • Operating System (e.g. Windows, macOS, Ubuntu): macOS
@philippspindler philippspindler added the needs triage This issue needs to be triaged by the team label Sep 13, 2021
@crisbeto crisbeto added area: cdk/scrolling P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent and removed needs triage This issue needs to be triaged by the team labels Sep 15, 2021
@chabb
Copy link
Contributor

chabb commented Dec 28, 2021

@philippspindler Thanks for opening this issue and posting a fix. I've encountered the same issue with the same fix. The issue with your fix is that it might break things when appendOnly is set to false ( the calculations of the rendered content offset is going to be incorrect in that case ) , and I do not think it takes in account the other issue you've opened #23578

The trick is to get the updated rendered change before setting the offset, that way, you'll get the correct range in all cases ( appendOnly being false or true ), and I believe it will take in account the fix you suggested in your other issue ( though I did not test that particular case )

Here is the fix

- this._viewport.setRenderedContentOffset(this._itemSize * newRange.start);
+ const newRangeStart = this._viewport.getRenderedRange().start;
+ this._viewport.setRenderedContentOffset(this._itemSize * newRangeStart);

It would be a nice fix to have, as using appendOnly results in a bad user experience when a list has more complex items; the user sees the same items when he scrolls down, which is very confusing.

Sometimes you are 'forced' to use appendOnly; for instance, let's suppose you are using a third-party list that relies on the list-items that has been rendered for managing the selection status of the items in the list; if you use cdk-virtual-scroll, the only way to make it work is to set appendOnly to true, otherwise you'll break the correct selection state

@MichaelJamesParsons as you added the appendOnly flag in 8f052cc,, and @mmalerba as ( I think ) your are the main author for the virtual-scrolling, may I ask you whether this fix makes sense and if it's worth creating a PR ? Thanks

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: cdk/scrolling P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants