[webkit-changes] [285366] trunk/Source/WebCore

Fri, 05 Nov 2021 23:54:23 -0700

Title: [285366] trunk/Source/WebCore
Revision
285366
Author
[email protected]
Date
2021-11-05 23:53:26 -0700 (Fri, 05 Nov 2021)

Log Message

Make it possible to avoid retargeting a ScrollingMomentumCalculator
https://bugs.webkit.org/show_bug.cgi?id=232778

Reviewed by Wenson Hsieh.

Calling -[_NSScrollingMomentumCalculator setDestinationOrigin:] to the same origin it's
already targeting has side effects related to rubber-banding which a future patch needs to
avoid. However, ScrollingMomentumCalculator is structured to expect that
setRetargetedScrollOffset() is called, otherwise m_retargetedScrollOffset is left unset.

Fix so that if the ScrollingMomentumCalculator's destination is already correct when no
retargeting is necessary. We continue to store the std::optional m_retargetedScrollOffset,
but always initialize m_initialDestinationOffset.

Now ScrollAnimationMomentum::startAnimatedScrollWithInitialVelocity() can avoid
calling setRetargetedScrollOffset() if the modifier function doesn't alter the
target scroll offset.

ScrollingMomentumCalculatorMac had some undesirable behavior in the
!gEnablePlatformMomentumScrollingPrediction code path; we need to ensure that
_NSScrollingMomentumCalculator agrees about the predicted destination.

Tested by existing scroll snap tests.

* platform/ScrollAnimationMomentum.cpp:
(WebCore::ScrollAnimationMomentum::startAnimatedScrollWithInitialVelocity):
(WebCore::ScrollAnimationMomentum::updateScrollExtents):
* platform/ScrollingEffectsController.cpp:
(WebCore::ScrollingEffectsController::startMomentumScrollWithInitialVelocity):
* platform/ScrollingMomentumCalculator.cpp:
(WebCore::ScrollingMomentumCalculator::setRetargetedScrollOffset):
(WebCore::BasicScrollingMomentumCalculator::BasicScrollingMomentumCalculator):
(WebCore::BasicScrollingMomentumCalculator::linearlyInterpolatedOffsetAtProgress):
(WebCore::BasicScrollingMomentumCalculator::initializeInterpolationCoefficientsIfNecessary):
(WebCore::BasicScrollingMomentumCalculator::initializeSnapProgressCurve):
* platform/ScrollingMomentumCalculator.h:
(WebCore::ScrollingMomentumCalculator::destinationScrollOffset const):
(WebCore::ScrollingMomentumCalculator::destinationScrollOffsetDidChange):
(WebCore::ScrollingMomentumCalculator::retargetedScrollOffset const): Deleted.
(WebCore::ScrollingMomentumCalculator::retargetedScrollOffsetDidChange): Deleted.
* platform/mac/ScrollingMomentumCalculatorMac.h:
* platform/mac/ScrollingMomentumCalculatorMac.mm:
(WebCore::ScrollingMomentumCalculatorMac::ScrollingMomentumCalculatorMac):
(WebCore::ScrollingMomentumCalculatorMac::scrollOffsetAfterElapsedTime):
(WebCore::ScrollingMomentumCalculatorMac::predictedDestinationOffset):
(WebCore::ScrollingMomentumCalculatorMac::destinationScrollOffsetDidChange):
(WebCore::ScrollingMomentumCalculatorMac::setMomentumCalculatorDestinationOffset):
(WebCore::ScrollingMomentumCalculatorMac::requiresMomentumScrolling):
(WebCore::ScrollingMomentumCalculatorMac::ensurePlatformMomentumCalculator):
(WebCore::ScrollingMomentumCalculatorMac::retargetedScrollOffsetDidChange): Deleted.

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (285365 => 285366)


