Diff
Modified: trunk/Source/WebCore/ChangeLog (269311 => 269312)
--- trunk/Source/WebCore/ChangeLog 2020-11-03 17:53:32 UTC (rev 269311)
+++ trunk/Source/WebCore/ChangeLog 2020-11-03 17:59:11 UTC (rev 269312)
@@ -1,3 +1,58 @@
+2020-11-02 Simon Fraser <[email protected]>
+
+ Scroll position can get reset after programmatic scroll
+ https://bugs.webkit.org/show_bug.cgi?id=218477
+
+ Reviewed by Antti Koivisto.
+
+ Part of rdar://problem/69599531: Scrolling on netflix.com sometimes jumps to the top.
+
+ Scrolling thread scroll notifications are handled on the main thread asynchronously, and
+ there are two sources of delay: first, the RunLoop::main().dispatch() in ThreadedScrollingTree::scrollingTreeNodeDidScroll(),
+ and second the zero-delay m_updateNodeScrollPositionTimer in AsyncScrollingCoordinator.
+
+ This is a problem when doing programmatic scrolls, since the main thread can
+ programmatically scroll, then the above asynchrony means that
+ AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll() gets called for an earlier
+ user scroll (or, as is often the case on netflix.com, a scroll triggered by the
+ rubberbanding timer on the scrolling thread).
+
+ This patches addresses these out-of-order scrolls by storing scrolling thread scroll updates
+ in a threadsafe data structure on the scrolling tree, and always applying those on the main
+ thread before handling other scroll changes.
+
+ This patch removes the zero-delay timer, which did serve a purpose of coalescing scroll
+ notifications from the scrolling thread and UI process (for iOS). With the patch, we still
+ get coalescing per RunLoop::main().dispatch(). We lose coalescing on iOS, but those
+ notifications should be just once per frame. If we find that it was necessary to coalesce,
+ we can put a timer back.
+
+ Not testable because it's very timing dependent.
+
+ * page/scrolling/AsyncScrollingCoordinator.cpp:
+ (WebCore::AsyncScrollingCoordinator::AsyncScrollingCoordinator):
+ (WebCore::AsyncScrollingCoordinator::requestScrollPositionUpdate):
+ (WebCore::AsyncScrollingCoordinator::synchronizeStateFromScrollingTree):
+ (WebCore::AsyncScrollingCoordinator::noteScrollingThreadSyncCompleteForNode):
+ (WebCore::AsyncScrollingCoordinator::applyPendingScrollUpdates):
+ (WebCore::AsyncScrollingCoordinator::applyScrollUpdate):
+ (WebCore::AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll):
+ (WebCore::AsyncScrollingCoordinator::scheduleUpdateScrollPositionAfterAsyncScroll): Deleted.
+ (WebCore::AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScrollTimerFired): Deleted.
+ * page/scrolling/AsyncScrollingCoordinator.h:
+ (WebCore::AsyncScrollingCoordinator::ScheduledScrollUpdate::ScheduledScrollUpdate): Deleted.
+ (WebCore::AsyncScrollingCoordinator::ScheduledScrollUpdate::matchesUpdateType const): Deleted.
+ * page/scrolling/ScrollingTree.cpp:
+ (WebCore::ScrollingTree::applyLayerPositionsInternal):
+ (WebCore::ScrollingTree::addPendingScrollUpdate):
+ (WebCore::ScrollingTree::takePendingScrollUpdates):
+ * page/scrolling/ScrollingTree.h:
+ (WebCore::ScrollingTree::ScrollUpdate::ScrollUpdate):
+ (WebCore::ScrollingTree::ScrollUpdate::canMerge const):
+ (WebCore::ScrollingTree::ScrollUpdate::merge):
+ * page/scrolling/ThreadedScrollingTree.cpp:
+ (WebCore::ThreadedScrollingTree::scrollingTreeNodeDidScroll):
+
2020-11-03 Sam Weinig <[email protected]>
Convert Settings.yaml to match the rough schema of the WebPreferences*.yaml files as a first step toward merging them
Modified: trunk/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp (269311 => 269312)
--- trunk/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp 2020-11-03 17:53:32 UTC (rev 269311)
+++ trunk/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp 2020-11-03 17:59:11 UTC (rev 269312)
@@ -58,7 +58,6 @@
AsyncScrollingCoordinator::AsyncScrollingCoordinator(Page* page)
: ScrollingCoordinator(page)
- , m_updateNodeScrollPositionTimer(*this, &AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScrollTimerFired)
, m_scrollingStateTree(makeUnique<ScrollingStateTree>(this))
{
}
@@ -255,7 +254,6 @@
{
ASSERT(isMainThread());
ASSERT(m_page);
-
auto scrollingNodeID = scrollableArea.scrollingNodeID();
if (!scrollingNodeID)
return false;
@@ -270,7 +268,7 @@
bool inBackForwardCache = frameView->frame().document()->backForwardCacheState() != Document::NotInBackForwardCache;
bool inProgrammaticScroll = scrollableArea.currentScrollType() == ScrollType::Programmatic;
if (inProgrammaticScroll || inBackForwardCache)
- updateScrollPositionAfterAsyncScroll(scrollingNodeID, scrollPosition, { }, ScrollType::Programmatic, ScrollingLayerPositionAction::Set, InformWheelEventMonitor::No);
+ applyScrollUpdate(scrollingNodeID, scrollPosition, { }, ScrollType::Programmatic, ScrollingLayerPositionAction::Set, InformWheelEventMonitor::No);
ASSERT(inProgrammaticScroll == (scrollType == ScrollType::Programmatic));
@@ -301,6 +299,7 @@
void AsyncScrollingCoordinator::synchronizeStateFromScrollingTree()
{
ASSERT(isMainThread());
+ applyPendingScrollUpdates();
m_scrollingTree->traverseScrollingTree([&](ScrollingNodeID nodeID, ScrollingNodeType, Optional<FloatPoint> scrollPosition, Optional<FloatPoint> layoutViewportOrigin, bool scrolledSinceLastCommit) {
if (scrollPosition && scrolledSinceLastCommit) {
@@ -314,7 +313,7 @@
{
#if PLATFORM(MAC)
if (m_page && m_page->isMonitoringWheelEvents()) {
- LOG_WITH_STREAM(WheelEventTestMonitor, stream << " (!) AsyncScrollingCoordinator::scheduleUpdateScrollPositionAfterAsyncScroll: Removing deferral on " << nodeID << " for reason " << WheelEventTestMonitor::ScrollingThreadSyncNeeded);
+ LOG_WITH_STREAM(WheelEventTestMonitor, stream << " (!) AsyncScrollingCoordinator::noteScrollingThreadSyncCompleteForNode: Removing deferral on " << nodeID << " for reason " << WheelEventTestMonitor::ScrollingThreadSyncNeeded);
m_page->wheelEventTestMonitor()->removeDeferralForReason(reinterpret_cast<WheelEventTestMonitor::ScrollableAreaIdentifier>(nodeID), WheelEventTestMonitor::ScrollingThreadSyncNeeded);
}
#else
@@ -322,33 +321,18 @@
#endif
}
-void AsyncScrollingCoordinator::scheduleUpdateScrollPositionAfterAsyncScroll(ScrollingNodeID nodeID, const FloatPoint& scrollPosition, const Optional<FloatPoint>& layoutViewportOrigin, ScrollingLayerPositionAction scrollingLayerPositionAction)
+void AsyncScrollingCoordinator::applyPendingScrollUpdates()
{
- ScheduledScrollUpdate scrollUpdate(nodeID, scrollPosition, layoutViewportOrigin, scrollingLayerPositionAction);
-
- if (m_updateNodeScrollPositionTimer.isActive()) {
- if (m_scheduledScrollUpdate.matchesUpdateType(scrollUpdate)) {
- m_scheduledScrollUpdate.scrollPosition = scrollPosition;
- m_scheduledScrollUpdate.layoutViewportOrigin = layoutViewportOrigin;
- return;
- }
-
- // If the parameters don't match what was previously scheduled, dispatch immediately.
- m_updateNodeScrollPositionTimer.stop();
- updateScrollPositionAfterAsyncScroll(m_scheduledScrollUpdate.nodeID, m_scheduledScrollUpdate.scrollPosition, m_scheduledScrollUpdate.layoutViewportOrigin, ScrollType::User, m_scheduledScrollUpdate.updateLayerPositionAction);
- updateScrollPositionAfterAsyncScroll(nodeID, scrollPosition, layoutViewportOrigin, ScrollType::User, scrollingLayerPositionAction);
+ if (!m_scrollingTree)
return;
+
+ auto scrollUpdates = m_scrollingTree->takePendingScrollUpdates();
+ for (auto& update : scrollUpdates) {
+ LOG_WITH_STREAM(Scrolling, stream << "AsyncScrollingCoordinator::applyPendingScrollUpdates - node " << update.nodeID << " scroll position " << update.scrollPosition);
+ updateScrollPositionAfterAsyncScroll(update.nodeID, update.scrollPosition, update.layoutViewportOrigin, ScrollType::User, update.updateLayerPositionAction, InformWheelEventMonitor::Yes);
}
-
- m_scheduledScrollUpdate = scrollUpdate;
- m_updateNodeScrollPositionTimer.startOneShot(0_s);
}
-void AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScrollTimerFired()
-{
- updateScrollPositionAfterAsyncScroll(m_scheduledScrollUpdate.nodeID, m_scheduledScrollUpdate.scrollPosition, m_scheduledScrollUpdate.layoutViewportOrigin, ScrollType::User, m_scheduledScrollUpdate.updateLayerPositionAction);
-}
-
FrameView* AsyncScrollingCoordinator::frameViewForScrollingNode(ScrollingNodeID scrollingNodeID) const
{
if (!m_scrollingStateTree->rootStateNode())
@@ -381,6 +365,12 @@
return nullptr;
}
+void AsyncScrollingCoordinator::applyScrollUpdate(ScrollingNodeID scrollingNodeID, const FloatPoint& scrollPosition, Optional<FloatPoint> layoutViewportOrigin, ScrollType scrollType, ScrollingLayerPositionAction scrollingLayerPositionAction, InformWheelEventMonitor informWheelEventMonitor)
+{
+ applyPendingScrollUpdates();
+ updateScrollPositionAfterAsyncScroll(scrollingNodeID, scrollPosition, layoutViewportOrigin, scrollType, scrollingLayerPositionAction, informWheelEventMonitor);
+}
+
void AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll(ScrollingNodeID scrollingNodeID, const FloatPoint& scrollPosition, Optional<FloatPoint> layoutViewportOrigin, ScrollType scrollType, ScrollingLayerPositionAction scrollingLayerPositionAction, InformWheelEventMonitor informWheelEventMonitor)
{
ASSERT(isMainThread());
@@ -395,7 +385,7 @@
if (!frameViewPtr)
return;
- LOG_WITH_STREAM(Scrolling, stream << "AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll node " << scrollingNodeID << " scrollPosition " << scrollPosition << " action " << scrollingLayerPositionAction);
+ LOG_WITH_STREAM(Scrolling, stream << "AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll node " << scrollingNodeID << " " << scrollType << " scrollPosition " << scrollPosition << " action " << scrollingLayerPositionAction);
auto& frameView = *frameViewPtr;
Modified: trunk/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h (269311 => 269312)
--- trunk/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h 2020-11-03 17:53:32 UTC (rev 269311)
+++ trunk/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h 2020-11-03 17:59:11 UTC (rev 269312)
@@ -51,10 +51,10 @@
void scrollingStateTreePropertiesChanged();
- WEBCORE_EXPORT void scheduleUpdateScrollPositionAfterAsyncScroll(ScrollingNodeID, const FloatPoint&, const Optional<FloatPoint>& layoutViewportOrigin, ScrollingLayerPositionAction);
+ void applyPendingScrollUpdates();
enum class InformWheelEventMonitor { Yes, No };
- void updateScrollPositionAfterAsyncScroll(ScrollingNodeID, const FloatPoint&, Optional<FloatPoint> layoutViewportOrigin, ScrollType, ScrollingLayerPositionAction, InformWheelEventMonitor = InformWheelEventMonitor::Yes);
+ WEBCORE_EXPORT void applyScrollUpdate(ScrollingNodeID, const FloatPoint&, Optional<FloatPoint> layoutViewportOrigin, ScrollType, ScrollingLayerPositionAction, InformWheelEventMonitor = InformWheelEventMonitor::Yes);
#if PLATFORM(COCOA)
WEBCORE_EXPORT void handleWheelEventPhase(ScrollingNodeID, PlatformWheelEventPhase) final;
@@ -155,38 +155,15 @@
void ensureRootStateNodeForFrameView(FrameView&);
- void updateScrollPositionAfterAsyncScrollTimerFired();
void setEventTrackingRegionsDirty();
void updateEventTrackingRegions();
void noteScrollingThreadSyncCompleteForNode(ScrollingNodeID);
+
+ void updateScrollPositionAfterAsyncScroll(ScrollingNodeID, const FloatPoint&, Optional<FloatPoint> layoutViewportOrigin, ScrollType, ScrollingLayerPositionAction, InformWheelEventMonitor);
FrameView* frameViewForScrollingNode(ScrollingNodeID) const;
- Timer m_updateNodeScrollPositionTimer;
-
- struct ScheduledScrollUpdate {
- ScheduledScrollUpdate() = default;
- ScheduledScrollUpdate(ScrollingNodeID scrollingNodeID, FloatPoint point, Optional<FloatPoint> viewportOrigin, ScrollingLayerPositionAction udpateAction)
- : nodeID(scrollingNodeID)
- , scrollPosition(point)
- , layoutViewportOrigin(viewportOrigin)
- , updateLayerPositionAction(udpateAction)
- { }
-
- ScrollingNodeID nodeID { 0 };
- FloatPoint scrollPosition;
- Optional<FloatPoint> layoutViewportOrigin;
- ScrollingLayerPositionAction updateLayerPositionAction { ScrollingLayerPositionAction::Sync };
-
- bool matchesUpdateType(const ScheduledScrollUpdate& other) const
- {
- return nodeID == other.nodeID && updateLayerPositionAction == other.updateLayerPositionAction;
- }
- };
-
- ScheduledScrollUpdate m_scheduledScrollUpdate;
-
std::unique_ptr<ScrollingStateTree> m_scrollingStateTree;
RefPtr<ScrollingTree> m_scrollingTree;
Modified: trunk/Source/WebCore/page/scrolling/ScrollingTree.cpp (269311 => 269312)
--- trunk/Source/WebCore/page/scrolling/ScrollingTree.cpp 2020-11-03 17:53:32 UTC (rev 269311)
+++ trunk/Source/WebCore/page/scrolling/ScrollingTree.cpp 2020-11-03 17:59:11 UTC (rev 269312)
@@ -402,7 +402,7 @@
if (!m_rootNode)
return;
- LOG(Scrolling, "\nScrollingTree %p applyLayerPositions (main thread %d)", this, isMainThread());
+// LOG(Scrolling, "\nScrollingTree %p applyLayerPositions (main thread %d)", this, isMainThread());
applyLayerPositionsRecursive(*m_rootNode);
}
@@ -577,6 +577,25 @@
return false;
}
+void ScrollingTree::addPendingScrollUpdate(ScrollUpdate&& update)
+{
+ LockHolder locker(m_pendingScrollUpdatesLock);
+ for (auto& existingUpdate : m_pendingScrollUpdates) {
+ if (existingUpdate.canMerge(update)) {
+ existingUpdate.merge(WTFMove(update));
+ return;
+ }
+ }
+
+ m_pendingScrollUpdates.append(WTFMove(update));
+}
+
+Vector<ScrollingTree::ScrollUpdate> ScrollingTree::takePendingScrollUpdates()
+{
+ LockHolder locker(m_pendingScrollUpdatesLock);
+ return std::exchange(m_pendingScrollUpdates, { });
+}
+
// Can be called from the main thread.
void ScrollingTree::setScrollPinningBehavior(ScrollPinningBehavior pinning)
{
Modified: trunk/Source/WebCore/page/scrolling/ScrollingTree.h (269311 => 269312)
--- trunk/Source/WebCore/page/scrolling/ScrollingTree.h 2020-11-03 17:53:32 UTC (rev 269311)
+++ trunk/Source/WebCore/page/scrolling/ScrollingTree.h 2020-11-03 17:59:11 UTC (rev 269312)
@@ -211,6 +211,27 @@
bool hasProcessedWheelEventsRecently();
WEBCORE_EXPORT void willProcessWheelEvent();
+ struct ScrollUpdate {
+ ScrollingNodeID nodeID { 0 };
+ FloatPoint scrollPosition;
+ Optional<FloatPoint> layoutViewportOrigin;
+ ScrollingLayerPositionAction updateLayerPositionAction { ScrollingLayerPositionAction::Sync };
+
+ bool canMerge(const ScrollUpdate& other) const
+ {
+ return nodeID == other.nodeID && updateLayerPositionAction == other.updateLayerPositionAction;
+ }
+
+ void merge(ScrollUpdate&& other)
+ {
+ scrollPosition = other.scrollPosition;
+ layoutViewportOrigin = other.layoutViewportOrigin;
+ }
+ };
+
+ void addPendingScrollUpdate(ScrollUpdate&&);
+ Vector<ScrollUpdate> takePendingScrollUpdates();
+
protected:
WheelEventHandlingResult handleWheelEventWithNode(const PlatformWheelEvent&, OptionSet<WheelEventProcessingSteps>, ScrollingTreeNode*);
@@ -279,6 +300,9 @@
Lock m_swipeStateMutex;
SwipeState m_swipeState;
+ Lock m_pendingScrollUpdatesLock;
+ Vector<ScrollUpdate> m_pendingScrollUpdates;
+
Lock m_lastWheelEventTimeMutex;
MonotonicTime m_lastWheelEventTime;
Modified: trunk/Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp (269311 => 269312)
--- trunk/Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp 2020-11-03 17:53:32 UTC (rev 269311)
+++ trunk/Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp 2020-11-03 17:59:11 UTC (rev 269312)
@@ -146,16 +146,19 @@
deferWheelEventTestCompletionForReason(reinterpret_cast<WheelEventTestMonitor::ScrollableAreaIdentifier>(node.scrollingNodeID()), WheelEventTestMonitor::ScrollingThreadSyncNeeded);
#endif
- LOG_WITH_STREAM(Scrolling, stream << "ThreadedScrollingTree::scrollingTreeNodeDidScroll " << node.scrollingNodeID() << " to " << scrollPosition << " bouncing to main thread");
-
if (RunLoop::isMain()) {
- m_scrollingCoordinator->updateScrollPositionAfterAsyncScroll(node.scrollingNodeID(), scrollPosition, layoutViewportOrigin, ScrollType::User, scrollingLayerPositionAction);
+ m_scrollingCoordinator->applyScrollUpdate(node.scrollingNodeID(), scrollPosition, layoutViewportOrigin, ScrollType::User, scrollingLayerPositionAction);
return;
}
- RunLoop::main().dispatch([strongThis = makeRef(*this), nodeID = node.scrollingNodeID(), scrollPosition, layoutViewportOrigin, scrollingLayerPositionAction] {
+ LOG_WITH_STREAM(Scrolling, stream << "ThreadedScrollingTree::scrollingTreeNodeDidScroll " << node.scrollingNodeID() << " to " << scrollPosition << " bouncing to main thread");
+
+ auto scrollUpdate = ScrollUpdate { node.scrollingNodeID(), scrollPosition, layoutViewportOrigin, scrollingLayerPositionAction };
+ addPendingScrollUpdate(WTFMove(scrollUpdate));
+
+ RunLoop::main().dispatch([strongThis = makeRef(*this)] {
if (auto* scrollingCoordinator = strongThis->m_scrollingCoordinator.get())
- scrollingCoordinator->scheduleUpdateScrollPositionAfterAsyncScroll(nodeID, scrollPosition, layoutViewportOrigin, scrollingLayerPositionAction);
+ scrollingCoordinator->applyPendingScrollUpdates();
});
}
Modified: trunk/Source/WebKit/ChangeLog (269311 => 269312)
--- trunk/Source/WebKit/ChangeLog 2020-11-03 17:53:32 UTC (rev 269311)
+++ trunk/Source/WebKit/ChangeLog 2020-11-03 17:59:11 UTC (rev 269312)
@@ -1,3 +1,13 @@
+2020-11-02 Simon Fraser <[email protected]>
+
+ Scroll position can get reset after programmatic scroll
+ https://bugs.webkit.org/show_bug.cgi?id=218477
+
+ Reviewed by Antti Koivisto.
+
+ * WebProcess/WebPage/RemoteLayerTree/RemoteScrollingCoordinator.mm:
+ (WebKit::RemoteScrollingCoordinator::scrollPositionChangedForNode):
+
2020-11-03 Brent Fulgham <[email protected]>
[macOS] Adopt additional QuartzCore entitlement to reduce accessible endpoints
Modified: trunk/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteScrollingCoordinator.mm (269311 => 269312)
--- trunk/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteScrollingCoordinator.mm 2020-11-03 17:53:32 UTC (rev 269311)
+++ trunk/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteScrollingCoordinator.mm 2020-11-03 17:59:11 UTC (rev 269312)
@@ -107,7 +107,7 @@
// Notification from the UI process that we scrolled.
void RemoteScrollingCoordinator::scrollPositionChangedForNode(ScrollingNodeID nodeID, const FloatPoint& scrollPosition, bool syncLayerPosition)
{
- scheduleUpdateScrollPositionAfterAsyncScroll(nodeID, scrollPosition, WTF::nullopt, syncLayerPosition ? ScrollingLayerPositionAction::Sync : ScrollingLayerPositionAction::Set);
+ applyScrollUpdate(nodeID, scrollPosition, WTF::nullopt, ScrollType::User, syncLayerPosition ? ScrollingLayerPositionAction::Sync : ScrollingLayerPositionAction::Set);
}
void RemoteScrollingCoordinator::currentSnapPointIndicesChangedForNode(ScrollingNodeID nodeID, unsigned horizontal, unsigned vertical)
