- Revision
- 171532
- Author
- benja...@webkit.org
- Date
- 2014-07-24 15:25:59 -0700 (Thu, 24 Jul 2014)
Log Message
[WK2] Fixed/Sticky layers can get mispositioned when the layer tree commit change their position or size
https://bugs.webkit.org/show_bug.cgi?id=135227
<rdar://problem/17279500>
Reviewed by Simon Fraser.
Source/WebCore:
Keep track of the creation/destruction of Fixed and Sticky nodes in the ScrollingTree.
* page/scrolling/ScrollingTree.cpp:
(WebCore::ScrollingTree::ScrollingTree):
* page/scrolling/ScrollingTree.h:
(WebCore::ScrollingTree::hasFixedOrSticky):
(WebCore::ScrollingTree::fixedOrStickyNodeAdded):
(WebCore::ScrollingTree::fixedOrStickyNodeRemoved):
* page/scrolling/mac/ScrollingTreeFixedNode.mm:
(WebCore::ScrollingTreeFixedNode::ScrollingTreeFixedNode):
(WebCore::ScrollingTreeFixedNode::~ScrollingTreeFixedNode):
* page/scrolling/mac/ScrollingTreeStickyNode.mm:
(WebCore::ScrollingTreeStickyNode::ScrollingTreeStickyNode):
(WebCore::ScrollingTreeStickyNode::~ScrollingTreeStickyNode):
Source/WebKit2:
In some cases, a fixed or sticky positioned layer would end up at its position corresponding to the WebProcess
instead of sticking the to real viewport in the UIProcess.
The sequence of event is:
1) A layer becomes fixed in some ScrollingTree transaction.
2) Later, some change in the WebProcess causes a LayerTree update for that exact same layer, but no corresponding
ScrollingTree update is made.
3) In the UIProcess, the position of the fixed layer is changed due to the LayerTree update.
But! There is no ScrollingTree change, updateScrollingTree() never sets fixedOrStickyLayerChanged to true,
and the position is not corrected.
-> The layer is now at the wrong position until the next VisibleContentRectUpdate.
Ideally, we should have fixedOrStickyLayerChanged track if either the position or size of a fixed layer changed
in the layer tree. This is tricky since the layer tree does not keep track of the fixed nodes of the scrolling tree.
Since this complexity seems risky at this point, I went for something simpler but with more overhead:
any time the scrolling tree contains either a fixed or sticky layer, viewportChangedViaDelegatedScrolling()
is called to "fix" the position.
* UIProcess/Scrolling/RemoteScrollingCoordinatorProxy.cpp:
(WebKit::RemoteScrollingCoordinatorProxy::updateScrollingTree):
(WebKit::RemoteScrollingCoordinatorProxy::connectStateNodeLayers):
* UIProcess/Scrolling/RemoteScrollingCoordinatorProxy.h:
(WebKit::RemoteScrollingCoordinatorProxy::hasFixedOrSticky):
* UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:
(WebKit::RemoteScrollingCoordinatorProxy::connectStateNodeLayers):
* UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm:
(WebKit::RemoteLayerTreeDrawingAreaProxy::commitLayerTree):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (171531 => 171532)
--- trunk/Source/WebCore/ChangeLog 2014-07-24 22:17:58 UTC (rev 171531)
+++ trunk/Source/WebCore/ChangeLog 2014-07-24 22:25:59 UTC (rev 171532)
@@ -1,3 +1,26 @@
+2014-07-24 Benjamin Poulain <benja...@webkit.org>
+
+ [WK2] Fixed/Sticky layers can get mispositioned when the layer tree commit change their position or size
+ https://bugs.webkit.org/show_bug.cgi?id=135227
+ <rdar://problem/17279500>
+
+ Reviewed by Simon Fraser.
+
+ Keep track of the creation/destruction of Fixed and Sticky nodes in the ScrollingTree.
+
+ * page/scrolling/ScrollingTree.cpp:
+ (WebCore::ScrollingTree::ScrollingTree):
+ * page/scrolling/ScrollingTree.h:
+ (WebCore::ScrollingTree::hasFixedOrSticky):
+ (WebCore::ScrollingTree::fixedOrStickyNodeAdded):
+ (WebCore::ScrollingTree::fixedOrStickyNodeRemoved):
+ * page/scrolling/mac/ScrollingTreeFixedNode.mm:
+ (WebCore::ScrollingTreeFixedNode::ScrollingTreeFixedNode):
+ (WebCore::ScrollingTreeFixedNode::~ScrollingTreeFixedNode):
+ * page/scrolling/mac/ScrollingTreeStickyNode.mm:
+ (WebCore::ScrollingTreeStickyNode::ScrollingTreeStickyNode):
+ (WebCore::ScrollingTreeStickyNode::~ScrollingTreeStickyNode):
+
2014-07-24 Wenson Hsieh <wenson_hs...@apple.com>
Let WheelEvent wrap a PlatformWheelEvent
Modified: trunk/Source/WebCore/page/scrolling/ScrollingTree.cpp (171531 => 171532)
--- trunk/Source/WebCore/page/scrolling/ScrollingTree.cpp 2014-07-24 22:17:58 UTC (rev 171531)
+++ trunk/Source/WebCore/page/scrolling/ScrollingTree.cpp 2014-07-24 22:25:59 UTC (rev 171532)
@@ -51,6 +51,7 @@
, m_latchedNode(0)
, m_scrollingPerformanceLoggingEnabled(false)
, m_isHandlingProgrammaticScroll(false)
+ , m_fixedOrStickyNodeCount(0)
{
}
Modified: trunk/Source/WebCore/page/scrolling/ScrollingTree.h (171531 => 171532)
--- trunk/Source/WebCore/page/scrolling/ScrollingTree.h 2014-07-24 22:17:58 UTC (rev 171531)
+++ trunk/Source/WebCore/page/scrolling/ScrollingTree.h 2014-07-24 22:25:59 UTC (rev 171532)
@@ -128,6 +128,14 @@
bool hasLatchedNode() const { return m_latchedNode; }
void setOrClearLatchedNode(const PlatformWheelEvent&, ScrollingNodeID);
+
+ bool hasFixedOrSticky() const { return !!m_fixedOrStickyNodeCount; }
+ void fixedOrStickyNodeAdded() { ++m_fixedOrStickyNodeCount; }
+ void fixedOrStickyNodeRemoved()
+ {
+ ASSERT(m_fixedOrStickyNodeCount);
+ --m_fixedOrStickyNodeCount;
+ }
protected:
void setMainFrameScrollPosition(FloatPoint);
@@ -167,6 +175,7 @@
bool m_scrollingPerformanceLoggingEnabled;
bool m_isHandlingProgrammaticScroll;
+ unsigned m_fixedOrStickyNodeCount;
};
#define SCROLLING_TREE_TYPE_CASTS(ToValueTypeName, predicate) \
Modified: trunk/Source/WebCore/page/scrolling/mac/ScrollingTreeFixedNode.mm (171531 => 171532)
--- trunk/Source/WebCore/page/scrolling/mac/ScrollingTreeFixedNode.mm 2014-07-24 22:17:58 UTC (rev 171531)
+++ trunk/Source/WebCore/page/scrolling/mac/ScrollingTreeFixedNode.mm 2014-07-24 22:25:59 UTC (rev 171532)
@@ -42,10 +42,12 @@
ScrollingTreeFixedNode::ScrollingTreeFixedNode(ScrollingTree& scrollingTree, ScrollingNodeID nodeID)
: ScrollingTreeNode(scrollingTree, FixedNode, nodeID)
{
+ scrollingTree.fixedOrStickyNodeAdded();
}
ScrollingTreeFixedNode::~ScrollingTreeFixedNode()
{
+ scrollingTree().fixedOrStickyNodeRemoved();
}
void ScrollingTreeFixedNode::updateBeforeChildren(const ScrollingStateNode& stateNode)
Modified: trunk/Source/WebCore/page/scrolling/mac/ScrollingTreeStickyNode.mm (171531 => 171532)
--- trunk/Source/WebCore/page/scrolling/mac/ScrollingTreeStickyNode.mm 2014-07-24 22:17:58 UTC (rev 171531)
+++ trunk/Source/WebCore/page/scrolling/mac/ScrollingTreeStickyNode.mm 2014-07-24 22:25:59 UTC (rev 171532)
@@ -43,10 +43,12 @@
ScrollingTreeStickyNode::ScrollingTreeStickyNode(ScrollingTree& scrollingTree, ScrollingNodeID nodeID)
: ScrollingTreeNode(scrollingTree, StickyNode, nodeID)
{
+ scrollingTree.fixedOrStickyNodeAdded();
}
ScrollingTreeStickyNode::~ScrollingTreeStickyNode()
{
+ scrollingTree().fixedOrStickyNodeRemoved();
}
void ScrollingTreeStickyNode::updateBeforeChildren(const ScrollingStateNode& stateNode)
Modified: trunk/Source/WebKit2/ChangeLog (171531 => 171532)
--- trunk/Source/WebKit2/ChangeLog 2014-07-24 22:17:58 UTC (rev 171531)
+++ trunk/Source/WebKit2/ChangeLog 2014-07-24 22:25:59 UTC (rev 171532)
@@ -1,3 +1,40 @@
+2014-07-24 Benjamin Poulain <benja...@webkit.org>
+
+ [WK2] Fixed/Sticky layers can get mispositioned when the layer tree commit change their position or size
+ https://bugs.webkit.org/show_bug.cgi?id=135227
+ <rdar://problem/17279500>
+
+ Reviewed by Simon Fraser.
+
+ In some cases, a fixed or sticky positioned layer would end up at its position corresponding to the WebProcess
+ instead of sticking the to real viewport in the UIProcess.
+
+ The sequence of event is:
+ 1) A layer becomes fixed in some ScrollingTree transaction.
+ 2) Later, some change in the WebProcess causes a LayerTree update for that exact same layer, but no corresponding
+ ScrollingTree update is made.
+ 3) In the UIProcess, the position of the fixed layer is changed due to the LayerTree update.
+ But! There is no ScrollingTree change, updateScrollingTree() never sets fixedOrStickyLayerChanged to true,
+ and the position is not corrected.
+ -> The layer is now at the wrong position until the next VisibleContentRectUpdate.
+
+ Ideally, we should have fixedOrStickyLayerChanged track if either the position or size of a fixed layer changed
+ in the layer tree. This is tricky since the layer tree does not keep track of the fixed nodes of the scrolling tree.
+
+ Since this complexity seems risky at this point, I went for something simpler but with more overhead:
+ any time the scrolling tree contains either a fixed or sticky layer, viewportChangedViaDelegatedScrolling()
+ is called to "fix" the position.
+
+ * UIProcess/Scrolling/RemoteScrollingCoordinatorProxy.cpp:
+ (WebKit::RemoteScrollingCoordinatorProxy::updateScrollingTree):
+ (WebKit::RemoteScrollingCoordinatorProxy::connectStateNodeLayers):
+ * UIProcess/Scrolling/RemoteScrollingCoordinatorProxy.h:
+ (WebKit::RemoteScrollingCoordinatorProxy::hasFixedOrSticky):
+ * UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:
+ (WebKit::RemoteScrollingCoordinatorProxy::connectStateNodeLayers):
+ * UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm:
+ (WebKit::RemoteLayerTreeDrawingAreaProxy::commitLayerTree):
+
2014-07-24 Oliver Hunt <oli...@apple.com>
Need to explicitly support location services in webcontent profile
Modified: trunk/Source/WebKit2/UIProcess/Scrolling/RemoteScrollingCoordinatorProxy.cpp (171531 => 171532)
--- trunk/Source/WebKit2/UIProcess/Scrolling/RemoteScrollingCoordinatorProxy.cpp 2014-07-24 22:17:58 UTC (rev 171531)
+++ trunk/Source/WebKit2/UIProcess/Scrolling/RemoteScrollingCoordinatorProxy.cpp 2014-07-24 22:25:59 UTC (rev 171532)
@@ -35,7 +35,6 @@
#include "RemoteScrollingCoordinator.h"
#include "RemoteScrollingCoordinatorMessages.h"
#include "RemoteScrollingCoordinatorTransaction.h"
-#include "RemoteScrollingTree.h"
#include "WebPageProxy.h"
#include "WebProcessProxy.h"
#include <WebCore/ScrollingStateFrameScrollingNode.h>
@@ -78,7 +77,7 @@
return &remoteDrawingArea->remoteLayerTreeHost();
}
-void RemoteScrollingCoordinatorProxy::updateScrollingTree(const RemoteScrollingCoordinatorTransaction& transaction, bool& fixedOrStickyLayerChanged)
+void RemoteScrollingCoordinatorProxy::updateScrollingTree(const RemoteScrollingCoordinatorTransaction& transaction)
{
OwnPtr<ScrollingStateTree> stateTree = const_cast<RemoteScrollingCoordinatorTransaction&>(transaction).scrollingStateTree().release();
@@ -88,12 +87,12 @@
return;
}
- connectStateNodeLayers(*stateTree, *layerTreeHost, fixedOrStickyLayerChanged);
+ connectStateNodeLayers(*stateTree, *layerTreeHost);
m_scrollingTree->commitNewTreeState(stateTree.release());
}
#if !PLATFORM(IOS)
-void RemoteScrollingCoordinatorProxy::connectStateNodeLayers(ScrollingStateTree& stateTree, const RemoteLayerTreeHost& layerTreeHost, bool& fixedOrStickyLayerChanged)
+void RemoteScrollingCoordinatorProxy::connectStateNodeLayers(ScrollingStateTree& stateTree, const RemoteLayerTreeHost& layerTreeHost)
{
for (auto& currNode : stateTree.nodeMap().values()) {
if (currNode->hasChangedProperty(ScrollingStateNode::ScrollLayer))
@@ -131,12 +130,7 @@
break;
}
case FixedNode:
- if (currNode->hasChangedProperty(ScrollingStateNode::ScrollLayer))
- fixedOrStickyLayerChanged = true;
- break;
case StickyNode:
- if (currNode->hasChangedProperty(ScrollingStateNode::ScrollLayer))
- fixedOrStickyLayerChanged = true;
break;
}
}
Modified: trunk/Source/WebKit2/UIProcess/Scrolling/RemoteScrollingCoordinatorProxy.h (171531 => 171532)
--- trunk/Source/WebKit2/UIProcess/Scrolling/RemoteScrollingCoordinatorProxy.h 2014-07-24 22:17:58 UTC (rev 171531)
+++ trunk/Source/WebKit2/UIProcess/Scrolling/RemoteScrollingCoordinatorProxy.h 2014-07-24 22:25:59 UTC (rev 171532)
@@ -30,6 +30,7 @@
#include "MessageReceiver.h"
#include "RemoteScrollingCoordinator.h"
+#include "RemoteScrollingTree.h"
#include <wtf/Noncopyable.h>
#include <wtf/RefPtr.h>
@@ -67,10 +68,11 @@
const RemoteLayerTreeHost* layerTreeHost() const;
- void updateScrollingTree(const RemoteScrollingCoordinatorTransaction&, bool& fixedOrStickyLayerChanged);
+ void updateScrollingTree(const RemoteScrollingCoordinatorTransaction&);
void setPropagatesMainFrameScrolls(bool propagatesMainFrameScrolls) { m_propagatesMainFrameScrolls = propagatesMainFrameScrolls; }
bool propagatesMainFrameScrolls() const { return m_propagatesMainFrameScrolls; }
+ bool hasFixedOrSticky() const { return m_scrollingTree->hasFixedOrSticky(); }
#if PLATFORM(IOS)
WebCore::FloatRect customFixedPositionRect() const;
@@ -80,7 +82,7 @@
#endif
private:
- void connectStateNodeLayers(WebCore::ScrollingStateTree&, const RemoteLayerTreeHost&, bool& fixedOrStickyLayerChanged);
+ void connectStateNodeLayers(WebCore::ScrollingStateTree&, const RemoteLayerTreeHost&);
WebPageProxy& m_webPageProxy;
RefPtr<RemoteScrollingTree> m_scrollingTree;
Modified: trunk/Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm (171531 => 171532)
--- trunk/Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm 2014-07-24 22:17:58 UTC (rev 171531)
+++ trunk/Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm 2014-07-24 22:25:59 UTC (rev 171532)
@@ -46,7 +46,7 @@
return LayerRepresentation(layerOrView.layer);
}
-void RemoteScrollingCoordinatorProxy::connectStateNodeLayers(ScrollingStateTree& stateTree, const RemoteLayerTreeHost& layerTreeHost, bool& fixedOrStickyLayerChanged)
+void RemoteScrollingCoordinatorProxy::connectStateNodeLayers(ScrollingStateTree& stateTree, const RemoteLayerTreeHost& layerTreeHost)
{
for (auto& currNode : stateTree.nodeMap().values()) {
switch (currNode->nodeType()) {
@@ -78,16 +78,9 @@
break;
}
case FixedNode:
- if (currNode->hasChangedProperty(ScrollingStateNode::ScrollLayer))
- currNode->setLayer(layerRepresentationFromLayerOrView(layerTreeHost.getLayer(currNode->layer())));
-
- fixedOrStickyLayerChanged |= currNode->hasChangedProperties();
- break;
case StickyNode:
if (currNode->hasChangedProperty(ScrollingStateNode::ScrollLayer))
currNode->setLayer(layerRepresentationFromLayerOrView(layerTreeHost.getLayer(currNode->layer())));
-
- fixedOrStickyLayerChanged |= currNode->hasChangedProperties();
break;
}
}
Modified: trunk/Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm (171531 => 171532)
--- trunk/Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm 2014-07-24 22:17:58 UTC (rev 171531)
+++ trunk/Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm 2014-07-24 22:25:59 UTC (rev 171532)
@@ -205,11 +205,10 @@
#endif
#if ENABLE(ASYNC_SCROLLING)
- bool fixedOrStickyLayerChanged = false;
- m_webPageProxy->scrollingCoordinatorProxy()->updateScrollingTree(scrollingTreeTransaction, fixedOrStickyLayerChanged);
+ m_webPageProxy->scrollingCoordinatorProxy()->updateScrollingTree(scrollingTreeTransaction);
#if PLATFORM(IOS)
- if (fixedOrStickyLayerChanged) {
+ if (m_webPageProxy->scrollingCoordinatorProxy()->hasFixedOrSticky()) {
FloatRect customFixedPositionRect = m_webPageProxy->computeCustomFixedPositionRect(m_webPageProxy->unobscuredContentRect(), m_webPageProxy->displayedContentScale());
// If we got a new layer for a fixed or sticky node, its position from the WebProcess is probably stale. We need to re-run the "viewport" changed logic to udpate it with our UI-side state.
m_webPageProxy->scrollingCoordinatorProxy()->viewportChangedViaDelegatedScrolling(m_webPageProxy->scrollingCoordinatorProxy()->rootScrollingNodeID(), customFixedPositionRect, m_webPageProxy->displayedContentScale());