--- trunk/Source/WebCore/ChangeLog	2021-11-06 04:49:48 UTC (rev 285365)
+++ trunk/Source/WebCore/ChangeLog	2021-11-06 06:53:26 UTC (rev 285366)
@@ -1,3 +1,56 @@
+2021-11-05  Simon Fraser  <[email protected]>
+
+        Make it possible to avoid retargeting a ScrollingMomentumCalculator
+        https://bugs.webkit.org/show_bug.cgi?id=232778
+
+        Reviewed by Wenson Hsieh.
+        
+        Calling -[_NSScrollingMomentumCalculator setDestinationOrigin:] to the same origin it's
+        already targeting has side effects related to rubber-banding which a future patch needs to
+        avoid. However, ScrollingMomentumCalculator is structured to expect that
+        setRetargetedScrollOffset() is called, otherwise m_retargetedScrollOffset is left unset.
+
+        Fix so that if the ScrollingMomentumCalculator's destination is already correct when no
+        retargeting is necessary. We continue to store the std::optional m_retargetedScrollOffset,
+        but always initialize m_initialDestinationOffset.
+
+        Now ScrollAnimationMomentum::startAnimatedScrollWithInitialVelocity() can avoid
+        calling setRetargetedScrollOffset() if the modifier function doesn't alter the
+        target scroll offset.
+
+        ScrollingMomentumCalculatorMac had some undesirable behavior in the
+        !gEnablePlatformMomentumScrollingPrediction code path; we need to ensure that
+        _NSScrollingMomentumCalculator agrees about the predicted destination.
+
+        Tested by existing scroll snap tests.
+
+        * platform/ScrollAnimationMomentum.cpp:
+        (WebCore::ScrollAnimationMomentum::startAnimatedScrollWithInitialVelocity):
+        (WebCore::ScrollAnimationMomentum::updateScrollExtents):
+        * platform/ScrollingEffectsController.cpp:
+        (WebCore::ScrollingEffectsController::startMomentumScrollWithInitialVelocity):
+        * platform/ScrollingMomentumCalculator.cpp:
+        (WebCore::ScrollingMomentumCalculator::setRetargetedScrollOffset):
+        (WebCore::BasicScrollingMomentumCalculator::BasicScrollingMomentumCalculator):
+        (WebCore::BasicScrollingMomentumCalculator::linearlyInterpolatedOffsetAtProgress):
+        (WebCore::BasicScrollingMomentumCalculator::initializeInterpolationCoefficientsIfNecessary):
+        (WebCore::BasicScrollingMomentumCalculator::initializeSnapProgressCurve):
+        * platform/ScrollingMomentumCalculator.h:
+        (WebCore::ScrollingMomentumCalculator::destinationScrollOffset const):
+        (WebCore::ScrollingMomentumCalculator::destinationScrollOffsetDidChange):
+        (WebCore::ScrollingMomentumCalculator::retargetedScrollOffset const): Deleted.
+        (WebCore::ScrollingMomentumCalculator::retargetedScrollOffsetDidChange): Deleted.
+        * platform/mac/ScrollingMomentumCalculatorMac.h:
+        * platform/mac/ScrollingMomentumCalculatorMac.mm:
+        (WebCore::ScrollingMomentumCalculatorMac::ScrollingMomentumCalculatorMac):
+        (WebCore::ScrollingMomentumCalculatorMac::scrollOffsetAfterElapsedTime):
+        (WebCore::ScrollingMomentumCalculatorMac::predictedDestinationOffset):
+        (WebCore::ScrollingMomentumCalculatorMac::destinationScrollOffsetDidChange):
+        (WebCore::ScrollingMomentumCalculatorMac::setMomentumCalculatorDestinationOffset):
+        (WebCore::ScrollingMomentumCalculatorMac::requiresMomentumScrolling):
+        (WebCore::ScrollingMomentumCalculatorMac::ensurePlatformMomentumCalculator):
+        (WebCore::ScrollingMomentumCalculatorMac::retargetedScrollOffsetDidChange): Deleted.
+
 2021-11-05  Megan Gardner  <[email protected]>
 
         Lookup for text would not show after clicking the page and using key commands to launch.

Modified: trunk/Source/WebCore/platform/ScrollAnimationMomentum.cpp (285365 => 285366)


--- trunk/Source/WebCore/platform/ScrollAnimationMomentum.cpp	2021-11-06 04:49:48 UTC (rev 285365)
+++ trunk/Source/WebCore/platform/ScrollAnimationMomentum.cpp	2021-11-06 06:53:26 UTC (rev 285366)
@@ -27,6 +27,7 @@
 
 #include "Logging.h"
 #include "ScrollingMomentumCalculator.h"
+#include <wtf/text/TextStream.h>
 
 namespace WebCore {
 
@@ -40,16 +41,21 @@
 bool ScrollAnimationMomentum::startAnimatedScrollWithInitialVelocity(const FloatPoint& initialOffset, const FloatSize& initialVelocity, const FloatSize& initialDelta, const Function<FloatPoint(const FloatPoint&)>& destinationModifier)
 {
     auto extents = m_client.scrollExtentsForAnimation(*this);
+    m_currentOffset = initialOffset;
 
     m_momentumCalculator = ScrollingMomentumCalculator::create(extents, initialOffset, initialDelta, initialVelocity);
-    auto predictedScrollOffset = m_momentumCalculator->predictedDestinationOffset();
+    auto destinationScrollOffset = m_momentumCalculator->destinationScrollOffset();
 
     if (destinationModifier) {
-        predictedScrollOffset = destinationModifier(predictedScrollOffset);
-        m_momentumCalculator->setRetargetedScrollOffset(predictedScrollOffset);
+        auto modifiedOffset = destinationModifier(destinationScrollOffset);
+        if (modifiedOffset != destinationScrollOffset) {
+            LOG_WITH_STREAM(ScrollAnimations, stream << "ScrollAnimationMomentum " << this << " startAnimatedScrollWithInitialVelocity - predicted offset " << destinationScrollOffset << " modified to " << modifiedOffset);
+            destinationScrollOffset = modifiedOffset;
+            m_momentumCalculator->setRetargetedScrollOffset(destinationScrollOffset);
+        }
     }
 
-    if (predictedScrollOffset == initialOffset) {
+    if (destinationScrollOffset == initialOffset) {
         m_momentumCalculator = nullptr;
         return false;
     }
@@ -99,7 +105,7 @@
 void ScrollAnimationMomentum::updateScrollExtents()
 {
     auto extents = m_client.scrollExtentsForAnimation(*this);
-    auto predictedScrollOffset = m_momentumCalculator->predictedDestinationOffset();
+    auto predictedScrollOffset = m_momentumCalculator->destinationScrollOffset();
     auto constrainedOffset = predictedScrollOffset.constrainedBetween(extents.minimumScrollOffset(), extents.maximumScrollOffset());
     if (constrainedOffset != predictedScrollOffset)
         retargetActiveAnimation(constrainedOffset);

Modified: trunk/Source/WebCore/platform/ScrollingEffectsController.cpp (285365 => 285366)


--- trunk/Source/WebCore/platform/ScrollingEffectsController.cpp	2021-11-06 04:49:48 UTC (rev 285365)
+++ trunk/Source/WebCore/platform/ScrollingEffectsController.cpp	2021-11-06 06:53:26 UTC (rev 285366)
@@ -150,7 +150,7 @@
     if (!m_currentAnimation)
         m_currentAnimation = makeUnique<ScrollAnimationMomentum>(*this);
 
-    LOG_WITH_STREAM(ScrollAnimations, stream << "ScrollingEffectsController " << this << " startMomentumScrollWithInitialVelocity " << initialVelocity << " from " << initialOffset);
+    LOG_WITH_STREAM(ScrollAnimations, stream << "ScrollingEffectsController " << this << " startMomentumScrollWithInitialVelocity " << initialVelocity << " initialDelta " << initialDelta << " from " << initialOffset);
     return downcast<ScrollAnimationMomentum>(*m_currentAnimation).startAnimatedScrollWithInitialVelocity(initialOffset, initialVelocity, initialDelta, destinationModifier);
 }

Modified: trunk/Source/WebCore/platform/ScrollingMomentumCalculator.cpp (285365 => 285366)


--- trunk/Source/WebCore/platform/ScrollingMomentumCalculator.cpp	2021-11-06 04:49:48 UTC (rev 285365)
+++ trunk/Source/WebCore/platform/ScrollingMomentumCalculator.cpp	2021-11-06 06:53:26 UTC (rev 285366)
@@ -52,11 +52,11 @@
 
 void ScrollingMomentumCalculator::setRetargetedScrollOffset(const FloatPoint& target)
 {
-    if (m_retargetedScrollOffset && m_retargetedScrollOffset == target)
-        return;
-
+    auto currentDestination = destinationScrollOffset();
     m_retargetedScrollOffset = target;
-    retargetedScrollOffsetDidChange();
+    
+    if (currentDestination != destinationScrollOffset())
+        destinationScrollOffsetDidChange();
 }
 
 FloatPoint ScrollingMomentumCalculator::predictedDestinationOffset()
@@ -85,11 +85,12 @@
 BasicScrollingMomentumCalculator::BasicScrollingMomentumCalculator(const ScrollExtents& scrollExtents, const FloatPoint& initialOffset, const FloatSize& initialDelta, const FloatSize& initialVelocity)
     : ScrollingMomentumCalculator(scrollExtents, initialOffset, initialDelta, initialVelocity)
 {
+    m_initialDestinationOffset = predictedDestinationOffset();
 }
 
 FloatPoint BasicScrollingMomentumCalculator::linearlyInterpolatedOffsetAtProgress(float progress)
 {
-    return m_initialScrollOffset + progress * (retargetedScrollOffset() - m_initialScrollOffset);
+    return m_initialScrollOffset + progress * (destinationScrollOffset() - m_initialScrollOffset);
 }
 
 FloatPoint BasicScrollingMomentumCalculator::cubicallyInterpolatedOffsetAtProgress(float progress) const
@@ -149,7 +150,7 @@
         return;
     }
 
-    FloatSize startToEndVector = retargetedScrollOffset() - m_initialScrollOffset;
+    FloatSize startToEndVector = destinationScrollOffset() - m_initialScrollOffset;
     float startToEndDistance = startToEndVector.diagonalLength();
     if (!startToEndDistance) {
         // The start and end positions are the same, so we shouldn't try to interpolate a path.
@@ -170,7 +171,7 @@
     m_snapAnimationCurveCoefficients[0] = initialOffsetAsSize;
     m_snapAnimationCurveCoefficients[1] = 3 * (controlVector1 - initialOffsetAsSize);
     m_snapAnimationCurveCoefficients[2] = 3 * (initialOffsetAsSize - 2 * controlVector1 + controlVector2);
-    m_snapAnimationCurveCoefficients[3] = 3 * (controlVector1 - controlVector2) - initialOffsetAsSize + toFloatSize(retargetedScrollOffset());
+    m_snapAnimationCurveCoefficients[3] = 3 * (controlVector1 - controlVector2) - initialOffsetAsSize + toFloatSize(destinationScrollOffset());
     m_forceLinearAnimationCurve = false;
 }
 
@@ -209,10 +210,10 @@
     static const float minScrollSnapInitialProgress = 0.1;
     static const float maxScrollSnapInitialProgress = 0.5;
 
-    FloatSize alignmentVector = m_initialDelta * (retargetedScrollOffset() - m_initialScrollOffset);
+    FloatSize alignmentVector = m_initialDelta * (destinationScrollOffset() - m_initialScrollOffset);
     float initialProgress;
     if (alignmentVector.width() + alignmentVector.height() > 0)
-        initialProgress = clampTo(m_initialDelta.diagonalLength() / (retargetedScrollOffset() - m_initialScrollOffset).diagonalLength(), minScrollSnapInitialProgress, maxScrollSnapInitialProgress);
+        initialProgress = clampTo(m_initialDelta.diagonalLength() / (destinationScrollOffset() - m_initialScrollOffset).diagonalLength(), minScrollSnapInitialProgress, maxScrollSnapInitialProgress);
     else
         initialProgress = minScrollSnapInitialProgress;

Modified: trunk/Source/WebCore/platform/ScrollingMomentumCalculator.h (285365 => 285366)


--- trunk/Source/WebCore/platform/ScrollingMomentumCalculator.h	2021-11-06 04:49:48 UTC (rev 285365)
+++ trunk/Source/WebCore/platform/ScrollingMomentumCalculator.h	2021-11-06 06:53:26 UTC (rev 285366)
@@ -47,16 +47,20 @@
 
     virtual FloatPoint scrollOffsetAfterElapsedTime(Seconds) = 0;
     virtual Seconds animationDuration() = 0;
-    virtual FloatPoint predictedDestinationOffset();
+
+    FloatPoint destinationScrollOffset() const { return m_retargetedScrollOffset.value_or(m_initialDestinationOffset); }
+
     void setRetargetedScrollOffset(const FloatPoint&);
 
 protected:
-    const FloatPoint& retargetedScrollOffset() const { return m_retargetedScrollOffset.value(); }
-    virtual void retargetedScrollOffsetDidChange() { }
+    virtual FloatPoint predictedDestinationOffset();
 
+    virtual void destinationScrollOffsetDidChange() { }
+
     FloatSize m_initialDelta;
     FloatSize m_initialVelocity;
     FloatPoint m_initialScrollOffset;
+    FloatPoint m_initialDestinationOffset;
     ScrollExtents m_scrollExtents;
 
 private:
@@ -70,9 +74,11 @@
 private:
     FloatPoint scrollOffsetAfterElapsedTime(Seconds) final;
     Seconds animationDuration() final;
+
     void initializeInterpolationCoefficientsIfNecessary();
     void initializeSnapProgressCurve();
     float animationProgressAfterElapsedTime(Seconds) const;
+
     FloatPoint linearlyInterpolatedOffsetAtProgress(float progress);
     FloatPoint cubicallyInterpolatedOffsetAtProgress(float progress) const;
 
@@ -81,7 +87,6 @@
     FloatSize m_snapAnimationCurveCoefficients[4] { };
     bool m_forceLinearAnimationCurve { false };
     bool m_momentumCalculatorRequiresInitialization { true };
-    std::optional<FloatSize> m_predictedDestinationOffset;
 };
 
 } // namespace WebCore

