Skip to content

Virtualize: Refresh when estimated item size changes. Fixes #25915 #26268

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

Closed

Conversation

SteveSandersonMS
Copy link
Member

This is a potential fix for #25915.

This PR is so we can have a conversation about whether to include this fix in 5.0. Personally, I don't think it's a clear-cut decision. We could argue either way. Obviously it's generally good to fix issues, but:

  • The issue only occurs in relatively special cases (when the actual item height is very close to the default height of 50px, but not exactly equal to 50px). It's an unfortunate coincidence that the rows in the FetchData table in the default template happen to be 48.89px high!
  • This issue has a fairly straightforward (albeit a bit inconvenient) workaround: developers can supply an ItemHeight value to <Virtualize> instead of relying on it auto-detecting the height. If they do that, the problem won't occur, as far as I can tell.
  • The fix in this PR is the sort of thing I'd really prefer to give quite a bit of bake-time for, and let people try it out in preview releases for a while, before being totally confident it will work everywhere and doesn't have any negative side-effects. In particular I'm nervous about whether this change could somehow cause extra unnecessary rendering, or even a loop of repeated rendering.
  • My gut sense is this is not the last quirk we're going to discover in <Virtualize>. As we all know, virtualization is fundamentally pretty hard to do and cover every edge case when it's not a native feature of the underlying UI technology. I think what we've got is really good and useful, but we should be prepared to refine it further as time goes on.

@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Sep 24, 2020
@SteveSandersonMS SteveSandersonMS changed the base branch from master to release/5.0-rc2 September 24, 2020 10:04
@SteveSandersonMS SteveSandersonMS requested a review from a team September 24, 2020 10:04
@SteveSandersonMS SteveSandersonMS changed the title Refresh when estimated item size changes. Fixes #25915 Virtualize: Refresh when estimated item size changes. Fixes #25915 Sep 24, 2020
// At this point, something unusual has occurred, likely due to misuse of this component.
// Reset the calculated item size to the user-provided item size.
_itemSize = ItemSize;
itemSizeChanged = false;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Some of the refactoring here isn't strictly necessary, but I've tidied up the operations so it only deals with the "item size changed" steps in cases where we might be assigning new values to _itemSize. These refactorings are very safe AFAIK.


// We don't want to create an infinite rendering loop due to floating point precision limits,
// so only re-render if the item size change is big enough
itemSizeChanged = Math.Abs(_itemSize - previousItemSize) > 0.5f;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the bit I'm a bit nervous about. Might there be some edge case where we create an infinite loop? That is, on every render we compute a slightly different item size, perhaps alternating between two values.

I'm pretty sure I could cause such a problem deliberately if I wanted by putting in some pathological CSS that varies the sizes weirdly. Whether or not that scenario happens to innocent developers acting in good faith is harder to guess.

@SteveSandersonMS
Copy link
Member Author

I'm closing this in favour of #26933

The other PR (#26933) has what I think is a safer fix and doesn't make me worry that there could be some subtle combination of conditions that lead to an infinite loop (whereas this one, #26268, does make me concerned about that).

@dougbu dougbu deleted the stevesa/fix-virtualize-getting-stuck branch August 21, 2021 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant