Changeset 216104 in webkit

Timestamp:

May 2, 2017, 4:06:13 PM (8 years ago)

Author:

Simon Fraser

Message:

Dynamically added position:fixed element is in the wrong place
​https://bugs.webkit.org/show_bug.cgi?id=170280
rdar://problem/31374008

Reviewed by Tim Horton.

Source/WebKit2:

Layers for position:fixed elements have their positions reconciled after scrolls
via AsyncScrollingCoordinator::reconcileViewportConstrainedLayerPositions() and GraphicsLayerCA::syncPosition(),
which updates the GraphicsLayer's position, but does not push it to the PlatformCALayer.

This bug was a case where a position:fixed element gained a fixed ancestor, so had a GraphicsLayerCA whose
position had been updated via syncPosition() in the past. A subsequent layout updated the GraphicsLayerCA position,
but to a value that was the same as its pre-sync position, so the PlatformCALayerRemote's position didn't change,
but there was no signal to the UI process to restore the layer's pre-scrolled position.

Fix by avoiding the early return in PlatformCALayerRemote::setPosition(), to ensure that GraphicsLayerCA
geometry updates in the web process always send new positions to the UI process.

• WebProcess/WebPage/mac/PlatformCALayerRemote.cpp:

(WebKit::PlatformCALayerRemote::setPosition):

LayoutTests:

• scrollingcoordinator/ios/nested-fixed-layer-positions-expected.html: Added.
• scrollingcoordinator/ios/nested-fixed-layer-positions.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/scrollingcoordinator/ios/nested-fixed-layer-positions-expected.html (added)
• LayoutTests/scrollingcoordinator/ios/nested-fixed-layer-positions.html (added)
• Source/WebKit2/ChangeLog (modified) (1 diff)
• Source/WebKit2/WebProcess/WebPage/mac/PlatformCALayerRemote.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2017-05-02  Simon Fraser  <simon.fraser@apple.com>
+
+        Dynamically added position:fixed element is in the wrong place
+        https://bugs.webkit.org/show_bug.cgi?id=170280
+        rdar://problem/31374008
+
+        Reviewed by Tim Horton.
+
+        * scrollingcoordinator/ios/nested-fixed-layer-positions-expected.html: Added.
+        * scrollingcoordinator/ios/nested-fixed-layer-positions.html: Added.
+
 2017-05-02  Ryan Haddad  <ryanhaddad@apple.com>
 

trunk/Source/WebKit2/ChangeLog

@@ +1 @@
+2017-05-02  Simon Fraser  <simon.fraser@apple.com>
+
+        Dynamically added position:fixed element is in the wrong place
+        https://bugs.webkit.org/show_bug.cgi?id=170280
+        rdar://problem/31374008
+
+        Reviewed by Tim Horton.
+
+        Layers for position:fixed elements have their positions reconciled after scrolls
+        via AsyncScrollingCoordinator::reconcileViewportConstrainedLayerPositions() and GraphicsLayerCA::syncPosition(),
+        which updates the GraphicsLayer's position, but does not push it to the PlatformCALayer.
+
+        This bug was a case where a position:fixed element gained a fixed ancestor, so had a GraphicsLayerCA whose
+        position had been updated via syncPosition() in the past. A subsequent layout updated the GraphicsLayerCA position,
+        but to a value that was the same as its pre-sync position, so the PlatformCALayerRemote's position didn't change,
+        but there was no signal to the UI process to restore the layer's pre-scrolled position.
+
+        Fix by avoiding the early return in PlatformCALayerRemote::setPosition(), to ensure that GraphicsLayerCA
+        geometry updates in the web process always send new positions to the UI process.
+
+        * WebProcess/WebPage/mac/PlatformCALayerRemote.cpp:
+        (WebKit::PlatformCALayerRemote::setPosition):
+
 2017-05-02  Gwang Yoon Hwang  <yoon@igalia.com>
 

trunk/Source/WebKit2/WebProcess/WebPage/mac/PlatformCALayerRemote.cpp

@@ -453 +453 @@
 void PlatformCALayerRemote::setPosition(const FloatPoint3D& value)
 {
-    if (value == m_properties.position)
-        return;
-
+    // We can't early return here if the position has not changed, since GraphicsLayerCA::syncPosition() may have changed
+    // the GraphicsLayer position (which doesn't force a geometry update) but we want a subsequent GraphicsLayerCA::setPosition()
+    // to push a new position to the UI process, even though our m_properties.position hasn't changed.
     m_properties.position = value;
     m_properties.notePropertiesChanged(RemoteLayerTreeTransaction::PositionChanged);