Modified: trunk/Source/WebCore/platform/mac/ScrollingMomentumCalculatorMac.h (285365 => 285366)


--- trunk/Source/WebCore/platform/mac/ScrollingMomentumCalculatorMac.h	2021-11-06 04:49:48 UTC (rev 285365)
+++ trunk/Source/WebCore/platform/mac/ScrollingMomentumCalculatorMac.h	2021-11-06 06:53:26 UTC (rev 285366)
@@ -40,14 +40,14 @@
     FloatPoint scrollOffsetAfterElapsedTime(Seconds) final;
     Seconds animationDuration() final;
     FloatPoint predictedDestinationOffset() final;
-    void retargetedScrollOffsetDidChange() final;
+    void destinationScrollOffsetDidChange() final;
 
     _NSScrollingMomentumCalculator *ensurePlatformMomentumCalculator();
     bool requiresMomentumScrolling();
+    void setMomentumCalculatorDestinationOffset(FloatPoint);
 
     RetainPtr<_NSScrollingMomentumCalculator> m_platformMomentumCalculator;
     std::optional<bool> m_requiresMomentumScrolling;
-    FloatPoint m_initialDestinationOrigin;
 };
 
 } // namespace WebCore

Modified: trunk/Source/WebCore/platform/mac/ScrollingMomentumCalculatorMac.mm (285365 => 285366)


