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

Fix Scroll Event Triggering and Enhance Accessibility with Private Properties for Scroll State Management #123

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

loyvsc
Copy link

@loyvsc loyvsc commented Mar 17, 2025

What does the pull request do?

This PR fixes a bug that caused the HorizontalScroll and VerticalScroll events not to be triggered using the pointer wheel, and also adds access to private properties that can be used to implement IsScrolledtoBottom and other things.

What is the current behavior?

The Vertical Scroll and Horizontal Scroll events are triggered when using ScrollBar.

What is the updated/expected behavior with this PR?

The Vertical Scroll and Horizontal Scroll events are triggered when using ScrollBar or pointer wheel.

Checklist

  • Fixed the issue of non-functioning of vertical and horizontal scrolling events
  • Added ScrollOffset and RowsTotalHeight properties

Copy link
Member

@MrJul MrJul left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!
This PR should really be split into two: one for the fix, and one for the public API.

That being said, it's unlikely that we're going to accept any new API without an example of why it's useful and what can't be done currently.

@@ -158,10 +158,21 @@ partial class DataGrid : TemplatedControl
// set as the rows were scrolled off.
private double _verticalOffset;
private byte _verticalScrollChangesIgnored;


public double VerticalScrollOffset => _verticalOffset;
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the new public API (applicable to all 3 new properties).

@@ -2370,6 +2381,9 @@ internal bool UpdateScroll(Vector delta)
{
DisplayData.PendingVerticalScrollHeight = scrollHeight;
handled = true;

var eventType = scrollHeight > 0 ? ScrollEventType.SmallIncrement : ScrollEventType.SmallDecrement;
VerticalScroll?.Invoke(this, new ScrollEventArgs(eventType, delta.Y));
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't that use the clamped scrollHeight rather than the original delta.Y?

@@ -2392,6 +2406,9 @@ internal bool UpdateScroll(Vector delta)
// We don't need to invalidate once again after UpdateHorizontalOffset.
ignoreInvalidate = true;
handled = true;

var eventType = delta.X > 0 ? ScrollEventType.SmallDecrement : ScrollEventType.SmallIncrement;
HorizontalScroll?.Invoke(this, new ScrollEventArgs(eventType, delta.X));
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't that use the clamped horizontalOffset rather than the original delta.X?

@@ -9,6 +9,7 @@

<ItemGroup>
<PackageReference Include="Avalonia.Desktop" Version="$(AvaloniaVersion)" />
<PackageReference Include="Avalonia.Diagnostics" Version="11.2.5" />
Copy link
Member

Choose a reason for hiding this comment

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

Please use $(AvaloniaVersion) instead.

@@ -7,5 +8,8 @@ public partial class MainWindow : Window
public MainWindow()
{
InitializeComponent();
Copy link
Member

Choose a reason for hiding this comment

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

AttachDevTools should be automatically included with source generators enabled (which are by default).

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.

3 participants