Fixed elements bounce when rubber-banding at the bottom of the page
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 17 Feb 2017 19:11:59 +0000 (19:11 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 17 Feb 2017 19:11:59 +0000 (19:11 +0000)
https://bugs.webkit.org/show_bug.cgi?id=168493
rdar://problem/30567713

Reviewed by Tim Horton.

Source/WebCore:

FrameView::visibleDocumentRect() was computing a bad visible rect when bottom-rubber-banding,
by adding rubberBandBottom which is negative, rather than subtracting.

Log some more scrolling stuff.

Ironically, the existing test didn't test stick-to-viewport fixed position because
backgroundShouldExtendBeyondPage() is off by default in WTR, so clone it to a test
that sets this, to test both behaviors.

This also revealed that dynamic changes to backgroundShouldExtendBeyondPage() need
to be propagated to the scrolling tree, which is fixed in AsyncScrollingCoordinator::frameViewLayoutUpdated().

Test: fast/visual-viewport/rubberbanding-viewport-rects-extended-background.html

* page/FrameView.cpp:
(WebCore::FrameView::updateLayoutViewport):
(WebCore::FrameView::visibleDocumentRect):
* page/scrolling/AsyncScrollingCoordinator.cpp:
(WebCore::AsyncScrollingCoordinator::frameViewLayoutUpdated):
* page/scrolling/ScrollingStateFrameScrollingNode.cpp:
(WebCore::ScrollingStateFrameScrollingNode::dumpProperties):
* page/scrolling/ScrollingTreeFrameScrollingNode.cpp:
(WebCore::ScrollingTreeFrameScrollingNode::layoutViewportForScrollPosition):
* page/scrolling/mac/ScrollingCoordinatorMac.mm:
(WebCore::ScrollingCoordinatorMac::scheduleTreeStateCommit):
(WebCore::ScrollingCoordinatorMac::commitTreeState):

LayoutTests:

* fast/visual-viewport/rubberbanding-viewport-rects-expected.txt:
* fast/visual-viewport/rubberbanding-viewport-rects-extended-background-expected.txt: Copied from LayoutTests/fast/visual-viewport/rubberbanding-viewport-rects-expected.txt.
* fast/visual-viewport/rubberbanding-viewport-rects-extended-background.html: Copied from LayoutTests/fast/visual-viewport/rubberbanding-viewport-rects.html.
* fast/visual-viewport/rubberbanding-viewport-rects.html:
* platform/ios-simulator-wk2/fast/visual-viewport/rubberbanding-viewport-rects-expected.txt:
* platform/ios-simulator-wk2/fast/visual-viewport/rubberbanding-viewport-rects-extended-background-expected.txt: Copied from LayoutTests/platform/ios-simulator-wk2/fast/visual-viewport/rubberbanding-viewport-rects-expected.txt.
* platform/mac-wk1/fast/visual-viewport/rubberbanding-viewport-rects-extended-background-expected.txt: Added.

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

14 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/visual-viewport/rubberbanding-viewport-rects-expected.txt
LayoutTests/fast/visual-viewport/rubberbanding-viewport-rects-extended-background-expected.txt [new file with mode: 0644]
LayoutTests/fast/visual-viewport/rubberbanding-viewport-rects-extended-background.html [new file with mode: 0644]
LayoutTests/fast/visual-viewport/rubberbanding-viewport-rects.html
LayoutTests/platform/ios-simulator-wk2/fast/visual-viewport/rubberbanding-viewport-rects-expected.txt
LayoutTests/platform/ios-simulator-wk2/fast/visual-viewport/rubberbanding-viewport-rects-extended-background-expected.txt [new file with mode: 0644]
LayoutTests/platform/mac-wk1/fast/visual-viewport/rubberbanding-viewport-rects-extended-background-expected.txt [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/FrameView.cpp
Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp
Source/WebCore/page/scrolling/ScrollingStateFrameScrollingNode.cpp
Source/WebCore/page/scrolling/ScrollingTreeFrameScrollingNode.cpp
Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm

index 4050e3e..f2947f1 100644 (file)
@@ -1,5 +1,21 @@
 2017-02-17  Simon Fraser  <simon.fraser@apple.com>
 
+        Fixed elements bounce when rubber-banding at the bottom of the page
+        https://bugs.webkit.org/show_bug.cgi?id=168493
+        rdar://problem/30567713
+
+        Reviewed by Tim Horton.
+
+        * fast/visual-viewport/rubberbanding-viewport-rects-expected.txt:
+        * fast/visual-viewport/rubberbanding-viewport-rects-extended-background-expected.txt: Copied from LayoutTests/fast/visual-viewport/rubberbanding-viewport-rects-expected.txt.
+        * fast/visual-viewport/rubberbanding-viewport-rects-extended-background.html: Copied from LayoutTests/fast/visual-viewport/rubberbanding-viewport-rects.html.
+        * fast/visual-viewport/rubberbanding-viewport-rects.html:
+        * platform/ios-simulator-wk2/fast/visual-viewport/rubberbanding-viewport-rects-expected.txt:
+        * platform/ios-simulator-wk2/fast/visual-viewport/rubberbanding-viewport-rects-extended-background-expected.txt: Copied from LayoutTests/platform/ios-simulator-wk2/fast/visual-viewport/rubberbanding-viewport-rects-expected.txt.
+        * platform/mac-wk1/fast/visual-viewport/rubberbanding-viewport-rects-extended-background-expected.txt: Added.
+
+2017-02-17  Simon Fraser  <simon.fraser@apple.com>
+
         REGRESSION (209396): Apple Pay buttons do not render
         https://bugs.webkit.org/show_bug.cgi?id=168523
         rdar://problem/30451563
index 066defe..e0eb106 100644 (file)
@@ -11,9 +11,9 @@ Scrolled to 475, 525
 JSON.stringify(layoutViewport) is {"top":525,"right":1260,"bottom":1110,"left":475,"width":785,"height":585}
 JSON.stringify(visualViewport) is {"top":525,"right":1260,"bottom":1110,"left":475,"width":785,"height":585}
 
-Scrolled to 1800, 1700
-JSON.stringify(layoutViewport) is {"top":1690,"right":2008,"bottom":2275,"left":1223,"width":785,"height":585}
-JSON.stringify(visualViewport) is {"top":1700,"right":2585,"bottom":2275,"left":1800,"width":785,"height":575}
+Scrolled to 1800, 1850
+JSON.stringify(layoutViewport) is {"top":1695,"right":2008,"bottom":2280,"left":1223,"width":785,"height":585}
+JSON.stringify(visualViewport) is {"top":1850,"right":2585,"bottom":2435,"left":1800,"width":785,"height":585}
 PASS successfullyParsed is true
 
 TEST COMPLETE
diff --git a/LayoutTests/fast/visual-viewport/rubberbanding-viewport-rects-extended-background-expected.txt b/LayoutTests/fast/visual-viewport/rubberbanding-viewport-rects-extended-background-expected.txt
new file mode 100644 (file)
index 0000000..fcdd2cb
--- /dev/null
@@ -0,0 +1,20 @@
+This test scrolls the page with extended backgrounds enabled and checks that the layout and visual viewports respond as expected.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+Scrolled to -123, -234
+JSON.stringify(layoutViewport) is {"top":-234,"right":662,"bottom":351,"left":-123,"width":785,"height":585}
+JSON.stringify(visualViewport) is {"top":-234,"right":662,"bottom":351,"left":-123,"width":785,"height":585}
+
+Scrolled to 475, 525
+JSON.stringify(layoutViewport) is {"top":525,"right":1260,"bottom":1110,"left":475,"width":785,"height":585}
+JSON.stringify(visualViewport) is {"top":525,"right":1260,"bottom":1110,"left":475,"width":785,"height":585}
+
+Scrolled to 1800, 1850
+JSON.stringify(layoutViewport) is {"top":1850,"right":2585,"bottom":2435,"left":1800,"width":785,"height":585}
+JSON.stringify(visualViewport) is {"top":1850,"right":2585,"bottom":2435,"left":1800,"width":785,"height":585}
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/visual-viewport/rubberbanding-viewport-rects-extended-background.html b/LayoutTests/fast/visual-viewport/rubberbanding-viewport-rects-extended-background.html
new file mode 100644 (file)
index 0000000..e20e941
--- /dev/null
@@ -0,0 +1,65 @@
+<!DOCTYPE html>
+
+<html>
+<head>
+    <script src="../../resources/js-test-pre.js"></script>
+    <style>
+        body {
+            height: 2000px;
+            width: 2000px;
+        }
+    </style>
+    <script>
+        description("This test scrolls the page with extended backgrounds enabled and checks that the layout and visual viewports respond as expected.");
+
+        if (window.internals) {
+            internals.settings.setVisualViewportEnabled(true);
+            internals.settings.setAllowUnclampedScrollPosition(true);
+            internals.settings.setBackgroundShouldExtendBeyondPage(true);
+        }
+
+        window.jsTestIsAsync = true;
+
+        var visualViewport;
+        function doTest()
+        {
+            // Zooming may scroll the view away from the origin.
+            window.scrollTo(-123, -234);
+            visualViewport = internals.visualViewportRect();
+            layoutViewport = internals.layoutViewportRect();
+            debug('Scrolled to ' + window.scrollX + ', ' + window.scrollY);
+
+            // Don't use shouldBeEqualToString() to avoid showing failures when correct output differs between platforms.
+            evalAndLogResult("JSON.stringify(layoutViewport)");
+            evalAndLogResult("JSON.stringify(visualViewport)");
+
+            debug('');
+            window.scrollTo(475, 525);
+            visualViewport = internals.visualViewportRect();
+            layoutViewport = internals.layoutViewportRect();
+            debug('Scrolled to ' + window.scrollX + ', ' + window.scrollY);
+
+            evalAndLogResult("JSON.stringify(layoutViewport)");
+            evalAndLogResult("JSON.stringify(visualViewport)");
+
+            debug('');
+            window.scrollTo(1800, 1850);
+            visualViewport = internals.visualViewportRect();
+            layoutViewport = internals.layoutViewportRect();
+            debug('Scrolled to ' + window.scrollX + ', ' + window.scrollY);
+
+            evalAndLogResult("JSON.stringify(layoutViewport)");
+            evalAndLogResult("JSON.stringify(visualViewport)");
+
+            window.scrollTo(0, 0);
+
+            finishJSTest();
+        }
+        
+        window.addEventListener('load', doTest, false);
+    </script>
+</head>
+<body>
+    <script src="../../resources/js-test-post.js"></script>
+</body>
+</html>
index c8bb3ce..b1c94cf 100644 (file)
@@ -28,7 +28,7 @@
             layoutViewport = internals.layoutViewportRect();
             debug('Scrolled to ' + window.scrollX + ', ' + window.scrollY);
 
-            // Don't use shouldBeEqualToString() to avoid showing failures when correct output differs between platforms. 
+            // Don't use shouldBeEqualToString() to avoid showing failures when correct output differs between platforms.
             evalAndLogResult("JSON.stringify(layoutViewport)");
             evalAndLogResult("JSON.stringify(visualViewport)");
 
@@ -42,7 +42,7 @@
             evalAndLogResult("JSON.stringify(visualViewport)");
 
             debug('');
-            window.scrollTo(1800, 1700);
+            window.scrollTo(1800, 1850);
             visualViewport = internals.visualViewportRect();
             layoutViewport = internals.layoutViewportRect();
             debug('Scrolled to ' + window.scrollX + ', ' + window.scrollY);
index 65589d0..0e8a229 100644 (file)
@@ -11,9 +11,9 @@ Scrolled to 475, 525
 JSON.stringify(layoutViewport) is {"top":525,"right":1275,"bottom":1125,"left":475,"width":800,"height":600}
 JSON.stringify(visualViewport) is {"top":525,"right":1275,"bottom":1125,"left":475,"width":800,"height":600}
 
-Scrolled to 1800, 1700
-JSON.stringify(layoutViewport) is {"top":1616,"right":2008,"bottom":2216,"left":1208,"width":800,"height":600}
-JSON.stringify(visualViewport) is {"top":1700,"right":2600,"bottom":2216,"left":1800,"width":800,"height":516}
+Scrolled to 1800, 1850
+JSON.stringify(layoutViewport) is {"top":1658,"right":2008,"bottom":2258,"left":1208,"width":800,"height":600}
+JSON.stringify(visualViewport) is {"top":1850,"right":2600,"bottom":2450,"left":1800,"width":800,"height":600}
 PASS successfullyParsed is true
 
 TEST COMPLETE
diff --git a/LayoutTests/platform/ios-simulator-wk2/fast/visual-viewport/rubberbanding-viewport-rects-extended-background-expected.txt b/LayoutTests/platform/ios-simulator-wk2/fast/visual-viewport/rubberbanding-viewport-rects-extended-background-expected.txt
new file mode 100644 (file)
index 0000000..d0c1a4f
--- /dev/null
@@ -0,0 +1,20 @@
+This test scrolls the page with extended backgrounds enabled and checks that the layout and visual viewports respond as expected.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+Scrolled to -123, -234
+JSON.stringify(layoutViewport) is {"top":-234,"right":677,"bottom":366,"left":-123,"width":800,"height":600}
+JSON.stringify(visualViewport) is {"top":-234,"right":677,"bottom":366,"left":-123,"width":800,"height":600}
+
+Scrolled to 475, 525
+JSON.stringify(layoutViewport) is {"top":525,"right":1275,"bottom":1125,"left":475,"width":800,"height":600}
+JSON.stringify(visualViewport) is {"top":525,"right":1275,"bottom":1125,"left":475,"width":800,"height":600}
+
+Scrolled to 1800, 1850
+JSON.stringify(layoutViewport) is {"top":1850,"right":2600,"bottom":2450,"left":1800,"width":800,"height":600}
+JSON.stringify(visualViewport) is {"top":1850,"right":2600,"bottom":2450,"left":1800,"width":800,"height":600}
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/platform/mac-wk1/fast/visual-viewport/rubberbanding-viewport-rects-extended-background-expected.txt b/LayoutTests/platform/mac-wk1/fast/visual-viewport/rubberbanding-viewport-rects-extended-background-expected.txt
new file mode 100644 (file)
index 0000000..b68d1da
--- /dev/null
@@ -0,0 +1,20 @@
+This test scrolls the page with extended backgrounds enabled and checks that the layout and visual viewports respond as expected.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+Scrolled to 0, 0
+JSON.stringify(layoutViewport) is {"top":0,"right":785,"bottom":585,"left":0,"width":785,"height":585}
+JSON.stringify(visualViewport) is {"top":0,"right":785,"bottom":585,"left":0,"width":785,"height":585}
+
+Scrolled to 475, 525
+JSON.stringify(layoutViewport) is {"top":525,"right":1260,"bottom":1110,"left":475,"width":785,"height":585}
+JSON.stringify(visualViewport) is {"top":525,"right":1260,"bottom":1110,"left":475,"width":785,"height":585}
+
+Scrolled to 1223, 1713
+JSON.stringify(layoutViewport) is {"top":1713,"right":2008,"bottom":2298,"left":1223,"width":785,"height":585}
+JSON.stringify(visualViewport) is {"top":1713,"right":2008,"bottom":2298,"left":1223,"width":785,"height":585}
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
index 155ae4b..b36ffbe 100644 (file)
@@ -1,5 +1,40 @@
 2017-02-17  Simon Fraser  <simon.fraser@apple.com>
 
+        Fixed elements bounce when rubber-banding at the bottom of the page
+        https://bugs.webkit.org/show_bug.cgi?id=168493
+        rdar://problem/30567713
+
+        Reviewed by Tim Horton.
+
+        FrameView::visibleDocumentRect() was computing a bad visible rect when bottom-rubber-banding,
+        by adding rubberBandBottom which is negative, rather than subtracting.
+
+        Log some more scrolling stuff.
+
+        Ironically, the existing test didn't test stick-to-viewport fixed position because
+        backgroundShouldExtendBeyondPage() is off by default in WTR, so clone it to a test
+        that sets this, to test both behaviors.
+
+        This also revealed that dynamic changes to backgroundShouldExtendBeyondPage() need
+        to be propagated to the scrolling tree, which is fixed in AsyncScrollingCoordinator::frameViewLayoutUpdated().
+
+        Test: fast/visual-viewport/rubberbanding-viewport-rects-extended-background.html
+
+        * page/FrameView.cpp:
+        (WebCore::FrameView::updateLayoutViewport):
+        (WebCore::FrameView::visibleDocumentRect):
+        * page/scrolling/AsyncScrollingCoordinator.cpp:
+        (WebCore::AsyncScrollingCoordinator::frameViewLayoutUpdated):
+        * page/scrolling/ScrollingStateFrameScrollingNode.cpp:
+        (WebCore::ScrollingStateFrameScrollingNode::dumpProperties):
+        * page/scrolling/ScrollingTreeFrameScrollingNode.cpp:
+        (WebCore::ScrollingTreeFrameScrollingNode::layoutViewportForScrollPosition):
+        * page/scrolling/mac/ScrollingCoordinatorMac.mm:
+        (WebCore::ScrollingCoordinatorMac::scheduleTreeStateCommit):
+        (WebCore::ScrollingCoordinatorMac::commitTreeState):
+
+2017-02-17  Simon Fraser  <simon.fraser@apple.com>
+
         REGRESSION (209396): Apple Pay buttons do not render
         https://bugs.webkit.org/show_bug.cgi?id=168523
         rdar://problem/30451563
index 34bd9fb..d150805 100644 (file)
@@ -1906,7 +1906,7 @@ void FrameView::updateLayoutViewport()
 
     LayoutRect layoutViewport = layoutViewportRect();
 
-    LOG_WITH_STREAM(Scrolling, stream << "\nFrameView " << this << " updateLayoutViewport() totalContentSize " << totalContentsSize() << " unscaledDocumentRect " << (renderView() ? renderView()->unscaledDocumentRect() : IntRect()) << " header height " << headerHeight() << " footer height " << footerHeight());
+    LOG_WITH_STREAM(Scrolling, stream << "\nFrameView " << this << " updateLayoutViewport() totalContentSize " << totalContentsSize() << " unscaledDocumentRect " << (renderView() ? renderView()->unscaledDocumentRect() : IntRect()) << " header height " << headerHeight() << " footer height " << footerHeight() << " fixed behavior " << scrollBehaviorForFixedElements());
     LOG_WITH_STREAM(Scrolling, stream << "layoutViewport: " << layoutViewport);
     LOG_WITH_STREAM(Scrolling, stream << "visualViewport: " << visualViewportRect());
     LOG_WITH_STREAM(Scrolling, stream << "scroll positions: min: " << unscaledMinimumScrollPosition() << " max: "<< unscaledMaximumScrollPosition());
@@ -1959,7 +1959,7 @@ LayoutRect FrameView::visibleDocumentRect(const FloatRect& visibleContentRect, f
     float visibleScaledDocumentTop = std::max<float>(visibleContentRect.y() - headerHeight, 0) + rubberBandTop;
     
     float rubberBandBottom = std::min<float>((totalContentsSize.height() - visibleContentRect.y()) - visibleContentRect.height(), 0);
-    float visibleScaledDocumentBottom = std::min<float>(visibleContentRect.maxY() - headerHeight, contentsHeight) + rubberBandBottom;
+    float visibleScaledDocumentBottom = std::min<float>(visibleContentRect.maxY() - headerHeight, contentsHeight) - rubberBandBottom;
 
     FloatRect visibleDocumentRect = visibleContentRect;
     visibleDocumentRect.setY(visibleScaledDocumentTop);
index d08f5a8..fce95a0 100644 (file)
@@ -157,6 +157,7 @@ void AsyncScrollingCoordinator::frameViewLayoutUpdated(FrameView& frameView)
     node->setTotalContentsSize(frameView.totalContentsSize());
     node->setReachableContentsSize(frameView.totalContentsSize());
     node->setFixedElementsLayoutRelativeToFrame(frameView.fixedElementsLayoutRelativeToFrame());
+    node->setScrollBehaviorForFixedElements(frameView.scrollBehaviorForFixedElements());
 
 #if ENABLE(CSS_SCROLL_SNAP)
     frameView.updateSnapOffsets();
index f518127..6313c13 100644 (file)
@@ -281,6 +281,11 @@ void ScrollingStateFrameScrollingNode::dumpProperties(TextStream& ts, int indent
         writeIndent(ts, indent + 1);
         ts << "(max layout viewport origin " << m_maxLayoutViewportOrigin << ")\n";
     }
+    
+    if (m_behaviorForFixed == StickToViewportBounds) {
+        writeIndent(ts, indent + 1);
+        ts << "(fixed behavior: stick to viewport)\n";
+    }
 
     if (!m_eventTrackingRegions.asynchronousDispatchRegion.isEmpty()) {
         ++indent;
index 1f48f15..17ac7a4 100644 (file)
@@ -106,7 +106,7 @@ FloatRect ScrollingTreeFrameScrollingNode::layoutViewportForScrollPosition(const
     LayoutRect visualViewport(FrameView::visibleDocumentRect(visibleContentRect, headerHeight(), footerHeight(), totalContentsSize(), scale));
     LayoutRect layoutViewport(m_layoutViewport);
 
-    LOG_WITH_STREAM(Scrolling, stream << "\nScrolling thread: " << "(visibleContentOrigin " << visibleContentOrigin << ")");
+    LOG_WITH_STREAM(Scrolling, stream << "\nScrolling thread: " << "(visibleContentOrigin " << visibleContentOrigin << ") fixed behavior " << m_behaviorForFixed);
     LOG_WITH_STREAM(Scrolling, stream << "  layoutViewport: " << layoutViewport);
     LOG_WITH_STREAM(Scrolling, stream << "  visualViewport: " << visualViewport);
     LOG_WITH_STREAM(Scrolling, stream << "  scroll positions: min: " << minLayoutViewportOrigin() << " max: "<< maxLayoutViewportOrigin());
index 86118b4..9d02116 100644 (file)
@@ -30,6 +30,7 @@
 #import "ScrollingCoordinatorMac.h"
 
 #include "FrameView.h"
+#include "Logging.h"
 #include "MainFrame.h"
 #include "Page.h"
 #include "PlatformWheelEvent.h"
@@ -103,15 +104,21 @@ void ScrollingCoordinatorMac::scheduleTreeStateCommit()
     if (m_scrollingStateTreeCommitterTimer.isActive())
         return;
 
+    LOG(Scrolling, "ScrollingCoordinatorMac::scheduleTreeStateCommit");
     m_scrollingStateTreeCommitterTimer.startOneShot(0);
 }
 
 void ScrollingCoordinatorMac::commitTreeState()
 {
     willCommitTree();
+
+    LOG(Scrolling, "ScrollingCoordinatorMac::commitTreeState, has changes %d", scrollingStateTree()->hasChangedProperties());
+
     if (!scrollingStateTree()->hasChangedProperties())
         return;
 
+    LOG(Scrolling, "%s", scrollingStateTreeAsText().utf8().data());
+
     RefPtr<ThreadedScrollingTree> threadedScrollingTree = downcast<ThreadedScrollingTree>(scrollingTree());
     ScrollingStateTree* unprotectedTreeState = scrollingStateTree()->commit(LayerRepresentation::PlatformLayerRepresentation).release();