SmoothScrollIntoView will scroll to the new offset even after is already scrolled to the correct position

[Vijay-Nirmal]

Describe the bug

SmoothScrollIntoView will scroll to the new offset even after is already scrolled to the correct position. This will happen only when item placement is Default and Scroll If Visible is true.

Steps to Reproduce

• Can this be reproduced in the Sample App?

1. Give a horizontal offset with item placement as Default and Scroll If Visible as true
2. Click Start Smooth Scrolling multiple time

Expected behavior

Need to discuss

Screenshots

Environment

NuGet Package(s): Not Released / Master Branch

Package Version(s): Not Released / Master Branch

Windows 10 Build Number:
- [x] Fall Creators Update (16299)
- [x] April 2018 Update (17134)
- [x] October 2018 Update (17763)
- [x] May 2019 Update (18362)
- [x] May 2020 Update (19041)
- [x] Insider Build (build number: )

App min and target version:
- [x] Fall Creators Update (16299)
- [x] April 2018 Update (17134)
- [x] October 2018 Update (17763)
- [x] May 2019 Update (18362)
- [x] May 2020 Update (19041)
- [x] Insider Build (xxxxx)

Device form factor:
- [ ] Desktop
- [ ] Xbox
- [ ] Surface Hub
- [ ] IoT

Visual Studio
- [ ] 2017 (version: )
- [ ] 2019 (version: )
- [ ] 2019 Preview (version: )

Additional context

Original discussion: #4129 (comment) and #4129 (comment)

Hello Vijay-Nirmal, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

[Vijay-Nirmal]

Looping @RosarioPulella for the discussion :)

[Rosuavio]

Okay so I thought about this from a few perspectives and what I am finding is that Default does not communicate any clear behavior to the consumer or to us as maintainers/implementers. What the option seems to do though is a sort of "Scroll until the item is in view, and stop once it is no matter what the direction it came from is". I think we could have a default ScollIteamPlacement but there should not be a Default ScollIteamPlacement value. (In general, I think its clearer if a value is a default in some context and not if default is a value).

If we want to "Scroll until the item is visible in any position", offset should not be hard, it could form a padded area in the container we are scrolling in as a target for the items placement. So instead of stopping the scroll where the item first fits it farthest corner completely into the container, we scroll the item until its farthest corner fits the inner padded area.

