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

ScrollViewer v2 bugs/missing functionality compared to the platform ScrollViewer #829

Open
lhak opened this issue Jun 8, 2019 · 13 comments
Assignees
Labels
area-Scrolling team-Controls Issue for the Controls team

Comments

@lhak
Copy link

lhak commented Jun 8, 2019

Describe the bug

I was asked to list the missing features and bugs that the winui ScrollViewer has compared to the platform ScrollViewer

Steps to reproduce the bug

Use some XAML code like this:

<muxc:ScrollViewer x:Name="outputScrollViewer"  HorizontalScrollMode="Enabled" VerticalScrollMode="Enabled" HorizontalScrollBarVisibility="Auto" VerticalScrollBarVisibility="Auto" ZoomMode="Enabled" MaxZoomFactor="100" MinZoomFactor="1" Width="200" Height="200">
  <Viewbox x:Name="outputViewBox" MaxWidth="{x:Bind outputScrollViewer.ViewportWidth, Mode=OneWay}" MaxHeight="{x:Bind outputScrollViewer.ViewportHeight, Mode=OneWay}">
    <Rectangle x:Name="outputElement" Width="400" Height="200">
      <Rectangle.Fill>
        <LinearGradientBrush StartPoint="0.5,0" EndPoint="1,1">
          <GradientStop Color="Yellow" Offset="0.0" />
          <GradientStop Color="Red" Offset="0.25" />
          <GradientStop Color="Blue" Offset="0.75" />
          <GradientStop Color="LimeGreen" Offset="1.0" />
        </LinearGradientBrush>
      </Rectangle.Fill>
    </Rectangle>
  </Viewbox>
</muxc:ScrollViewer>

These are the regressions:

  • Missing: ViewportWidth and ViewportHeight are not bindable and so extra code behind is required

  • Missing: ChangeView() method to set the position and zoom at the same time

  • Broken: The Viewbox handles aspect ratio differences between the scrollviewer and the element by adding empty space at the borders. This empty space is still present when zooming in (and the element is now larger than the scrollviewer in all dimensions). The platform scrollviewer handles this correctly.

  • Broken: The ScrollTo() method always scrolls to the wrong position in the scenario above. Same seems to be true for the ZoomTo() method if a center point is specified.

  • Missing: CanContentRenderOutsideBounds property and the related functionality

Expected behavior

ScrollViewer should have parity with the platform ScrollViewer

Screenshots

Version Info

NuGet package version:

Windows 10 version Saw the problem?
Insider Build (xxxxx)
May 2019 Update (18362)
October 2018 Update (17763)
April 2018 Update (17134)
Fall Creators Update (16299)
Creators Update (15063)
Anniversary Update (14393)
Device form factor Saw the problem?
Desktop
Mobile
Xbox
Surface Hub
IoT

Additional context

@jevansaks
Copy link
Member

This is great feedback, thanks!

@jevansaks jevansaks changed the title ScrollViewer regressions compared to the platform ScrollViewer ScrollViewer v2 bugs/missing functionality compared to the platform ScrollViewer Jun 10, 2019
RBrid added a commit that referenced this issue Jun 18, 2019
…dth or ExtentHeight is incorrect when Content stretches partially.
RBrid added a commit that referenced this issue Jun 26, 2019
* Fixing Scroller layout bug mentioned in Issue #829: Scroller.ExtentWidth or ExtentHeight is incorrect when Content stretches partially.

* Using lambdas in Scroller::ArrangeOverride.
@RBrid
Copy link
Contributor

RBrid commented Jun 26, 2019

Thank you @lhak, points #3 and #4 should now be addressed.

@lhak
Copy link
Author

lhak commented Jul 3, 2019

@RBrid I can confirm that that the third issue is now solved (Thank you!), but I still have issues with the ScrollTo() and ZoomTo() methods. I basically want to zoom to an selection rectangle drawn with the mouse. With the platform scrollviewer, I can just calculate the required zoom level and offset by registering the pointer event handlers on the xaml element and transforming the received coordinates to the parent viewbox. Then a single call to ChangeView() is sufficient to end up at the right position and zoom level. With the winui scrollviewer I have to use a combination of ZoomTo() (with null as the center point), followed by a ScrollTo(). This works fine in most cases but sometimes the transition does not seem smooth (i.e. I see a jump after the zoom operation). The same issue is apparent when reseting the view again (i.e. going to position 0,0 and zoom level 1). It seems that the mismatch of the aspect ratios still causes issues during the animation.

@RBrid
Copy link
Contributor

RBrid commented Jul 5, 2019

Thank you @lhak. Maybe you are seeing issues with combining ScrollTo/ZoomTo calls because this PR #972 still needs to be merged with master.

Anyways, regarding the ChangeView method, I just wrote a little utility method in C# that in many cases simulates the behavior of the old ScrollViewer.ChangeView method. It uses its exact signature and triggers a single call the ScrollTo or ZoomTo. I hope this helps.

