Dynamically added position:fixed element is in the wrong place
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 2 May 2017 23:06:13 +0000 (23:06 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 2 May 2017 23:06:13 +0000 (23:06 +0000)
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.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@216104 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/scrollingcoordinator/ios/nested-fixed-layer-positions-expected.html [new file with mode: 0644]
LayoutTests/scrollingcoordinator/ios/nested-fixed-layer-positions.html [new file with mode: 0644]
Source/WebKit2/ChangeLog
Source/WebKit2/WebProcess/WebPage/mac/PlatformCALayerRemote.cpp

index c17f15a..3ae3d36 100644 (file)
@@ -1,3 +1,14 @@
+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>
 
         Move flaky expectation for svg/animations/getCurrentTime-pause-unpause.html ios-wk1 to ios TestExpectations file.
diff --git a/LayoutTests/scrollingcoordinator/ios/nested-fixed-layer-positions-expected.html b/LayoutTests/scrollingcoordinator/ios/nested-fixed-layer-positions-expected.html
new file mode 100644 (file)
index 0000000..602d25f
--- /dev/null
@@ -0,0 +1,29 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true ] -->
+
+<html>
+<head>
+    <meta name="viewport" content="initial-scale=1.0">
+    <style>
+        body {
+            height: 2000px;
+            margin: 0;
+        }
+
+        .fixed {
+            position: fixed;
+        }
+        
+        .fixed-bar {
+            position: fixed;
+            top: 10px;
+            left: 0;
+            height: 100px;
+            width: 400px;
+            background-color: blue;
+        }
+    </style>
+</head>
+<body class="fixed">
+    <div class="fixed-bar"></div>
+</body>
+</html>
diff --git a/LayoutTests/scrollingcoordinator/ios/nested-fixed-layer-positions.html b/LayoutTests/scrollingcoordinator/ios/nested-fixed-layer-positions.html
new file mode 100644 (file)
index 0000000..43636ef
--- /dev/null
@@ -0,0 +1,62 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true ] -->
+
+<html>
+<head>
+    <meta name="viewport" content="initial-scale=1.0">
+    <style>
+        body {
+            height: 2000px;
+            margin: 0;
+        }
+
+        .fixed {
+            position: fixed;
+        }
+        
+        .fixed-bar {
+            position: fixed;
+            top: 10px;
+            left: 0;
+            height: 100px;
+            width: 400px;
+            background-color: blue;
+        }
+    </style>
+    <script>
+        if (window.testRunner)
+            testRunner.waitUntilDone();
+
+        function getScrollDownUIScript()
+        {
+            return `(function() {
+                uiController.scrollToOffset(0, 300);
+                
+                uiController.didEndScrollingCallback = function() {
+                    uiController.uiScriptComplete();
+                };
+            })();`;
+        }
+        
+        function makeBodyFixed()
+        {
+            document.body.classList.add('fixed');
+            testRunner.notifyDone();
+        }
+
+        function doTest()
+        {
+            if (!testRunner.runUIScript)
+                return
+
+            testRunner.runUIScript(getScrollDownUIScript(), function() {
+                makeBodyFixed();
+            });
+        }
+        
+        window.addEventListener('load', doTest, false);
+    </script>
+</head>
+<body>
+    <div class="fixed-bar"></div>
+</body>
+</html>
index 20465f9..467a64a 100644 (file)
@@ -1,3 +1,26 @@
+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>
 
         [GTK] Drop coordinated surfaces from the compositing thread as soon as possible
index 09e8b8b..c1d8b88 100644 (file)
@@ -452,9 +452,9 @@ FloatPoint3D PlatformCALayerRemote::position() const
 
 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);
 }