Skip to content

Commit

Permalink
Fixing Scroller's RepeatedSnapPoint bug: wrong snapping before first …
Browse files Browse the repository at this point in the history 
…snap point (#1758)

* Fixing Scroller's repeated snap points bug.

* Fixing new test fragility.
  • Loading branch information
@RBrid
RBrid authored Dec 14, 2019
1 parent 38b359c commit f5355bf
Show file tree
Hide file tree
Showing 11 changed files with 673 additions and 58 deletions.
109 changes: 107 additions & 2 deletions dev/Scroller/APITests/ScrollerSnapPointTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
using MUXControlsTestApp.Utilities;
using System;
using System.Numerics;
using Windows.UI.Xaml.Controls;
using Windows.UI.Xaml.Controls.Primitives;
using System.Threading;
using Windows.UI.Xaml.Shapes;
using Common;

#if USING_TAEF
Expand All @@ -18,9 +18,12 @@
#endif

using Scroller = Microsoft.UI.Xaml.Controls.Primitives.Scroller;
using AnimationMode = Microsoft.UI.Xaml.Controls.AnimationMode;
using SnapPointsMode = Microsoft.UI.Xaml.Controls.SnapPointsMode;
using ScrollSnapPointsAlignment = Microsoft.UI.Xaml.Controls.Primitives.ScrollSnapPointsAlignment;
using ScrollSnapPoint = Microsoft.UI.Xaml.Controls.Primitives.ScrollSnapPoint;
using RepeatedScrollSnapPoint = Microsoft.UI.Xaml.Controls.Primitives.RepeatedScrollSnapPoint;
using RepeatedZoomSnapPoint = Microsoft.UI.Xaml.Controls.Primitives.RepeatedZoomSnapPoint;
using ZoomSnapPoint = Microsoft.UI.Xaml.Controls.Primitives.ZoomSnapPoint;
using ScrollerTestHooks = Microsoft.UI.Private.Controls.ScrollerTestHooks;

Expand Down Expand Up @@ -283,5 +286,107 @@ public void CanShareSnapPointsInMultipleCollections()
Verify.AreEqual<int>(1, combinationCount31);
});
}

[TestMethod]
[TestProperty("Description", "Snap to the first instance of a repeated scroll snap point and ensure it is placed after the Start value.")]
public void SnapToFirstRepeatedScrollSnapPoint()
{
Scroller scroller = null;
Rectangle rectangleScrollerContent = null;
AutoResetEvent scrollerLoadedEvent = new AutoResetEvent(false);

RunOnUIThread.Execute(() =>
{
rectangleScrollerContent = new Rectangle();
scroller = new Scroller();

SetupDefaultUI(scroller, rectangleScrollerContent, scrollerLoadedEvent);
});

WaitForEvent("Waiting for Loaded event", scrollerLoadedEvent);

// Jump to absolute offsets
ScrollTo(scroller, 60.0, 0.0, AnimationMode.Disabled, SnapPointsMode.Default);

// Add scroll repeated snap point with different offset and start.
RunOnUIThread.Execute(() =>
{
RepeatedScrollSnapPoint snapPoint = new RepeatedScrollSnapPoint(
offset: 50,
interval: 60,
start: 10,
end: 1190,
alignment: ScrollSnapPointsAlignment.Near);

scroller.HorizontalSnapPoints.Add(snapPoint);
});

// Flick with horizontal offset velocity to naturally land around offset 15.
ScrollFrom(scroller, horizontalVelocity: -165.0f, verticalVelocity: 0.0f, horizontalInertiaDecayRate: null, verticalInertiaDecayRate: null, hookViewChanged: false);

RunOnUIThread.Execute(() =>
{
// HorizontalOffset expected to have snapped to first instance of repeated snap point: 50.
Verify.AreEqual(50.0, scroller.HorizontalOffset);
Verify.AreEqual(0.0, scroller.VerticalOffset);
Verify.AreEqual(1.0f, scroller.ZoomFactor);
});
}