private bool ChangeView(ScrollViewer scrollViewer, double? horizontalOffset, double? verticalOffset, float? zoomFactor, bool disableAnimation)
{
    double targetHorizontalOffset = horizontalOffset == null ? scrollViewer.HorizontalOffset : (double)horizontalOffset;
    double targetVerticalOffset = verticalOffset == null ? scrollViewer.VerticalOffset : (double)verticalOffset;
    float targetZoomFactor = zoomFactor == null ? scrollViewer.ZoomFactor : (float)Math.Max(Math.Min((double)zoomFactor, scrollViewer.MaxZoomFactor), scrollViewer.MinZoomFactor);
    float deltaZoomFactor = targetZoomFactor - scrollViewer.ZoomFactor;

    if (disableAnimation)
    {
        targetHorizontalOffset = Math.Max(Math.Min(targetHorizontalOffset, scrollViewer.ExtentWidth * targetZoomFactor - scrollViewer.ViewportWidth), 0.0);
        targetVerticalOffset = Math.Max(Math.Min(targetVerticalOffset, scrollViewer.ExtentHeight * targetZoomFactor - scrollViewer.ViewportHeight), 0.0);
    }

    if (deltaZoomFactor == 0.0f)
    {
        if (targetHorizontalOffset == scrollViewer.HorizontalOffset && targetVerticalOffset == scrollViewer.VerticalOffset)
            return false;

        scrollViewer.ScrollTo(
            targetHorizontalOffset,
            targetVerticalOffset,
            new ScrollOptions(
                disableAnimation ? AnimationMode.Disabled : AnimationMode.Enabled,
                disableAnimation ? SnapPointsMode.Ignore : SnapPointsMode.Default));
    }
    else
    {
        Vector2 centerPoint = new Vector2(
            (float)(targetHorizontalOffset * scrollViewer.ZoomFactor - scrollViewer.HorizontalOffset * targetZoomFactor) / deltaZoomFactor,
            (float)(targetVerticalOffset * scrollViewer.ZoomFactor - scrollViewer.VerticalOffset * targetZoomFactor) / deltaZoomFactor);

        scrollViewer.ZoomTo(
            targetZoomFactor,
            centerPoint,
            new ZoomOptions(
                disableAnimation ? AnimationMode.Disabled : AnimationMode.Enabled,
                disableAnimation ? SnapPointsMode.Ignore : SnapPointsMode.Default));
    }

    return true;
}

@adrientetar
Copy link

Nice, the snippet could go into the docs (@micahl).

@lhak
Copy link
Author

lhak commented Jul 9, 2019

@RBrid Thank you for the code. Unfortunately, when using it in the scenario above, different aspect ratios of the scrollviewer viewport and the xaml element still lead to an incorrect final position after zooming in to a certain region.

@RBrid
Copy link
Contributor

RBrid commented Jul 9, 2019

@lhak, if you provide me with a little repro application or code & instructions to repro the issue, I can investigate. Thank you.

@adrientetar
Copy link

@RBrid, I tried using the snippet you provided in my app, and I still have the problem I reported in #937 even though it does only one method call, here: https://github.com/adrientetar/Fonte/blob/dd9ff616458b375f3c697194cb4e5aa110cdc246/Fonte.App/Controls/DesignCanvas.xaml.cs#L353-L357
If you uncomment the zoom ratio it will be in a wrong position.

@lhak
Copy link
Author

lhak commented Jul 10, 2019

@RBrid I have attached a sample application that shows this issue below. Make sure the window width is larger than the rectangle with the gradient, then left-click to zoom in. The upper left of the view should correspond to the position of the click after the zoom operation. This works well with a combination of ZoomTo()/ScrollTo() calls, but the animation is not smooth. Using your sample code seems to result in an incorrect final position. A right-click will reset the scrollviewer (please note the jump during the animation). You can also change to the platform scrollviewer in the XAML file by removing the namespace prefix and uncomment the calls to its ChangeView() methods in the code. As you can see, zooming in and out works fine in this case with smooth animations.
App1.zip

@RBrid
Copy link
Contributor

RBrid commented Jul 10, 2019

Thanks much @adrientetar and @lhak, this is really helpful.

@lhak, I ran your little app and quickly realized that the code snippet I provided last Friday had a bug in its centerPoint evaluation. In particular, it was wrong when the content was smaller than the viewport. You will see below that the new centerPoint evaluation is a bit more complex now.

Secondly, if you clicked close to the right and bottom edges of the Rectangle, you would attempt to animate beyond valid offsets. In your case, you probably prefer to systematically clamp the target offsets, so I commented out the 'if (disableAnimation)' line below for you.
In general, I would prefer the ChangeView caller to do that clamping because in some scenarios, offsets that are out of bounds at the beginning of an animation may become valid during the course of the animation.