--- trunk/Source/WebCore/platform/mac/ScrollingMomentumCalculatorMac.mm	2021-11-06 04:49:48 UTC (rev 285365)
+++ trunk/Source/WebCore/platform/mac/ScrollingMomentumCalculatorMac.mm	2021-11-06 06:53:26 UTC (rev 285366)
@@ -47,12 +47,14 @@
 ScrollingMomentumCalculatorMac::ScrollingMomentumCalculatorMac(const ScrollExtents& scrollExtents, const FloatPoint& initialOffset, const FloatSize& initialDelta, const FloatSize& initialVelocity)
     : ScrollingMomentumCalculator(scrollExtents, initialOffset, initialDelta, initialVelocity)
 {
+    m_initialDestinationOffset = predictedDestinationOffset();
+    // We could compute m_requiresMomentumScrolling here, based on whether initialDelta is non-zero or we are in a rubber-banded state.
 }
 
 FloatPoint ScrollingMomentumCalculatorMac::scrollOffsetAfterElapsedTime(Seconds elapsedTime)
 {
     if (!requiresMomentumScrolling())
-        return retargetedScrollOffset();
+        return destinationScrollOffset();
 
     return [ensurePlatformMomentumCalculator() positionAfterDuration:elapsedTime.value()];
 }
@@ -59,17 +61,29 @@
 
 FloatPoint ScrollingMomentumCalculatorMac::predictedDestinationOffset()
 {
-    if (!gEnablePlatformMomentumScrollingPrediction)
-        return ScrollingMomentumCalculator::predictedDestinationOffset();
+    ensurePlatformMomentumCalculator();
 
-    ensurePlatformMomentumCalculator();
-    return { m_initialDestinationOrigin.x(), m_initialDestinationOrigin.y() };
+    if (!gEnablePlatformMomentumScrollingPrediction) {
+        auto nonPlatformPredictedOffset = ScrollingMomentumCalculator::predictedDestinationOffset();
+        // We need to make sure the _NSScrollingMomentumCalculator has the same idea of what offset we're shooting for.
+        if (nonPlatformPredictedOffset != m_initialDestinationOffset)
+            setMomentumCalculatorDestinationOffset(nonPlatformPredictedOffset);
+
+        return nonPlatformPredictedOffset;
+    }
+
+    return m_initialDestinationOffset;
 }
 