The problem with Scroll until the item is visible in any position is it hard to make a decision on where to scroll to if we want to "Scroll if visible". Say we scrolled the item into the container, then what if the area for our item in the container is too small for the item ( like a the item is too wide or tall for the ListView). Then scrolling the item when its out of view is trivial, scroll until the farthest corner is completely visible in the container ( This will push another edge or the whole opposite corner out of view, but that's predictable and maybe the best we can do in that scenario). The issue if if the item is already visible, and we want to scroll into view anyways, then if we scroll the farthest corner out of view into view, we would just push out the other edge or corner. And if we scrolled again it would just move back.

For these reasons, I think the "Default" behavior could be made more clear be changing into a "scroll into view anywhere", but It cannot be mixed with a "Scroll if visible" option (It should never scroll if visible).

Because the other ScrollItemPlacements have absolute positions in the view they can "scroll if visible", because there is always an absolute position to scroll to.

In that sense, maybe instead of having one type of "Scroll" method, there should be two.

// This scrolls the item's farthest corner completely into view (plus offsets).
public static async Task SmoothScrollIntoViewWithIndexAsync(this ListViewBase listViewBase, int index, bool disableAnimation = false, int additionalHorizontalOffset = 0, int additionalVerticalOffset = 0)

// This scrolls the item in a particular placement, with an offset.
// And `ScrollItemPlacement` only consists of values that describe consistent locations relative to the view ( like Top|Left|Center|Bottom|Right)
public static async Task SmoothScrollIntoPlacementWithIndexAsync(this ListViewBase listViewBase, int index, ScrollItemPlacement itemPlacement = ScrollItemPlacement.Center, bool disableAnimation = false, bool scrollIfVisible = true, int additionalHorizontalOffset = 0, int additionalVerticalOffset = 0)

Other notes.

• Each method would have its own accompanying "WithItem" method
• You could have have a default itemPlacement to make the users choice more explicit

[Vijay-Nirmal]

• SmoothScrollIntoView method is a replacement for ScrollIntoView built-in method which uses ScrollIntoViewAlignment for specifying the item placement. Default value in our ScollItemPlacement enum is inspired by Default value in ScrollIntoViewAlignment enum (Built-in type). Looking at this perspective, it makes more sense to have Default value.

• offset should not be hard, it could form a padded area in the container we are scrolling in as a target for the items placement

  If this is the case then this will affect other item placements. For example, it the item placement is Top then we need to add additionalOffset to introduce padding, but if the item placement is Bottom then we need to subtract additionalOffset to introduce padding. If this is a expected behavior then we should change the name from additionalXOffset to verticalPadding or just padding with 0,0,0,0 as the padding format (just like normal padding). It should support negative value as well.

• Say we scrolled the item into the container, then what if the area for our item in the container is too small for the item ( like a the item is too wide or tall for the ListView)

  I need to check how ScrollIntoView built-in method handle this case. But personal I think, this is a edge case can be ignored if we don't have a perfect solution.

In my opinion, when item placement is Default and Scroll If Visible is true, we should scroll the item to nearest edge with additional offset even if the item is already visible. If the user doesn't want to scroll when the item is visible then they must set Scroll If Visible as false

[Rosuavio]

* `SmoothScrollIntoView` method is a replacement for [`ScrollIntoView`](https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.controls.listviewbase.scrollintoview?view=winrt-20348) built-in method which uses [`ScrollIntoViewAlignment`](https://docs.microsoft.com/en-us/uwp/api/Windows.UI.Xaml.Controls.ScrollIntoViewAlignment?view=winrt-20348) for specifying the item placement. `Default` value in our `ScollItemPlacement` enum is inspired by `Default` value in [`ScrollIntoViewAlignment`](https://docs.microsoft.com/en-us/uwp/api/Windows.UI.Xaml.Controls.ScrollIntoViewAlignment?view=winrt-20348) enum (Built-in type). Looking at this perspective, it makes more sense to have `Default` value.

So to me it seems ScrollIntoViewAlignment and our ScollItemPlacement actually differ significantly (Except for Default).
The rest of the placements are relative to specific edges of the view, but our Default and the ScrollIntoViewAlignment are about what part of the item aligns with the edge the item encounters when it scrolls into view. For that reason it seems like a good idea to separate these behaviors.

* > offset should not be hard, it could form a padded area in the container we are scrolling in as a target for the items placement
  
  
  If this is the case then this will affect other item placements. For example, it the item placement is Top then we need to **_add_** additionalOffset to introduce padding, but if the item placement is _Bottom_ then we need to **_subtract_** additionalOffset to introduce padding. If this is a expected behavior then we should change the name from `additionalXOffset` to `verticalPadding` or just `padding` with `0,0,0,0` as the padding format (just like normal padding). It should support negative value as well.

The Padding idea was just a way of looking at how to do the Default behavior, I think the absolute positions are spot on already.

* > Say we scrolled the item into the container, then what if the area for our item in the container is too small for the item ( like a the item is too wide or tall for the ListView)
  
  
  I need to check how [`ScrollIntoView`](https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.controls.listviewbase.scrollintoview?view=winrt-20348) built-in method handle this case. But personal I think, this is a edge case can be ignored if we don't have a perfect solution.

I agree it does not have to be perfect.

In my opinion, when item placement is Default and Scroll If Visible is true, we should scroll the item to nearest edge with additional offset even if the item is already visible. If the user doesn't want to scroll when the item is visible then they must set Scroll If Visible as false

I think that is sensible behavior compared to the current. While I think splitting out this Default option would make the API clearer, I am perfectly happy with us changing it in the way you described.

[Vijay-Nirmal]

I am back.

I think that is sensible behavior compared to the current. While I think splitting out this Default option would make the API clearer, I am perfectly happy with us changing it in the way you described.

Additional offset is important for default item placement, it is inspired by many smooth scroll implementations in build-in apps like Store, PowerPoint etc. Additional offset for default should work dynamically because if the item is aligned at the top/left then we should add the offset but if the item is aligned at the bottom/right then we should subtract the offset, in this case, it will act like the "additional negative offset". So, to make it clear I will change the additional offset to padding.

Doing this will cause breaking change (for persons using the preview package). I hope this if fine since there is no actual package released. @michael-hawker @RosarioPulella

Breaking Changes are

• Parameter name change: additionalHorizontalOffset and additionalVerticalOffset to horizontalPadding and verticalPadding responsibility
• Behavorial change when item placement is Default, Bottom or Right

I will try to make these changes before the main release. @michael-hawker When is the code freeze for the main release?

[Vijay-Nirmal]

Also, Should I combine Top and Left item placement to Starting and combine Right and Bottom to item placement Leading?

[Vijay-Nirmal]

In my opinion, when item placement is Default and Scroll If Visible is true, we should scroll the item to nearest edge with additional offset even if the item is already visible. If the user doesn't want to scroll when the item is visible then they must set Scroll If Visible as false

I think that is sensible behavior compared to the current. While I think splitting out this Default option would make the API clearer, I am perfectly happy with us changing it in the way you described.

After further thought, why anyone would want to scroll to the nearest edge when the item is visible?

WindowsCommunityToolkit/Microsoft.Toolkit.Uwp.UI/Extensions/ListViewBase/ListViewExtensions.SmoothScrollIntoView.cs

Line 115 in ee93385

finalXPosition = previousXOffset + additionalHorizontalOffset;

WindowsCommunityToolkit/Microsoft.Toolkit.Uwp.UI/Extensions/ListViewBase/ListViewExtensions.SmoothScrollIntoView.cs

Line 128 in ee93385

finalYPosition = previousYOffset + additionalVerticalOffset;

I think we can just remove additionalHorizontalOffset and additionalVerticalOffset from these places and close the issue. This is solve our orginal issue (SmoothScrollIntoView will scroll to the new offset even after is already scrolled to the correct position).