-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Fix the bug with scrolling in Virtualize component with scaled elements #64013
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -94,16 +94,60 @@ function init(dotNetHelper: DotNet.DotNetObject, spacerBefore: HTMLElement, spac | |
// scrolling glitches. | ||
rangeBetweenSpacers.setStartAfter(spacerBefore); | ||
rangeBetweenSpacers.setEndBefore(spacerAfter); | ||
const spacerSeparation = rangeBetweenSpacers.getBoundingClientRect().height; | ||
const rangeRect = rangeBetweenSpacers.getBoundingClientRect(); | ||
const spacerSeparation = rangeRect.height; | ||
const containerSize = entry.rootBounds?.height; | ||
|
||
// Accumulate scale factors from all parent elements as they multiply together | ||
let scaleFactor = 1.0; | ||
|
||
// Check for CSS scale/zoom/transform properties on parent elements (including body and html) | ||
let element = spacerBefore.parentElement; | ||
while (element) { | ||
const computedStyle = getComputedStyle(element); | ||
|
||
// Check for zoom property (applies uniform scaling) | ||
if (computedStyle.zoom && computedStyle.zoom !== 'normal' && computedStyle.zoom !== '1') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have a check for:
DOM size can be also changed using width/height directly. Does it also affect the scrolling?
ilonatommy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
scaleFactor *= parseFloat(computedStyle.zoom); | ||
} | ||
|
||
// Check for scale property (can have separate X/Y values) | ||
if (computedStyle.scale && computedStyle.scale !== 'none' && computedStyle.scale !== '1') { | ||
const parts = computedStyle.scale.split(' '); | ||
const scaleX = parseFloat(parts[0]); | ||
const scaleY = parts.length > 1 ? parseFloat(parts[1]) : scaleX; | ||
scaleFactor *= scaleY; // Use vertical scale for vertical scrolling | ||
ilonatommy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
// Check for transform property (matrix form) | ||
if (computedStyle.transform && computedStyle.transform !== 'none') { | ||
const match = computedStyle.transform.match(/matrix\(([^,]+),\s*([^,]+),\s*([^,]+),\s*([^,]+)/); | ||
dariatiurina marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
if (match) { | ||
const scaleX = parseFloat(match[1]); | ||
const scaleY = parseFloat(match[4]); | ||
if (Math.abs(scaleX - 1.0) > 0.01 || Math.abs(scaleY - 1.0) > 0.01) { | ||
dariatiurina marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
scaleFactor *= scaleY; // Use vertical scale | ||
} | ||
} | ||
} | ||
element = element.parentElement; | ||
} | ||
|
||
// Divide by scale factor to convert from physical pixels to logical pixels. | ||
const unscaledSpacerSeparation = spacerSeparation / scaleFactor; | ||
const unscaledContainerSize = containerSize !== null && containerSize !== undefined ? containerSize / scaleFactor : null; | ||
|
||
if (entry.target === spacerBefore) { | ||
dotNetHelper.invokeMethodAsync('OnSpacerBeforeVisible', entry.intersectionRect.top - entry.boundingClientRect.top, spacerSeparation, containerSize); | ||
const spacerSize = entry.intersectionRect.top - entry.boundingClientRect.top; | ||
const unscaledSpacerSize = spacerSize / scaleFactor; | ||
dotNetHelper.invokeMethodAsync('OnSpacerBeforeVisible', unscaledSpacerSize, unscaledSpacerSeparation, unscaledContainerSize); | ||
} else if (entry.target === spacerAfter && spacerAfter.offsetHeight > 0) { | ||
// When we first start up, both the "before" and "after" spacers will be visible, but it's only relevant to raise a | ||
// single event to load the initial data. To avoid raising two events, skip the one for the "after" spacer if we know | ||
// it's meaningless to talk about any overlap into it. | ||
dotNetHelper.invokeMethodAsync('OnSpacerAfterVisible', entry.boundingClientRect.bottom - entry.intersectionRect.bottom, spacerSeparation, containerSize); | ||
const spacerSize = entry.boundingClientRect.bottom - entry.intersectionRect.bottom; | ||
const unscaledSpacerSize = spacerSize / scaleFactor; | ||
dotNetHelper.invokeMethodAsync('OnSpacerAfterVisible', unscaledSpacerSize, unscaledSpacerSeparation, unscaledContainerSize); | ||
} | ||
}); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -426,6 +426,18 @@ public void CanRefreshItemsProviderResultsInPlace() | |
name => Assert.Equal("Person 3", name)); | ||
} | ||
|
||
[Fact] | ||
public void CanScrollWhenAppliedScale() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this fail without the change? The check that I used for detecting the glitch was much more complex. Some solutions did not prevent the full loading of data but still, resulted in the glitch on the way to the bottom. |
||
{ | ||
Browser.MountTestComponent<VirtualizationScale>(); | ||
var container = Browser.Exists(By.Id("virtualize")); | ||
|
||
var people = GetPeopleNames(container); | ||
Assert.True(GetPeopleNames(container).Length > 0); | ||
ScrollTopToEnd(Browser, container); | ||
Assert.True(GetPeopleNames(container).Length > 0); | ||
} | ||
|
||
[Theory] | ||
[InlineData("simple-scroll-horizontal")] | ||
[InlineData("complex-scroll-horizontal")] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
<div id="virtualize" style="height: 200px; overflow: auto"> | ||
<Virtualize ItemsProvider="@itemsProvider"> | ||
<tr class="person"><span>@context.Name</span></tr> | ||
</Virtualize> | ||
</div> | ||
|
||
@code { | ||
internal class Person | ||
{ | ||
public int Id { get; set; } | ||
public string Name { get; set; } = string.Empty; | ||
} | ||
|
||
private ItemsProviderDelegate<Person> itemsProvider = default!; | ||
|
||
protected override void OnInitialized() | ||
{ | ||
itemsProvider = async request => | ||
{ | ||
await Task.CompletedTask; | ||
return new ItemsProviderResult<Person>( | ||
items: Enumerable.Range(request.StartIndex, request.Count).Select(i => new Person { Id = i, Name = $"Person {i}" }).ToList(), | ||
totalItemCount: 100); | ||
}; | ||
} | ||
} | ||
|
||
<style> | ||
body { | ||
scale: 2 0.5; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because in the algo we focus on y-axis value, the tests check mainly 50% scaling (decreasing). Would such an approach work, so that we can test increasing and decreasing cases?
I'm not sure how to pass these params to the component, though. The component is mounted directly, not navigated to. Maybe we can add Skip if it's too much test logic changes and you tested both cases. We can always add additional tests after the fix lands. |
||
transform-origin: top left; | ||
} | ||
</style> |
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 change is redundant,
rangeRect
is not used anywhere.