-void ScrollingMomentumCalculatorMac::retargetedScrollOffsetDidChange()
+void ScrollingMomentumCalculatorMac::destinationScrollOffsetDidChange()
 {
+    setMomentumCalculatorDestinationOffset(destinationScrollOffset());
+}
+
+void ScrollingMomentumCalculatorMac::setMomentumCalculatorDestinationOffset(FloatPoint scrollOffset)
+{
     _NSScrollingMomentumCalculator *calculator = ensurePlatformMomentumCalculator();
-    calculator.destinationOrigin = retargetedScrollOffset();
+    calculator.destinationOrigin = scrollOffset;
     [calculator calculateToReachDestination];
 }
 
@@ -84,7 +98,7 @@
 bool ScrollingMomentumCalculatorMac::requiresMomentumScrolling()
 {
     if (m_requiresMomentumScrolling == std::nullopt)
-        m_requiresMomentumScrolling = m_initialScrollOffset != retargetedScrollOffset() || m_initialVelocity.area();
+        m_requiresMomentumScrolling = m_initialScrollOffset != destinationScrollOffset() || m_initialVelocity.area();
     return m_requiresMomentumScrolling.value();
 }
 
@@ -97,7 +111,7 @@
     NSRect contentFrame = NSMakeRect(0, 0, m_scrollExtents.contentsSize.width(), m_scrollExtents.contentsSize.height());
     NSPoint velocity = NSMakePoint(m_initialVelocity.width(), m_initialVelocity.height());
     m_platformMomentumCalculator = adoptNS([[_NSScrollingMomentumCalculator alloc] initWithInitialOrigin:origin velocity:velocity documentFrame:contentFrame constrainedClippingOrigin:NSZeroPoint clippingSize:m_scrollExtents.viewportSize tolerance:NSMakeSize(1, 1)]);
-    m_initialDestinationOrigin = [m_platformMomentumCalculator destinationOrigin];
+    m_initialDestinationOffset = [m_platformMomentumCalculator destinationOrigin];
     return m_platformMomentumCalculator.get();
 }
 

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to