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

ScrollContentPresenter only looks at the first level of content for an IScrollSnapPointsInfo #17867

Open
BobLd opened this issue Jan 2, 2025 · 13 comments
Labels

Comments

@BobLd
Copy link
Contributor

BobLd commented Jan 2, 2025

Describe the bug

This is a follow up on my previous issue #17460 (old PR #17461)

When using a ScrollViewer for an ItemsPresenter inside a LayoutTransformControl, the ScrollViewer's Extent property is not correctly computed.

<ScrollViewer Name="PART_ScrollViewer"
			VerticalScrollBarVisibility="Auto"
			HorizontalScrollBarVisibility="Auto"
			Focusable="True"
			HorizontalAlignment="Stretch"
			VerticalAlignment="Stretch"
			IsScrollInertiaEnabled="True">
  
  <LayoutTransformControl Name="PART_LayoutTransformControl"
						  HorizontalAlignment="Center"
						  VerticalAlignment="Center"
						  ClipToBounds="True">

	  <ItemsPresenter Name="PART_ItemsPresenter"
					  HorizontalAlignment="Center"
					  VerticalAlignment="Stretch"
					  ItemsPanel="{TemplateBinding ItemsPanel}"/>

  </LayoutTransformControl>
</ScrollViewer>

This is visible as the vertical scrollbar is not correct (assumes the ItemsPresenter is smaller than it actually is), see below:
image

When removing the LayoutTransformControl, the Extend is correct, as is the scrollbar:

<ScrollViewer Name="PART_ScrollViewer"
			VerticalScrollBarVisibility="Auto"
			HorizontalScrollBarVisibility="Auto"
			Focusable="True"
			HorizontalAlignment="Stretch"
			VerticalAlignment="Stretch"
			IsScrollInertiaEnabled="True">

  <ItemsPresenter Name="PART_ItemsPresenter"
				  HorizontalAlignment="Center"
				  VerticalAlignment="Stretch"
				  ItemsPanel="{TemplateBinding ItemsPanel}"/>

</ScrollViewer>

image

I've created a repos to reproduce the issue: https://github.com/BobLd/ScrollBarSizeIssue and below are some metrics from the app:

No layout transform:

  • Grid
    • Bounds 0, 0, 1585.3333333333333, 907.3333333333334
  • ScrollViewer
    • Bounds 0, 0, 1585.3333333333333, 907.3333333333334
    • DesiredSize 204, 907.3333333333334
    • Extent 204, 44844
  • ItemsPresenter
    • Bounds 690.6666666666666, 0, 204, 44844
    • DesiredSize 204, 44844

With layout transform:

  • Grid
    • Bounds 0, 0, 1585.3333333333333, 907.3333333333334
  • ScrollViewer
    • Bounds 0, 0, 1585.3333333333333, 907.3333333333334
    • DesiredSize 204, 907.3333333333334
    • Extent 204, 3912 <- Not correct
  • LayoutTransformControl
    • Bounds 690.6666666666666, 0, 204, 3912
    • DesiredSize 204, 3912
  • ItemsPresenter
    • Bounds 0, 0, 204, 3912
    • DesiredSize 204, 3912

To Reproduce

See https://github.com/BobLd/ScrollBarSizeIssue
If you run the app as is, you will see the problem (incorrect behaviour).

If you remove the LayoutTransformControl in TestItemsControl.axaml, the behaviour will be correct.

Expected behavior

The ScrollViewer Extent should be correct even if it contains a LayoutTransformControl

Avalonia version

11.2.3

OS

No response

Additional context

No response

@BobLd BobLd added the bug label Jan 2, 2025
@BobLd BobLd changed the title Incorrect ScrollViewer Extend when ItemsPresenter is contained in a LayoutTransformControl Incorrect ScrollViewer Extent when ItemsPresenter is contained in a LayoutTransformControl Jan 2, 2025
@BobLd
Copy link
Contributor Author

BobLd commented Jan 2, 2025

I think I manage to narrow down the actual problem:

It looks like the ScrollContentPresenter only looks at the first level of content for an IScrollSnapPointsInfo implementation, see for example:

https://github.com/AvaloniaUI/Avalonia/blob/0dd628ffb70336bfa5246ee552c4114407058f11/src/Avalonia.Controls/Presenters/ScrollContentPresenter.cs#L604C1-L610C24

When the ItemsPresenter is inside a LayoutTransformControl, the IScrollSnapPointsInfo won't be found and the relevant logic not applied.

I believe ScrollContentPresenter should look into more children levels to look for a ScrollContentPresenter. If you think this is the correct approach, I'm happy to produce a PR for that.

EDIT: A temporary solution is to implement a LayoutTransformControl that also implements IScrollSnapPointsInfo. See the https://github.com/BobLd/ScrollBarSizeIssue/tree/layout-transform branch. I do not believe this is a long term solution though

@BobLd BobLd changed the title Incorrect ScrollViewer Extent when ItemsPresenter is contained in a LayoutTransformControl ScrollContentPresenter only looks at the first level of content for an IScrollSnapPointsInfo Jan 2, 2025
@timunie
Copy link
Contributor

timunie commented Jan 2, 2025

Cc @emmauss

@emmauss
Copy link
Contributor

emmauss commented Jan 2, 2025