[TestMethod]
[TestProperty("Description", "Snap to the first instance of a repeated zoom snap point and ensure it is placed after the Start value.")]
public void SnapToFirstRepeatedZoomSnapPoint()
{
Scroller scroller = null;
Rectangle rectangleScrollerContent = null;
AutoResetEvent scrollerLoadedEvent = new AutoResetEvent(false);

RunOnUIThread.Execute(() =>
{
rectangleScrollerContent = new Rectangle();
scroller = new Scroller();

SetupDefaultUI(scroller, rectangleScrollerContent, scrollerLoadedEvent);
});

WaitForEvent("Waiting for Loaded event", scrollerLoadedEvent);

// Jump to absolute zoom factor, and center the content in the viewport.
ZoomTo(scroller,
zoomFactor: 6.0f,
centerPointX: 690.0f,
centerPointY: 340.0f,
AnimationMode.Disabled,
SnapPointsMode.Default);

// Add zoom repeated snap point with different offset and start.
RunOnUIThread.Execute(() =>
{
RepeatedZoomSnapPoint snapPoint = new RepeatedZoomSnapPoint(
offset: 5,
interval: 6,
start: 1,
end: 9);

scroller.ZoomSnapPoints.Add(snapPoint);
});

// Flick with zoom factor velocity to naturally land around factor 1.5.
ZoomFrom(scroller,
zoomFactorVelocity: -5.0f,
inertiaDecayRate: 0.6675f,
centerPointX: 150.0f,
centerPointY: 100.0f,
hookViewChanged: false);

RunOnUIThread.Execute(() =>
{
// ZoomFactor expected to have snapped to first instance of repeated snap point: 5.
// Scroll offsets do not snap and end close to 2850, 1400 for a centered content.
Verify.IsTrue(Math.Abs(scroller.HorizontalOffset - 2850.0) < 1.0);
Verify.IsTrue(Math.Abs(scroller.VerticalOffset - 1400.0) < 1.0);
Verify.AreEqual(5.0f, scroller.ZoomFactor);
});
}
}
}
34 changes: 17 additions & 17 deletions dev/Scroller/InteractionTests/ScrollerTestsWithInputHelper.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1322,7 +1322,7 @@ private void MoveTowardsTwoManditoryIrregularSnapPoint(ScrollSnapPointsAlignment
{
SetOutputDebugStringLevel("Verbose");

var elements = GoToSnapPointsPage();
var elements = GoToScrollSnapPointsPage();

int warningCount = 0;
const double viewportHeight = 500.0;
Expand Down Expand Up @@ -1456,7 +1456,7 @@ private void MoveWithinARepeatedMandatorySnapPoint(bool withOffsetEqualToStart,
{
SetOutputDebugStringLevel("Verbose");

var elements = GoToSnapPointsPage();
var elements = GoToScrollSnapPointsPage();

if (withTouch)
{
Expand Down Expand Up @@ -1620,7 +1620,7 @@ public void PanTowardsASingleOptionalIrregularSnapPoint()
}

private void SetUpOptionalIrregularSnapPoints(
SnapPointsTestPageElements elements,
ScrollSnapPointsTestPageElements elements,
string previousValue,
string previousRange,
string snap1Value,
Expand Down Expand Up @@ -2910,7 +2910,7 @@ public void ChangeOffsetWithSnapPoints()
}
}

private void SnapPointsPageChangeOffset(SnapPointsTestPageElements elements, String amount, double minValue, double maxValue)
private void SnapPointsPageChangeOffset(ScrollSnapPointsTestPageElements elements, String amount, double minValue, double maxValue)
{
Log.Comment("SnapPointsPageChangeOffset with amount: " + amount + ", minValue: " + minValue + ", maxValue: " + maxValue);

Expand All @@ -2920,7 +2920,7 @@ private void SnapPointsPageChangeOffset(SnapPointsTestPageElements elements, Str
}
#endif

private void SnapPointsPageChangeOffset(SnapPointsTestPageElements elements, String amount, double value)
private void SnapPointsPageChangeOffset(ScrollSnapPointsTestPageElements elements, String amount, double value)
{
Log.Comment("SnapPointsPageChangeOffset with amount: " + amount + ", value: " + value);

Expand Down Expand Up @@ -2962,17 +2962,17 @@ private void GoToManipulationModePage()
Wait.ForIdle();
}

private SnapPointsTestPageElements GoToSnapPointsPage()
private ScrollSnapPointsTestPageElements GoToScrollSnapPointsPage()
{
Log.Comment("Navigating to SnapPointsPage");
UIObject navigateToSnapPointsUIObject = FindElement.ByName("navigateToSnapPoints");
Verify.IsNotNull(navigateToSnapPointsUIObject, "Verifying that navigateToSnapPoints Button was found");
Log.Comment("Navigating to ScrollSnapPointsPage");
UIObject navigateToScrollSnapPointsUIObject = FindElement.ByName("navigateToScrollSnapPoints");
Verify.IsNotNull(navigateToScrollSnapPointsUIObject, "Verifying that navigateToScrollSnapPoints Button was found");

Button navigateToSimpleContentsButton = new Button(navigateToSnapPointsUIObject);
navigateToSimpleContentsButton.Invoke();
Button navigateToScrollSnapPointsButton = new Button(navigateToScrollSnapPointsUIObject);
navigateToScrollSnapPointsButton.Invoke();
Wait.ForIdle();

return GatherSnapPointsTestPageElements();
return GatherScrollSnapPointsTestPageElements();
}

private int WaitForOffsetUpdated(
Expand Down Expand Up @@ -3308,10 +3308,10 @@ private void PanToZero(UIObject scrollerUIObject, Edit text, int maxTries = 10)
Verify.AreEqual(0.0, Convert.ToDouble(text.Value));
}

private SnapPointsTestPageElements GatherSnapPointsTestPageElements()
private ScrollSnapPointsTestPageElements GatherScrollSnapPointsTestPageElements()
{
Log.Comment("GatherSnapPointsTestPageElements - entry");
var elements = new SnapPointsTestPageElements();
Log.Comment("GatherScrollSnapPointsTestPageElements - entry");
var elements = new ScrollSnapPointsTestPageElements();

elements.btnAddMISnapPointUIObject = new Button(FindElement.ByName("btnMIAddSnapPoint"));
elements.txtMISnapPointValueUIObject = new Edit(FindElement.ByName("txtMISnapPointValue"));
Expand Down Expand Up @@ -3348,11 +3348,11 @@ private SnapPointsTestPageElements GatherSnapPointsTestPageElements()

elements.scrollerOffset.SetValue("0");

Log.Comment("GatherSnapPointsTestPageElements - exit");
Log.Comment("GatherScrollSnapPointsTestPageElements - exit");
return elements;
}

private struct SnapPointsTestPageElements
private struct ScrollSnapPointsTestPageElements
{
public Button btnAddMISnapPointUIObject;
public Edit txtMISnapPointValueUIObject;
Expand Down
64 changes: 47 additions & 17 deletions dev/Scroller/ScrollerSnapPoint.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ winrt::hstring SnapPointBase::GetTargetExpression(winrt::hstring const& target)

winrt::hstring SnapPointBase::GetIsInertiaFromImpulseExpression(winrt::hstring const& target) const
{
// Returns 'it.IsInertiaFromImpulse' or 'this.Target.IsInertiaFromImpulse' starting with RS5, and 'iIFI' prior to RS5.
// Returns 'T.IsInertiaFromImpulse' or 'this.Target.IsInertiaFromImpulse' starting with RS5, and 'iIFI' prior to RS5.
return SharedHelpers::IsRS5OrHigher() ? StringUtil::FormatString(L"%1!s!.IsInertiaFromImpulse", target.data()) : s_isInertiaFromImpulse.data();
}

Expand Down Expand Up @@ -215,7 +215,7 @@ winrt::ExpressionAnimation ScrollSnapPoint::CreateRestingPointExpression(
winrt::hstring const& scale,
bool isInertiaFromImpulse)
{
winrt::hstring expression = StringUtil::FormatString(L"%1!s! * %2!s!", s_snapPointValue.data(), scale.data());
winrt::hstring expression = StringUtil::FormatString(L"%1!s!*%2!s!", s_snapPointValue.data(), scale.data());

SCROLLER_TRACE_VERBOSE(nullptr, TRACE_MSG_METH_STR, METH_NAME, this, expression.c_str());

Expand All @@ -234,7 +234,7 @@ winrt::ExpressionAnimation ScrollSnapPoint::CreateConditionalExpression(
winrt::hstring const& scale,
bool isInertiaFromImpulse)
{
wstring_view scaledValue = L"(%1!s! * %2!s!)";
wstring_view scaledValue = L"(%1!s!*%2!s!)";
winrt::hstring isInertiaFromImpulseExpression = GetIsInertiaFromImpulseExpression(L"this.Target");
winrt::hstring targetExpression = GetTargetExpression(target);
winrt::hstring scaledMinApplicableRange = StringUtil::FormatString(
Expand All @@ -254,7 +254,7 @@ winrt::ExpressionAnimation ScrollSnapPoint::CreateConditionalExpression(
s_maxImpulseApplicableValue.data(),
scale.data());
winrt::hstring expression = StringUtil::FormatString(
L"%1!s! ? (%2!s! >= %5!s! && %2!s! <= %6!s!) : (%2!s! >= %3!s! && %2!s! <= %4!s!)",
L"%1!s!?(%2!s!>=%5!s!&&%2!s!<=%6!s!):(%2!s!>=%3!s!&&%2!s!<= %4!s!)",
isInertiaFromImpulseExpression.data(),
targetExpression.data(),
scaledMinApplicableRange.data(),
Expand Down Expand Up @@ -681,18 +681,33 @@ winrt::ExpressionAnimation RepeatedScrollSnapPoint::CreateRestingPointExpression
// Previous snapped value is ignored. Pick the next snapped value if any, else the ignored value.
(impIgn + interval <= effectiveEnd ? (impIgn + interval) * scale : impIgn * scale)
:
// Pick previous snapped value.
prevSnap * scale
(
prevSnap < first i.e. fracTarget < -0.5
?
// Pick next snapped value as previous snapped value is outside applicable zone.
nextSnap * scale
:
// Pick previous snapped value as it is within applicable zone.
prevSnap * scale
)
)
:
// Regular mode. Pick previous snapped value.
prevSnap * scale
// Regular mode.
(
prevSnap < first i.e. fracTarget < -0.5
?
// Pick next snapped value as previous snapped value is outside applicable zone.
nextSnap * scale
:
// Pick previous snapped value as it is within applicable zone.
prevSnap * scale
)
)
*/

winrt::hstring isInertiaFromImpulseExpression = GetIsInertiaFromImpulseExpression(s_interactionTracker.data());
winrt::hstring expression = StringUtil::FormatString(
L"((Abs(it.%2!s!/it.Scale-((Floor((it.%2!s!/it.Scale-fst)/itv)*itv)+fst))>=Abs(it.%2!s!/it.Scale-((Ceil((it.%2!s!/it.Scale-fst)/itv)*itv)+fst)))&&(((Ceil((it.%2!s!/it.Scale-fst)/itv)*itv)+fst)<=(%1!s!?iEnd:end)))?(%1!s!?(((Ceil((it.%2!s!/it.Scale-fst)/itv)*itv)+fst)==iIgn?((iIgn==fst?fst*it.Scale:(iIgn-itv)*it.Scale)):((Ceil((it.%2!s!/it.Scale-fst)/itv)*itv)+fst)*it.Scale):((Ceil((it.%2!s!/it.Scale-fst)/itv)*itv)+fst)*it.Scale):(%1!s!?(((Floor((it.%2!s!/it.Scale-fst)/itv)*itv)+fst)==iIgn?(iIgn+itv<=(%1!s!?iEnd:end)?(iIgn+itv)*it.Scale:iIgn*it.Scale):((Floor((it.%2!s!/it.Scale-fst)/itv)*itv)+fst)*it.Scale):((Floor((it.%2!s!/it.Scale-fst)/itv)*itv)+fst)*it.Scale)",
L"((Abs(T.%2!s!/T.Scale-(Floor((T.%2!s!/T.Scale-P)/V)*V+P))>=Abs(T.%2!s!/T.Scale-(Ceil((T.%2!s!/T.Scale-P)/V)*V+P)))&&((Ceil((T.%2!s!/T.Scale-P)/V)*V+P)<=(%1!s!?iE:E)))?(%1!s!?((Ceil((T.%2!s!/T.Scale-P)/V)*V+P)==M?((M==P?P*T.Scale:(M-V)*T.Scale)):(Ceil((T.%2!s!/T.Scale-P)/V)*V+P)*T.Scale):(Ceil((T.%2!s!/T.Scale-P)/V)*V+P)*T.Scale):(%1!s!?((Floor((T.%2!s!/T.Scale-P)/V)*V+P)==M?(M+V<=(%1!s!?iE:E)?(M+V)*T.Scale:M*T.Scale):(T.%2!s!/T.Scale<P-0.5*V?(Ceil((T.%2!s!/T.Scale-P)/V)*V+P)*T.Scale:(Floor((T.%2!s!/T.Scale-P)/V)*V+P)*T.Scale)):(T.%2!s!/T.Scale<P-0.5*V?(Ceil((T.%2!s!/T.Scale-P)/V)*V+P)*T.Scale:(Floor((T.%2!s!/T.Scale-P)/V)*V+P)*T.Scale))",
isInertiaFromImpulseExpression.data(),
target.data());

Expand Down Expand Up @@ -753,7 +768,7 @@ winrt::ExpressionAnimation RepeatedScrollSnapPoint::CreateConditionalExpression(

winrt::hstring isInertiaFromImpulseExpression = GetIsInertiaFromImpulseExpression(s_interactionTracker.data());
winrt::hstring expression = StringUtil::FormatString(
L"((!%1!s! && it.%2!s! / it.Scale >= stt && it.%2!s! / it.Scale <= end) || (%1!s! && it.%2!s! / it.Scale >= iStt && it.%2!s! / it.Scale <= iEnd)) && (((Floor((it.%2!s! / it.Scale - fst) / itv) * itv) + fst + aRg >= it.%2!s! / it.Scale) || (((Ceil((it.%2!s! / it.Scale - fst) / itv) * itv) + fst - aRg <= it.%2!s! / it.Scale) && ((Ceil((it.%2!s! / it.Scale - fst) / itv) * itv) + fst <= (%1!s! ? iEnd : end))))",
L"((!%1!s!&&T.%2!s!/T.Scale>=S&&T.%2!s!/T.Scale<=E)||(%1!s!&&T.%2!s!/T.Scale>=iS&&T.%2!s!/T.Scale<=iE))&&(((Floor((T.%2!s!/T.Scale-P)/V)*V)+P+aR>=T.%2!s!/T.Scale)||(((Ceil((T.%2!s!/T.Scale-P)/V)*V)+P-aR<=T.%2!s!/T.Scale)&&((Ceil((T.%2!s!/T.Scale-P)/V)*V)+P<=(%1!s!?iE:E))))",
isInertiaFromImpulseExpression.data(),
target.data());

Expand Down Expand Up @@ -1154,7 +1169,7 @@ winrt::ExpressionAnimation ZoomSnapPoint::CreateConditionalExpression(
winrt::hstring isInertiaFromImpulseExpression = GetIsInertiaFromImpulseExpression(L"this.Target");
winrt::hstring targetExpression = GetTargetExpression(target);
winrt::hstring expression = StringUtil::FormatString(
L"%1!s! ? (%2!s! >= %5!s! && %2!s! <= %6!s!) : (%2!s! >= %3!s! && %2!s! <= %4!s!)",
L"%1!s!?(%2!s!>=%5!s!&&%2!s!<=%6!s!):(%2!s!>=%3!s!&&%2!s!<=%4!s!)",
isInertiaFromImpulseExpression.data(),
targetExpression.data(),
s_minApplicableValue.data(),
Expand Down Expand Up @@ -1569,18 +1584,33 @@ winrt::ExpressionAnimation RepeatedZoomSnapPoint::CreateRestingPointExpression(
// Previous snapped value is ignored. Pick the next snapped value if any, else the ignored value.
(impIgn + interval <= effectiveEnd ? impIgn + interval : impIgn)
:
// Pick previous snapped value.
prevSnap
(
prevSnap < first i.e. fracTarget < -0.5
?
// Pick next snapped value as previous snapped value is outside applicable zone.
nextSnap
:
// Pick previous snapped value as it is within applicable zone.
prevSnap
)
)
:
// Regular mode. Pick previous snapped value.
prevSnap
// Regular mode.
(
prevSnap < first i.e. fracTarget < -0.5
?
// Pick next snapped value as previous snapped value is outside applicable zone.
nextSnap
:
// Pick previous snapped value as it is within applicable zone.
prevSnap
)
)
*/

winrt::hstring isInertiaFromImpulseExpression = GetIsInertiaFromImpulseExpression(s_interactionTracker.data());
winrt::hstring expression = StringUtil::FormatString(
L"((Abs(it.%2!s!-((Floor((it.%2!s!-fst)/itv)*itv)+fst))>=Abs(it.%2!s!-((Ceil((it.%2!s!-fst)/itv)*itv)+fst)))&&(((Ceil((it.%2!s!-fst)/itv)*itv)+fst)<=(%1!s!?iEnd:end)))?(%1!s!?(((Ceil((it.%2!s!-fst)/itv)*itv)+fst)==iIgn?((iIgn==fst?fst:iIgn-itv)):(Ceil((it.%2!s!-fst)/itv)*itv)+fst):(Ceil((it.%2!s!-fst)/itv)*itv)+fst):(%1!s!?(((Floor((it.%2!s!-fst)/itv)*itv)+fst)==iIgn?(iIgn+itv<=(%1!s!?iEnd:end)?iIgn+itv:iIgn):(Floor((it.%2!s!-fst)/itv)*itv)+fst):(Floor((it.%2!s!-fst)/itv)*itv)+fst)",
L"((Abs(T.%2!s!-(Floor((T.%2!s!-P)/V)*V+P))>=Abs(T.%2!s!-(Ceil((T.%2!s!-P)/V)*V+P)))&&(Ceil((T.%2!s!-P)/V)*V+P<=(%1!s!?iE:E)))?(%1!s!?(Ceil((T.%2!s!-P)/V)*V+P==M?(M==P?P:M-V):Ceil((T.%2!s!-P)/V)*V+P):Ceil((T.%2!s!-P)/V)*V+P):(%1!s!?(Floor((T.%2!s!-P)/V)*V+P==M?(M+V<=(%1!s!?iE:E)?(M+V):M):(T.%2!s!<P-0.5*V?Ceil((T.%2!s!-P)/V)*V+P:Floor((T.%2!s!-P)/V)*V+P)):(T.%2!s!<P-0.5*V?Ceil((T.%2!s!-P)/V)*V+P:Floor((T.%2!s!-P)/V)*V+P))",
isInertiaFromImpulseExpression.data(),
target.data());

Expand Down Expand Up @@ -1642,7 +1672,7 @@ winrt::ExpressionAnimation RepeatedZoomSnapPoint::CreateConditionalExpression(

winrt::hstring isInertiaFromImpulseExpression = GetIsInertiaFromImpulseExpression(s_interactionTracker.data());
winrt::hstring expression = StringUtil::FormatString(
L"((!%1!s! && it.%2!s! >= stt && it.%2!s! <= end) || (%1!s! && it.%2!s! >= iStt && it.%2!s! <= iEnd)) && (((Floor((it.%2!s! - fst) / itv) * itv) + fst + aRg >= it.%2!s!) || (((Ceil((it.%2!s! - fst) / itv) * itv) + fst - aRg <= it.%2!s!) && ((Ceil((it.%2!s! - fst) / itv) * itv) + fst <= (%1!s! ? iEnd : end))))",
L"((!%1!s!&&T.%2!s!>=S&&T.%2!s!<=E)||(%1!s!&&T.%2!s!>=iS&&T.%2!s!<=iE))&&((Floor((T.%2!s!-P)/V)*V+P+aR>=T.%2!s!)||((Ceil((T.%2!s!-P)/V)*V+P-aR<=T.%2!s!)&&(Ceil((T.%2!s!-P)/V)*V+P<=(%1!s!?iE:E))))",
isInertiaFromImpulseExpression.data(),
target.data());

Expand Down
Loading

0 comments on commit f5355bf

Please sign in to comment.