Draft: Improve Refresh-Container usability

[nickodei]

Important: this is a first draft to get more feedback and discuss some behavior that is expected to happen.
The behaviour of the RefreshContainer is unintuitive for mobile users. This was picket up by this issue: #15529.

What does the pull request do?

This improved the drag-to-refresh behavior especially for mobile users and allows for a smoother experience. You are now able to start the refresh independent on the clicked position. For TopToBottom pulls it is only required that you scrolled to the top.

What is the current behavior?

#15529 goes into great detail but tldr:

1. You can only trigger a refresh in a small treshold (mostly the first item)

2. Weard behaviour where the items get scaled away from the container when refreshing at diffrent positions

This is especially noticable when you have configured the PullDirection to be anything than TopToBottom

What is the updated/expected behavior with this PR?

You are now able to trigger a refresh independen on the position you clicked, as long as you didnt scroll down (or up when the scroll behavior is BottomToTop). Left and Right scroll can be triggerd at any time right now (because it is a List that goes from top to bottom but I didnt think about Lists or things that go from left to right).

I show the desktop version here because I could easily capture the output. On mobile the experience is a lot smoother:

I was not able to fix the weard scroll-behavior but I will still try to find the bug.

How was the solution implemented (if it's not obvious)?

I created a new implementation of the GestureRecognizer where I moved more logic into the touch-moved event to allow for a smoother scroll and refresh behavior. Just fixing the canPull condition describet in the issue was not possible because it would hinder the scroll-down and making the refresh more 'reactive' is only possible in the touch-moved event.

Checklist

• Allow for dragging on any position, when the view is scrolled to the end
• Fix offset mismatch when refreshing on different heights
• You can refresh without clicking an item first
• Added unit tests (if possible)?
• Added XML documentation to any related classes?
• Consider submitting a PR to https://github.com/AvaloniaUI/avalonia-docs with user documentation

Breaking changes

• Different refresh and scroll behavior than before

Discussion

There are some points I want to discuss first befor I cleaned up the implementation:

1. Shouldn"t it be possible to refresh with the mouse as well (like shown in the GIFs) ?
2. I was not able the fix the weard behavior in number 2. Any Tipps ?
3. I create a new GestureRecognizer for the ScrollViewer because I needed to get the Offset reliable ? Is it possible to make it generic ?

Fixed issues

Fixes #15529

[avaloniaui-bot]

You can test this PR using the following package version. 11.3.999-cibuild0053260-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[emmauss]

Pushed a fix for number 2.

[emmauss]

It would be better to merge your gesture recognizer with the existing Pull recognizer. As long as the API is maintained, it should be fine.

[avaloniaui-bot]

You can test this PR using the following package version. 11.3.999-cibuild0053302-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[nickodei]

Pushed a fix for number 2.

Thank you for the commit. Unfortunately it didn't completely fix the offset-issue, especially when using BottomToTop, but it helped guide me to another working solution.

The problem arose because the offsets of the visual and visualizerVisual objects where set to the same values which is true for TopToBottom but unfortunately when using something like BottomToTop you need to add the offset caused from the visualizerVisual to your inital-offset so it "matches" the offset. This works now with all 4 directions as I would expect it

[avaloniaui-bot]

You can test this PR using the following package version. 11.3.999-cibuild0053304-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[nickodei]

It would be better to merge your gesture recognizer with the existing Pull recognizer. As log as the API is maintained, it should be fine.

I looked into it but I didn't find a way to do this. The problem is that Visual is just not enough for the offset calculations. ScrollViewer has the Offset, Width/Height and Extend Properties so I can even calculate the pull-direction BottomToTop with viewer.Offset.Y + viewer.Viewport.Height - viewer.Extent.Height. Event the CompositionVisual Property was not enougth. Did I miss something there?

[nickodei]

There are some other points I found while I was trying to improve the usability of the refresh behavior:

• Having a smooth transition between Scrolling and Refreshing is currently not possible. The problem is the Capture(e.Pointer); which is called in both ScrollGestureRecoginzer and ScrollViewerGestureRecognizer. It will inevitably be called by one of the recognizers and therefore the other one is skipped. So the other one does not get the important pointer events to process it. Therefore you have the feeling of "Clicking an item first to refresh" or you need to "release your finger and tap again so you can start the refresh". => E.g while it always recognizes the scroll when we match on offset.Y >= 0, when we capture and find out that the delta is negative (we should scroll), the ScrollGestureRecoginzer does not get this event and you don't scroll. Using offset.Y > 0, will show the behavior "Clicking an item first to refresh" because the ScrollGestureRecoginzer will capture all events and the ScrollViewerGestureRecognizer will be starved

• I wanted to add a Dropdown on the Demo where you could choose the pull-direction and while I was able to do that, it had some side-effects that are not easy to fix. Because on PullDirection-Changed we do : _refreshInfoProviderAdapter = new ScrollViewerIRefreshInfoProviderAdapter(PullDirection);. This is bad because it will loose all references to the private members an therefore you cant change the PullDirection of ScrollViewerGestureRecognizer or when you call at some time AdaptFromTree, It will create a new ScrollViewerGestureRecognizer but the old events aren't cleared and so you have double the event-handlers with different PullDirections. My attempts where not that successful because on every route, some artifacts where sill there and every solution didn't feed good

While this current implementation works, it is still far from perfect and does not feel that smooth like an native Android-Control. I don't know how much time I can pull into this issue, without any help.

[emmauss]

There are some other points I found while I was trying to improve the usability of the refresh behavior:

* Having a smooth transition between Scrolling and Refreshing is currently not possible. The problem is the `Capture(e.Pointer);` which is called in both `ScrollGestureRecoginzer` and `ScrollViewerGestureRecognizer`. It will inevitably be called by one of the recognizers and therefore the other one is skipped. So the other one does not get the important pointer events to process it. Therefore you have the feeling of "Clicking an item first to refresh" or you need to "release your finger and tap again so you can start the refresh". => E.g while it always recognizes the scroll when we match on `offset.Y >= 0`, when we capture and find out that the delta is negative (we should scroll), the `ScrollGestureRecoginzer` does not get this event and you don't scroll. Using `offset.Y > 0`, will show the behavior "Clicking an item first to refresh" because the `ScrollGestureRecoginzer` will capture all events and the  `ScrollViewerGestureRecognizer` will be starved

* I wanted to add a Dropdown on the Demo where you could choose the pull-direction and while I was able to do that, it had some side-effects that are not easy to fix. Because on PullDirection-Changed we do : `_refreshInfoProviderAdapter = new ScrollViewerIRefreshInfoProviderAdapter(PullDirection);`. This is bad because it will loose all references to the private members an therefore you cant change the PullDirection of `ScrollViewerGestureRecognizer` or when you call at some time `AdaptFromTree`, It will create a new `ScrollViewerGestureRecognizer` but the old events aren't cleared and so you have double the event-handlers with different PullDirections. My attempts where not that successful because on every route, some artifacts where sill there and every solution didn't feed good

While this current implementation works, it is still far from perfect and does not feel that smooth like an native Android-Control. I don't know how much time I can pull into this issue, without any help.

I don't quite understand the first point. If you have a list and are at the top of it, i.e. Offset = 0, pulling down will trigger the refresh. If you are not at the top, like Offset =0, pulling down will trigger scrolling with -delta Y, requiring you to release when at the top and then pulling down to trigger refresh. I think to some that behavior is preferable. But, yeah, it's not possible to transition from scrolling to pulling.

The behavior in the second point was ported directly from WinUI. It's still open for improvement, especially making it work well with binding.

Thanks for looking into this.

[nickodei]

Thank you for your quick answer. I was to vague on my first point so I have an example. I added some logs and the important part is that when you scroll "fast" (or just not slow enough) the first initial pointer event ScrollViewerPullRecognizer will give you a delta of 0 (PointerMoved(PointerEventArgs e) was called twice). Because I don"t know if we are pulling, I don"t capture the pointer and im not calling Capture(e.Pointer);. But because of that, the ScrollGestureRecognizer is calling Capture(e.Pointer); and therefore it will get all the event and I dont get the events that it was indeed negative and I should have scrolled (TopToBottom => Scrolling down). These events would be there on the third or next event but the ScrollViewerPullRecognizer already took these events and handled these:

[ViewRootImpl@9ec7c43[MainActivity]] ViewPostIme pointer 0
[DOTNET] Initial Position: 143.2888888888889, 145.42222222222222
[DOTNET] [ScrollViewerPullRecognizer]: Offset: 0, 0
[DOTNET] Delta: 0, 0 with current poition: 143.24166666666667, 145.42222222222222
[DOTNET] [ScrollViewerPullRecognizer]: Offset: 0, 0
[DOTNET] Delta: 0, 0 with current poition: 143.2888888888889, 145.42222222222222
[DOTNET] [ScrollGestureRecognizer]: Capturing point!
[ViewRootImpl@9ec7c43[MainActivity]] ViewPostIme pointer 1

If I now call Capture(e.Pointer); event if I the delta is 0, it reliably can detect if it needs to scroll or not (delta positiv or negative). I don't however have a possibility to say "hey, my delta is currently positive and I captured this pointer already to skip all other recognizer. I want to revert it and allow the other recognizer to react to this or the next pointers".

Edit:
So in the end, if I dont Initially capture the Point to find out in the next 2 to 3 events, if my delta is positive (do scroll) or negative (do refresh), the ScrollGestureRecognizer will be faster than me and I will not get the change to refresh

[emmauss]

There's an issue with cancelled pulls. The scrollviewer's visual offset doesn't reset when you do not pull fully, even though the refresh visualizer resets.
Also, the reason pulling doesn't work in your case is due to how events work. The scrollviewer receives pointer events first because its deeper in the visual tree, and pointer events bubble up. so a fast gesture will exceed the minimum scroll distance for the scroll gesture recognizer, thus triggering a scroll gesture and not a pull gesture. Currently, ScrollGestureRecognizer doesn't have any data on the scroll viewer's offset, and can't cancel scrolling if it starts with 0,0 offset.

[emmauss]

We could allow other gestures to run if an active gesture is in a state where it can't process any more input, by suspending it and notifying all gestures that the pointer can be captured.

[nickodei]

Okay I honestly didn't think about it. Im passing the Offset, Height and Extent to the ScrollGestureRecognizer as a "dirty" implementation so I cant test how it feels when im not capturing Points that are not for scrolling. And at this stage the ScrollView with its refresh feel actually nice. The transitions are smooth and all Pull-Directions work as expected. There is probably a opportunity to use the IScrollable interface since it has all the properties needed to make the calculations and move the logic to the general PullGestureRecognizer.

I also fixed the glitch where when you cancle the pull, it does not reset the Offset of the ScrollViewer.

[emmauss]

Okay I honestly didn't think about it. Im passing the Offset, Height and Extent to the ScrollGestureRecognizer as a "dirty" implementation so I cant test how it feels when im not capturing Points that are not for scrolling. And at this stage the ScrollView with its refresh feel actually nice. The transitions are smooth and all Pull-Directions work as expected. There is probably a opportunity to use the IScrollable interface since it has all the properties needed to make the calculations and move the logic to the general PullGestureRecognizer.

I also fixed the glitch where when you cancle the pull, it does not reset the Offset of the ScrollViewer.

It will be best to keep the new recognizer as a separate gesture recognizer. It just needs a better name.