LayoutTransformControl wraps your ItemsPresenter and to the ScrollViewer, it behaves like a non-panel control. It expects the LayoutTransformControl to report the correct size of its content for ScrollViewer to compute the Extent. It has no relation with Snap Points, as Extent still needs to be computed even if the content doesn't implement IScrollSnapPointInfo.

Your issue comes from VirtualizingStackPanel. _lastEstimatedElementSizeU in the panel code is never set in normal layout pass, which I think is an error. As such, item size defaults to 25, which gives incorrect panel size. Your workaround works because if the direct scrollviewer content is an IScrollSnapPointInfo that forwards snap point requests to the ItemsPresenter, the VirtualizingStackPanel will force estimate the element size, thus setting _lastEstimatedElementSizeU internally and correcting the Extent.

@BobLd
Copy link
Contributor Author

BobLd commented Jan 2, 2025

@emmauss thanks a lot for taking the time to look into that. I first came to the same conclusion regarding the _lastEstimatedElementSizeU and tried to fix it with the following PR: #17461

I closed it recently but I'd be more than happy to reopen it. if you have a moment, do you mind having a quick look (it's a one-liner)?

@BobLd BobLd closed this as completed Jan 2, 2025
@BobLd BobLd reopened this Jan 2, 2025
@timunie
Copy link
Contributor

timunie commented Jan 2, 2025

Regardless I think it should look down the visual tree for the first snappoint provider if direct content isnt it.

@emmauss
Copy link
Contributor

emmauss commented Jan 3, 2025

Regardless I think it should look down the visual tree for the first snappoint provider if direct content isnt it.

The problem with that is there could be multiple snappoint providers in the scrollviewer descendants, even on the same level.

@BobLd
Copy link
Contributor Author

BobLd commented Jan 3, 2025

@emmauss we could aim to assess if there's a unique IScrollSnapPointsInfo descendant, and if this is the case, use it?

Side question: in the current implementation, what is the benefit for the ScrollContentPresenter to find a IScrollSnapPointsInfo vs. not finding one? i.e. is there any benefit for me to implement a LayoutTransformControl that also implements IScrollSnapPointsInfo?

@emmauss
Copy link
Contributor

emmauss commented Jan 3, 2025

LayoutTransformControl is a decorator, the same as border. It's not a panel or an items control so it makes no sense to implement IScrollSnapPointsInfo, which was created for panels. If you implement it for LayoutTransformControl, you'd have to do the same for content control, border and content presenter. Because to a scrollviewer, all those controls are virtually the same.

Also, parent controls aren't supposed to search their descendants for controls that implement an interface, unless they only searching a fixed short depth. They are notified of visual tree updates only when they themselves are added to the visual tree. Their descendants can change at anytime and they won't be notified. On the other hand, descendants are free to search up the visual tree for providers they can register to. That's what panels do with IScrollAnchorProvider, and what refresh visualizer does with refresh control's scrollviewer. They know when their position in the visual tree changes and can register/unregister when they want to.

@BobLd
Copy link
Contributor Author

BobLd commented Jan 3, 2025

@emmauss makes a lot of sense. My question was more about what are the advantages of the approach I took here, i.e. passing the ItemsPresenter's IScrollSnapPointsInfo implementations to the ScrollContentPresenter via the LayoutTransformControl, instead of not doing so (putting aside the issue here).

Also, would that make sense to handle Decorator implementations (i.e. LayoutTransformControl, Border) with specific logic, to also look for a IScrollSnapPointsInfo in their direct descendant? i.e. something along those lines:

private void OnScrollGestureInertiaStartingEnded(object? sender, ScrollGestureInertiaStartingEventArgs e)
{
	var scrollable = Content;

	if (Content is ItemsControl itemsControl)
		scrollable = itemsControl.Presenter?.Panel;
	
	// Added logic:
	else if (Content is Decorator decorator && decorator.Child is IScrollSnapPointsInfo s)
		scrollable = s;
	
	if (scrollable is not IScrollSnapPointsInfo)
		return;

[...]

@emmauss
Copy link
Contributor

emmauss commented Jan 4, 2025

LayoutTransformControl

We shouldn't be adding special behaviors thats not related to the primary function of a control. Only panels and scrolling controls should interact with IScrollSnapPointsInfo. Content controls should only render their content and apply whatever they were designed to do, i.e. Border applies a border, LayoutTransformControl applies a transforms, etc. Telling others what kind of content they hosting isn't their role.

@emmauss
Copy link
Contributor

emmauss commented Jan 4, 2025

The better option would be for any IScrollSnapPointsInfo to search up its ancestors for a ScrollViewer like control and register itself as snap point source.

@BobLd
Copy link
Contributor Author

BobLd commented Jan 5, 2025

@emmauss thanks for the insight here. Not sure if the Avalonia team has a plan to address that in the short-term, but I'm happy to try doing a PR.

Do you mind pointing me towards classes that implement this kind of patern ("search up its ancestors for a control and register itself")?

@emmauss
Copy link
Contributor

emmauss commented Jan 6, 2025

@emmauss thanks for the insight here. Not sure if the Avalonia team has a plan to address that in the short-term, but I'm happy to try doing a PR.

Do you mind pointing me towards classes that implement this kind of patern ("search up its ancestors for a control and register itself")?

VirtualizingStackPanel searches for a IScrollAnchorProvider in its ancestors when it's attached to visual tree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants