- Revision
- 243607
- Author
- [email protected]
- Date
- 2019-03-28 09:44:27 -0700 (Thu, 28 Mar 2019)
Log Message
[macOS WK2] Overlays on instagram.com are shifted if you click on a photo after scrolling
https://bugs.webkit.org/show_bug.cgi?id=196330
rdar://problem/49100304
Source/WebCore:
Reviewed by Antti Koivisto.
When we call ScrollingTree::applyLayerPositions() on the main thread after a flush,
we need to ensure that the most recent version of the scrolling tree has been committed,
because it has to have state (like requested scroll position and layout viewport rect)
that match the layer flush.
To fix this we have to have the main thread wait for the commit to complete, so
ThreadedScrollingTree keeps track of a pending commit count, and uses a condition
variable to allow the main thread to safely wait for it to reach zero.
Tracing shows that this works as expected, and the main thread is never blocked for
more than a few tens of microseconds.
Also lock the tree mutex in ScrollingTree::handleWheelEvent(), since we enter the
scrolling tree here and we don't want that racing with applyLayerPositions() on the
main thread.
Test: scrollingcoordinator/mac/fixed-scrolled-body.html
* page/scrolling/ScrollingTree.cpp:
(WebCore::ScrollingTree::handleWheelEvent):
(WebCore::ScrollingTree::applyLayerPositions):
* page/scrolling/ScrollingTree.h:
* page/scrolling/ThreadedScrollingTree.cpp:
(WebCore::ThreadedScrollingTree::commitTreeState):
(WebCore::ThreadedScrollingTree::incrementPendingCommitCount):
(WebCore::ThreadedScrollingTree::decrementPendingCommitCount):
(WebCore::ThreadedScrollingTree::waitForPendingCommits):
(WebCore::ThreadedScrollingTree::applyLayerPositions):
* page/scrolling/ThreadedScrollingTree.h:
* page/scrolling/mac/ScrollingCoordinatorMac.mm:
(WebCore::ScrollingCoordinatorMac::commitTreeState):
LayoutTests:
Reviewed by NOBODY (OOPS!).
* scrollingcoordinator/mac/fixed-scrolled-body-expected.html: Added.
* scrollingcoordinator/mac/fixed-scrolled-body.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (243606 => 243607)
--- trunk/LayoutTests/ChangeLog 2019-03-28 16:36:18 UTC (rev 243606)
+++ trunk/LayoutTests/ChangeLog 2019-03-28 16:44:27 UTC (rev 243607)
@@ -1,3 +1,14 @@
+2019-03-28 Simon Fraser <[email protected]>
+
+ [macOS WK2] Overlays on instagram.com are shifted if you click on a photo after scrolling
+ https://bugs.webkit.org/show_bug.cgi?id=196330
+ rdar://problem/49100304
+
+ Reviewed by Antti Koivisto.
+
+ * scrollingcoordinator/mac/fixed-scrolled-body-expected.html: Added.
+ * scrollingcoordinator/mac/fixed-scrolled-body.html: Added.
+
2019-03-28 Zalan Bujtas <[email protected]>
[SimpleLineLayout] Disable SLL when text-underline-position is not auto.
Added: trunk/LayoutTests/scrollingcoordinator/mac/fixed-scrolled-body-expected.html (0 => 243607)
--- trunk/LayoutTests/scrollingcoordinator/mac/fixed-scrolled-body-expected.html (rev 0)
+++ trunk/LayoutTests/scrollingcoordinator/mac/fixed-scrolled-body-expected.html 2019-03-28 16:44:27 UTC (rev 243607)
@@ -0,0 +1,29 @@
+<!DOCTYPE html>
+<html>
+<head>
+ <style>
+ body, html {
+ height: 100%;
+ margin: 0;
+ position: fixed;
+ top: -200px;
+ }
+ .section {
+ height: 2000px;
+ }
+ .overlay {
+ position: fixed;
+ left: 0;
+ top: 0;
+ width: 400px;
+ height: 100px;
+ background-color: blue;
+ }
+ </style>
+</head>
+<body>
+ <div class="section"></div>
+ <div class="overlay"></div>
+</body>
+</html>
+
Added: trunk/LayoutTests/scrollingcoordinator/mac/fixed-scrolled-body.html (0 => 243607)
--- trunk/LayoutTests/scrollingcoordinator/mac/fixed-scrolled-body.html (rev 0)
+++ trunk/LayoutTests/scrollingcoordinator/mac/fixed-scrolled-body.html 2019-03-28 16:44:27 UTC (rev 243607)
@@ -0,0 +1,46 @@
+<!DOCTYPE html>
+<html>
+<head>
+ <style>
+ body, html {
+ height: 100%;
+ width: 100%;
+ margin: 0;
+ }
+ .section {
+ height: 2000px;
+ }
+ .overlay {
+ position: fixed;
+ left: 0;
+ top: 0;
+ width: 400px;
+ height: 100px;
+ background-color: blue;
+ }
+ </style>
+ <script>
+ if (window.testRunner)
+ testRunner.waitUntilDone();
+
+ function doTest()
+ {
+ window.scrollTo(0, 200);
+ requestAnimationFrame(() => {
+ document.body.style.top = -window.scrollY + 'px';
+ document.body.style.position = 'fixed';
+
+ if (window.testRunner)
+ testRunner.notifyDone();
+ });
+ }
+
+ window.addEventListener('load', doTest, false);
+ </script>
+</head>
+<body>
+ <div class="section"></div>
+ <div class="overlay"></div>
+</body>
+</html>
+
Modified: trunk/Source/WebCore/ChangeLog (243606 => 243607)
--- trunk/Source/WebCore/ChangeLog 2019-03-28 16:36:18 UTC (rev 243606)
+++ trunk/Source/WebCore/ChangeLog 2019-03-28 16:44:27 UTC (rev 243607)
@@ -1,3 +1,43 @@
+2019-03-28 Simon Fraser <[email protected]>
+
+ [macOS WK2] Overlays on instagram.com are shifted if you click on a photo after scrolling
+ https://bugs.webkit.org/show_bug.cgi?id=196330
+ rdar://problem/49100304
+
+ Reviewed by Antti Koivisto.
+
+ When we call ScrollingTree::applyLayerPositions() on the main thread after a flush,
+ we need to ensure that the most recent version of the scrolling tree has been committed,
+ because it has to have state (like requested scroll position and layout viewport rect)
+ that match the layer flush.
+
+ To fix this we have to have the main thread wait for the commit to complete, so
+ ThreadedScrollingTree keeps track of a pending commit count, and uses a condition
+ variable to allow the main thread to safely wait for it to reach zero.
+
+ Tracing shows that this works as expected, and the main thread is never blocked for
+ more than a few tens of microseconds.
+
+ Also lock the tree mutex in ScrollingTree::handleWheelEvent(), since we enter the
+ scrolling tree here and we don't want that racing with applyLayerPositions() on the
+ main thread.
+
+ Test: scrollingcoordinator/mac/fixed-scrolled-body.html
+
+ * page/scrolling/ScrollingTree.cpp:
+ (WebCore::ScrollingTree::handleWheelEvent):
+ (WebCore::ScrollingTree::applyLayerPositions):
+ * page/scrolling/ScrollingTree.h:
+ * page/scrolling/ThreadedScrollingTree.cpp:
+ (WebCore::ThreadedScrollingTree::commitTreeState):
+ (WebCore::ThreadedScrollingTree::incrementPendingCommitCount):
+ (WebCore::ThreadedScrollingTree::decrementPendingCommitCount):
+ (WebCore::ThreadedScrollingTree::waitForPendingCommits):
+ (WebCore::ThreadedScrollingTree::applyLayerPositions):
+ * page/scrolling/ThreadedScrollingTree.h:
+ * page/scrolling/mac/ScrollingCoordinatorMac.mm:
+ (WebCore::ScrollingCoordinatorMac::commitTreeState):
+
2019-03-28 Zalan Bujtas <[email protected]>
[SimpleLineLayout] Disable SLL when text-underline-position is not auto.
Modified: trunk/Source/WebCore/page/scrolling/ScrollingTree.cpp (243606 => 243607)
--- trunk/Source/WebCore/page/scrolling/ScrollingTree.cpp 2019-03-28 16:36:18 UTC (rev 243606)
+++ trunk/Source/WebCore/page/scrolling/ScrollingTree.cpp 2019-03-28 16:44:27 UTC (rev 243607)
@@ -93,6 +93,8 @@
{
LOG_WITH_STREAM(Scrolling, stream << "ScrollingTree " << this << " handleWheelEvent (async scrolling enabled: " << asyncFrameOrOverflowScrollingEnabled() << ")");
+ LockHolder locker(m_treeMutex);
+
if (!asyncFrameOrOverflowScrollingEnabled()) {
if (m_rootNode)
downcast<ScrollingTreeScrollingNode>(*m_rootNode).handleWheelEvent(wheelEvent);
@@ -106,7 +108,6 @@
return downcast<ScrollingTreeScrollingNode>(*node).handleWheelEvent(wheelEvent);
}
- LockHolder locker(m_treeMutex);
if (m_rootNode) {
auto& frameScrollingNode = downcast<ScrollingTreeFrameScrollingNode>(*m_rootNode);
@@ -260,9 +261,9 @@
node->commitStateAfterChildren(*stateNode);
}
-// Called from the main thread.
void ScrollingTree::applyLayerPositions()
{
+ ASSERT(isMainThread());
LockHolder locker(m_treeMutex);
if (!m_rootNode)
Modified: trunk/Source/WebCore/page/scrolling/ScrollingTree.h (243606 => 243607)
--- trunk/Source/WebCore/page/scrolling/ScrollingTree.h 2019-03-28 16:36:18 UTC (rev 243606)
+++ trunk/Source/WebCore/page/scrolling/ScrollingTree.h 2019-03-28 16:44:27 UTC (rev 243607)
@@ -69,7 +69,7 @@
virtual void invalidate() { }
WEBCORE_EXPORT virtual void commitTreeState(std::unique_ptr<ScrollingStateTree>);
- WEBCORE_EXPORT void applyLayerPositions();
+ WEBCORE_EXPORT virtual void applyLayerPositions();
virtual Ref<ScrollingTreeNode> createScrollingTreeNode(ScrollingNodeType, ScrollingNodeID) = 0;
@@ -153,7 +153,7 @@
HashSet<ScrollingNodeID>& positionedNodesWithRelatedOverflow() { return m_positionedNodesWithRelatedOverflow; }
WEBCORE_EXPORT String scrollingTreeAsText(ScrollingStateTreeAsTextBehavior = ScrollingStateTreeAsTextBehaviorNormal);
-
+
protected:
void setMainFrameScrollPosition(FloatPoint);
Modified: trunk/Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp (243606 => 243607)
--- trunk/Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp 2019-03-28 16:36:18 UTC (rev 243606)
+++ trunk/Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp 2019-03-28 16:44:27 UTC (rev 243607)
@@ -89,6 +89,8 @@
{
ASSERT(ScrollingThread::isCurrentThread());
ScrollingTree::commitTreeState(WTFMove(scrollingStateTree));
+
+ decrementPendingCommitCount();
}
void ThreadedScrollingTree::scrollingTreeNodeDidScroll(ScrollingTreeScrollingNode& node, ScrollingLayerPositionAction scrollingLayerPositionAction)
@@ -124,6 +126,35 @@
});
}
+void ThreadedScrollingTree::incrementPendingCommitCount()
+{
+ LockHolder commitLocker(m_pendingCommitCountMutex);
+ ++m_pendingCommitCount;
+}
+
+void ThreadedScrollingTree::decrementPendingCommitCount()
+{
+ LockHolder commitLocker(m_pendingCommitCountMutex);
+ ASSERT(m_pendingCommitCount > 0);
+ if (!--m_pendingCommitCount)
+ m_commitCondition.notifyOne();
+}
+
+void ThreadedScrollingTree::waitForPendingCommits()
+{
+ ASSERT(isMainThread());
+
+ LockHolder commitLocker(m_pendingCommitCountMutex);
+ while (m_pendingCommitCount)
+ m_commitCondition.wait(m_pendingCommitCountMutex);
+}
+
+void ThreadedScrollingTree::applyLayerPositions()
+{
+ waitForPendingCommits();
+ ScrollingTree::applyLayerPositions();
+}
+
#if PLATFORM(COCOA)
void ThreadedScrollingTree::currentSnapPointIndicesDidChange(ScrollingNodeID nodeID, unsigned horizontal, unsigned vertical)
{
Modified: trunk/Source/WebCore/page/scrolling/ThreadedScrollingTree.h (243606 => 243607)
--- trunk/Source/WebCore/page/scrolling/ThreadedScrollingTree.h 2019-03-28 16:36:18 UTC (rev 243606)
+++ trunk/Source/WebCore/page/scrolling/ThreadedScrollingTree.h 2019-03-28 16:44:27 UTC (rev 243607)
@@ -29,6 +29,7 @@
#include "ScrollingStateTree.h"
#include "ScrollingTree.h"
+#include <wtf/Condition.h>
#include <wtf/RefPtr.h>
namespace WebCore {
@@ -54,6 +55,9 @@
void invalidate() override;
+ void incrementPendingCommitCount();
+ void decrementPendingCommitCount();
+
protected:
explicit ThreadedScrollingTree(AsyncScrollingCoordinator&);
@@ -74,8 +78,15 @@
private:
bool isThreadedScrollingTree() const override { return true; }
+ void applyLayerPositions() override;
RefPtr<AsyncScrollingCoordinator> m_scrollingCoordinator;
+
+ void waitForPendingCommits();
+
+ Lock m_pendingCommitCountMutex;
+ unsigned m_pendingCommitCount { 0 };
+ Condition m_commitCondition;
};
} // namespace WebCore
Modified: trunk/Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm (243606 => 243607)
--- trunk/Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm 2019-03-28 16:36:18 UTC (rev 243606)
+++ trunk/Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm 2019-03-28 16:44:27 UTC (rev 243607)
@@ -122,6 +122,8 @@
RefPtr<ThreadedScrollingTree> threadedScrollingTree = downcast<ThreadedScrollingTree>(scrollingTree());
ScrollingStateTree* unprotectedTreeState = scrollingStateTree()->commit(LayerRepresentation::PlatformLayerRepresentation).release();
+ threadedScrollingTree->incrementPendingCommitCount();
+
ScrollingThread::dispatch([threadedScrollingTree, unprotectedTreeState] {
std::unique_ptr<ScrollingStateTree> treeState(unprotectedTreeState);
threadedScrollingTree->commitTreeState(WTFMove(treeState));