private bool ChangeView(ScrollViewer scrollViewer, double? horizontalOffset, double? verticalOffset, float? zoomFactor, bool disableAnimation)
{
    double targetHorizontalOffset = horizontalOffset == null ? scrollViewer.HorizontalOffset : (double)horizontalOffset;
    double targetVerticalOffset = verticalOffset == null ? scrollViewer.VerticalOffset : (double)verticalOffset;
    float targetZoomFactor = zoomFactor == null ? scrollViewer.ZoomFactor : (float)Math.Max(Math.Min((double)zoomFactor, scrollViewer.MaxZoomFactor), scrollViewer.MinZoomFactor);
    float deltaZoomFactor = targetZoomFactor - scrollViewer.ZoomFactor;

    //if (disableAnimation)
    {
        targetHorizontalOffset = Math.Max(Math.Min(targetHorizontalOffset, scrollViewer.ExtentWidth * targetZoomFactor - scrollViewer.ViewportWidth), 0.0);
        targetVerticalOffset = Math.Max(Math.Min(targetVerticalOffset, scrollViewer.ExtentHeight * targetZoomFactor - scrollViewer.ViewportHeight), 0.0);
    }

    if (deltaZoomFactor == 0.0f)
    {
        if (targetHorizontalOffset == scrollViewer.HorizontalOffset && targetVerticalOffset == scrollViewer.VerticalOffset)
            return false;

        scrollViewer.ScrollTo(
            targetHorizontalOffset,
            targetVerticalOffset,
            new ScrollOptions(
                disableAnimation ? AnimationMode.Disabled : AnimationMode.Enabled,
                disableAnimation ? SnapPointsMode.Ignore : SnapPointsMode.Default));
    }
    else
    {
        FrameworkElement contentAsFE = scrollViewer.Content as FrameworkElement;
        HorizontalAlignment contentHorizontalAlignment = contentAsFE == null ? HorizontalAlignment.Stretch : contentAsFE.HorizontalAlignment;
        VerticalAlignment contentVerticalAlignment = contentAsFE == null ? VerticalAlignment.Stretch : contentAsFE.VerticalAlignment;
        double currentPositionX = scrollViewer.HorizontalOffset;
        double currentPositionY = scrollViewer.VerticalOffset;
        double currentViewportExcessX = scrollViewer.ViewportWidth - scrollViewer.ExtentWidth * scrollViewer.ZoomFactor;
        double targetViewportExcessX = scrollViewer.ViewportWidth - scrollViewer.ExtentWidth * targetZoomFactor;
        double currentViewportExcessY = scrollViewer.ViewportHeight - scrollViewer.ExtentHeight * scrollViewer.ZoomFactor;
        double targetViewportExcessY = scrollViewer.ViewportHeight - scrollViewer.ExtentHeight * targetZoomFactor;

        switch (contentHorizontalAlignment)
        {
            case HorizontalAlignment.Center:
            case HorizontalAlignment.Stretch:
                if (currentViewportExcessX > 0) currentPositionX -= currentViewportExcessX / 2.0;
                if (targetViewportExcessX > 0) targetHorizontalOffset -= targetViewportExcessX / 2.0;
                break;
            case HorizontalAlignment.Right:
                if (currentViewportExcessX > 0) currentPositionX -= currentViewportExcessX;
                if (targetViewportExcessX > 0) targetHorizontalOffset -= targetViewportExcessX;
                break;
        }

        switch (contentVerticalAlignment)
        {
            case VerticalAlignment.Center:
            case VerticalAlignment.Stretch:
                if (currentViewportExcessY > 0) currentPositionY -= currentViewportExcessY / 2.0;
                if (targetViewportExcessY > 0) targetVerticalOffset -= targetViewportExcessY / 2.0;
                break;
            case VerticalAlignment.Bottom:
                if (currentViewportExcessY > 0) currentPositionY -= currentViewportExcessY;
                if (targetViewportExcessY > 0) targetVerticalOffset -= targetViewportExcessY;
                break;
        }

        Vector2 centerPoint = new Vector2(
            (float)(targetHorizontalOffset * scrollViewer.ZoomFactor - currentPositionX * targetZoomFactor) / deltaZoomFactor,
            (float)(targetVerticalOffset * scrollViewer.ZoomFactor - currentPositionY * targetZoomFactor) / deltaZoomFactor);

        scrollViewer.ZoomTo(
            targetZoomFactor,
            centerPoint,
            new ZoomOptions(
                disableAnimation ? AnimationMode.Disabled : AnimationMode.Enabled,
                disableAnimation ? SnapPointsMode.Ignore : SnapPointsMode.Default));
    }

    return true;
}

@adrientetar, I have not looked at your scenario yet. If it is still broken after these updates, I will investigate further. Thanks.

@lhak
Copy link
Author

lhak commented Jul 23, 2019

@RBrid Thank you very much for the code. I added it to my application and it is working great with smooth animations. I actually clamp the rectangle for the zoom operation to valid values in my application so I could remove the code in the if(disableAnimation) block.

@micahl micahl assigned predavid-zz and unassigned micahl Aug 12, 2019
@jevansaks jevansaks added the team-Controls Issue for the Controls team label Nov 7, 2019
@github-actions
Copy link

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@lhak
Copy link
Author

lhak commented Jul 29, 2023

Still relevant

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Scrolling team-Controls Issue for the Controls team
Projects
None yet
Development

No branches or pull requests

6 participants