constrainScrollPositionForOverhang needs to handle scrollOrigin correctly
authortimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 2 Jul 2013 18:14:47 +0000 (18:14 +0000)
committertimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 2 Jul 2013 18:14:47 +0000 (18:14 +0000)
https://bugs.webkit.org/show_bug.cgi?id=118176
<rdar://problem/14301271>

Reviewed by Anders Carlsson.

Test: compositing/geometry/fixed-position-flipped-writing-mode.html

WebCore makes use of constrainScrollPositionForOverhang not only for
constraining fixed- and sticky-positioned elements to the viewport, but
also for clamping the tile cache's visible rect.

Therefore, constrainScrollPositionForOverhang needs to correctly take
the scrollOrigin into account. The easiest way I saw to do this was to
reimplement the function in terms of a pair of rect intersections
between a virtual scrollable "viewport" and the document (with a bit
of complication from headers and footers).

The first intersection is performed, then if the viewport doesn't fit,
it is pushed down and to the right, from the origin. Next, we intersect
again, this time pushing the rect up by the amount it overflowed the document
rect from the bottom right. This performs effectively the same constraint
as previously, but handles the scrollOrigin correctly and is also somewhat
easier to read and understand (with pictures).

* page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm:
(WebCore::ScrollingTreeScrollingNodeMac::setScrollLayerPosition):
Subtract the scrollOrigin out of the offset passed to scrollOffsetForFixedPosition,
it's expecting an offset without the origin included.

* platform/ScrollableArea.cpp:
(WebCore::ScrollableArea::constrainScrollPositionForOverhang):
Reimplement the function as described above.

Add a test that ensures that fixed position works with a scrollable document
and direction: rtl, which is one case where scrollOrigin is set.

Disable the test on Mac WebKit1 because of https://bugs.webkit.org/show_bug.cgi?id=118269.

* compositing/geometry/fixed-position-flipped-writing-mode-expected.txt: Added.
* compositing/geometry/fixed-position-flipped-writing-mode.html: Added.
* platform/mac/compositing/geometry/fixed-position-flipped-writing-mode-expected.png: Added.
* platform/mac/TestExpectations:
* platform/mac-wk2/TestExpectations:

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

LayoutTests/ChangeLog
LayoutTests/compositing/geometry/fixed-position-flipped-writing-mode-expected.txt [new file with mode: 0644]
LayoutTests/compositing/geometry/fixed-position-flipped-writing-mode.html [new file with mode: 0644]
LayoutTests/platform/mac-wk2/TestExpectations
LayoutTests/platform/mac/TestExpectations
LayoutTests/platform/mac/compositing/geometry/fixed-position-flipped-writing-mode-expected.png [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm
Source/WebCore/platform/ScrollableArea.cpp

index 81ce4e7..7ac69c6 100644 (file)
@@ -1,3 +1,22 @@
+2013-07-02  Tim Horton  <timothy_horton@apple.com>
+
+        constrainScrollPositionForOverhang needs to handle scrollOrigin correctly
+        https://bugs.webkit.org/show_bug.cgi?id=118176
+        <rdar://problem/14301271>
+
+        Reviewed by Anders Carlsson.
+
+        Add a test that ensures that fixed position works with a scrollable document
+        and direction: rtl, which is one case where scrollOrigin is set.
+
+        Disable the test on Mac WebKit1 because of https://bugs.webkit.org/show_bug.cgi?id=118269.
+
+        * compositing/geometry/fixed-position-flipped-writing-mode-expected.txt: Added.
+        * compositing/geometry/fixed-position-flipped-writing-mode.html: Added.
+        * platform/mac/compositing/geometry/fixed-position-flipped-writing-mode-expected.png: Added.
+        * platform/mac/TestExpectations:
+        * platform/mac-wk2/TestExpectations:
+
 2013-07-02  Mario Sanchez Prada  <mario.prada@samsung.com>
 
         Unreviewed gardening. Added new common expectations for test after r151665.
 2013-07-02  Mario Sanchez Prada  <mario.prada@samsung.com>
 
         Unreviewed gardening. Added new common expectations for test after r151665.
diff --git a/LayoutTests/compositing/geometry/fixed-position-flipped-writing-mode-expected.txt b/LayoutTests/compositing/geometry/fixed-position-flipped-writing-mode-expected.txt
new file mode 100644 (file)
index 0000000..e7d3d68
--- /dev/null
@@ -0,0 +1,25 @@
+(GraphicsLayer
+  (position -4208.00 0.00)
+  (bounds 5008.00 585.00)
+  (children 1
+    (GraphicsLayer
+      (bounds 5008.00 585.00)
+      (contentsOpaque 1)
+      (children 2
+        (GraphicsLayer
+          (position 4308.00 0.00)
+          (bounds 200.00 585.00)
+          (contentsOpaque 1)
+        )
+        (GraphicsLayer
+          (position 0.00 13.00)
+          (bounds 5000.00 15.00)
+          (opacity 0.00)
+          (usingTiledLayer 1)
+          (drawsContent 1)
+        )
+      )
+    )
+  )
+)
+
diff --git a/LayoutTests/compositing/geometry/fixed-position-flipped-writing-mode.html b/LayoutTests/compositing/geometry/fixed-position-flipped-writing-mode.html
new file mode 100644 (file)
index 0000000..7ef795d
--- /dev/null
@@ -0,0 +1,57 @@
+<!DOCTYPE html>
+
+<html>
+<head>
+    <title>It should be possible to cover the red box with the green box by scrolling.</title>
+    <style>
+    html {
+        direction: rtl;
+    }
+
+    body {
+        width: 5000px;
+    }
+
+    .box {
+        width: 200px;
+        height: 100%;
+        top: 0px;
+    }
+
+    #background {
+        position: absolute;
+        background-color: red;
+        right: 500px;
+    }
+
+    #cover {
+        position: fixed;
+        background-color: green;
+        left: 50%;
+    }
+
+    #layers {
+        opacity: 0;
+    }
+    </style>
+    <script>
+    if (window.testRunner)
+        testRunner.dumpAsText(true);
+
+    function runTest()
+    {
+        window.scrollTo(-300, 0);
+        if (window.testRunner)
+            document.getElementById('layers').innerText = window.internals.layerTreeAsText(document);
+    }
+
+    window.addEventListener('load', runTest, false);
+    </script>
+</head>
+<body>
+    <div id="background" class="box"></div>
+    <div id="cover" class="box"></div>
+
+    <pre id="layers">Layer tree goes here.</pre>
+</body>
+</html>
index a5888a8..1a1402d 100644 (file)
@@ -398,6 +398,9 @@ webaudio/mediastreamaudiodestinationnode.html
 webaudio/mediastreamaudiosourcenode.html
 webaudio/codec-tests/vorbis/
 
 webaudio/mediastreamaudiosourcenode.html
 webaudio/codec-tests/vorbis/
 
+# Asserts in WebKit1-debug only, so reenabling.
+compositing/geometry/fixed-position-flipped-writing-mode.html [ Pass ]
+
 ### END OF (5) Features that are not supported in WebKit1, so skipped in mac/TestExpectations then re-enabled here
 ########################################
 
 ### END OF (5) Features that are not supported in WebKit1, so skipped in mac/TestExpectations then re-enabled here
 ########################################
 
index 37450f7..2c91090 100644 (file)
@@ -1336,3 +1336,6 @@ webkit.org/b/116388 fast/js/post-message-numeric-property.html
 webkit.org/b/116582 [ Debug ] inspector/styles/import-pseudoclass-crash.html [ Pass Crash ]
 
 webkit.org/b/116592 inspector/geolocation-error.html [ Pass Failure ]
 webkit.org/b/116582 [ Debug ] inspector/styles/import-pseudoclass-crash.html [ Pass Crash ]
 
 webkit.org/b/116592 inspector/geolocation-error.html [ Pass Failure ]
+
+# Asserts in WebKit1-debug due to a preexisting issue with overflow rect computation
+webkit.org/b/118269 compositing/geometry/fixed-position-flipped-writing-mode.html
diff --git a/LayoutTests/platform/mac/compositing/geometry/fixed-position-flipped-writing-mode-expected.png b/LayoutTests/platform/mac/compositing/geometry/fixed-position-flipped-writing-mode-expected.png
new file mode 100644 (file)
index 0000000..beaa17a
Binary files /dev/null and b/LayoutTests/platform/mac/compositing/geometry/fixed-position-flipped-writing-mode-expected.png differ
index 18543fd..64855d0 100644 (file)
@@ -1,3 +1,39 @@
+2013-07-02  Tim Horton  <timothy_horton@apple.com>
+
+        constrainScrollPositionForOverhang needs to handle scrollOrigin correctly
+        https://bugs.webkit.org/show_bug.cgi?id=118176
+        <rdar://problem/14301271>
+
+        Reviewed by Anders Carlsson.
+
+        Test: compositing/geometry/fixed-position-flipped-writing-mode.html
+
+        WebCore makes use of constrainScrollPositionForOverhang not only for
+        constraining fixed- and sticky-positioned elements to the viewport, but
+        also for clamping the tile cache's visible rect.
+
+        Therefore, constrainScrollPositionForOverhang needs to correctly take
+        the scrollOrigin into account. The easiest way I saw to do this was to
+        reimplement the function in terms of a pair of rect intersections
+        between a virtual scrollable "viewport" and the document (with a bit
+        of complication from headers and footers).
+
+        The first intersection is performed, then if the viewport doesn't fit,
+        it is pushed down and to the right, from the origin. Next, we intersect
+        again, this time pushing the rect up by the amount it overflowed the document
+        rect from the bottom right. This performs effectively the same constraint
+        as previously, but handles the scrollOrigin correctly and is also somewhat
+        easier to read and understand (with pictures).
+
+        * page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm:
+        (WebCore::ScrollingTreeScrollingNodeMac::setScrollLayerPosition):
+        Subtract the scrollOrigin out of the offset passed to scrollOffsetForFixedPosition,
+        it's expecting an offset without the origin included.
+
+        * platform/ScrollableArea.cpp:
+        (WebCore::ScrollableArea::constrainScrollPositionForOverhang):
+        Reimplement the function as described above.
+
 2013-07-02  Christophe Dumez  <ch.dumez@sisa.samsung.com>
 
         Remove SVGStyledLocatableElement class
 2013-07-02  Christophe Dumez  <ch.dumez@sisa.samsung.com>
 
         Remove SVGStyledLocatableElement class
index 4382464..98b60d7 100644 (file)
@@ -311,7 +311,8 @@ void ScrollingTreeScrollingNodeMac::setScrollLayerPosition(const IntPoint& posit
     ASSERT(!shouldUpdateScrollLayerPositionOnMainThread());
     m_scrollLayer.get().position = CGPointMake(-position.x() + scrollOrigin().x(), -position.y() + scrollOrigin().y());
 
     ASSERT(!shouldUpdateScrollLayerPositionOnMainThread());
     m_scrollLayer.get().position = CGPointMake(-position.x() + scrollOrigin().x(), -position.y() + scrollOrigin().y());
 
-    IntSize scrollOffsetForFixedChildren = FrameView::scrollOffsetForFixedPosition(viewportRect(), totalContentsSize(), position, scrollOrigin(), frameScaleFactor(), false, headerHeight(), footerHeight());
+    IntPoint scrollOffset = position - toIntSize(scrollOrigin());
+    IntSize scrollOffsetForFixedChildren = FrameView::scrollOffsetForFixedPosition(viewportRect(), totalContentsSize(), scrollOffset, scrollOrigin(), frameScaleFactor(), false, headerHeight(), footerHeight());
     if (m_counterScrollingLayer)
         m_counterScrollingLayer.get().position = FloatPoint(scrollOffsetForFixedChildren);
 
     if (m_counterScrollingLayer)
         m_counterScrollingLayer.get().position = FloatPoint(scrollOffsetForFixedChildren);
 
@@ -320,7 +321,7 @@ void ScrollingTreeScrollingNodeMac::setScrollLayerPosition(const IntPoint& posit
     // then we should recompute scrollOffsetForFixedChildren for the banner with a scale factor of 1.
     float horizontalScrollOffsetForBanner = scrollOffsetForFixedChildren.width();
     if (frameScaleFactor() != 1)
     // then we should recompute scrollOffsetForFixedChildren for the banner with a scale factor of 1.
     float horizontalScrollOffsetForBanner = scrollOffsetForFixedChildren.width();
     if (frameScaleFactor() != 1)
-        horizontalScrollOffsetForBanner = FrameView::scrollOffsetForFixedPosition(viewportRect(), totalContentsSize(), position, scrollOrigin(), 1, false, headerHeight(), footerHeight()).width();
+        horizontalScrollOffsetForBanner = FrameView::scrollOffsetForFixedPosition(viewportRect(), totalContentsSize(), scrollOffset, scrollOrigin(), 1, false, headerHeight(), footerHeight()).width();
 
     if (m_headerLayer)
         m_headerLayer.get().position = FloatPoint(horizontalScrollOffsetForBanner, 0);
 
     if (m_headerLayer)
         m_headerLayer.get().position = FloatPoint(horizontalScrollOffsetForBanner, 0);
index 6d85cc5..25084ff 100644 (file)
@@ -412,36 +412,30 @@ IntRect ScrollableArea::visibleContentRect(VisibleContentRectIncludesScrollbars
                    std::max(0, visibleHeight() + horizontalScrollbarHeight));
 }
 
                    std::max(0, visibleHeight() + horizontalScrollbarHeight));
 }
 
-static int constrainedScrollPosition(int visibleContentSize, int totalContentsSize, int scrollPosition, int scrollOrigin, int headerHeight, int footerHeight)
-{
-    int maxValue = totalContentsSize - visibleContentSize - footerHeight;
-    if (maxValue <= 0)
-        return 0;
-
-    if (!scrollOrigin) {
-        if (scrollPosition <= headerHeight)
-            return 0;
-        if (scrollPosition > maxValue)
-            scrollPosition = maxValue - headerHeight;
-        else
-            scrollPosition -= headerHeight;
-    } else {
-        // FIXME: position:fixed elements are currently broken when there is a non-zero y-value in the scroll origin
-        // such as when -webkit-writing-mode:horizontal-bt; is set. But when we fix that, we need to make such
-        // pages work correctly with headers and footers as well. https://bugs.webkit.org/show_bug.cgi?id=113741
-        if (scrollPosition >= 0)
-            return 0;
-        if (scrollPosition < -maxValue)
-            scrollPosition = -maxValue;
-    }
-
-    return scrollPosition;
-}
-
 IntPoint ScrollableArea::constrainScrollPositionForOverhang(const IntRect& visibleContentRect, const IntSize& totalContentsSize, const IntPoint& scrollPosition, const IntPoint& scrollOrigin, int headerHeight, int footerHeight)
 {
 IntPoint ScrollableArea::constrainScrollPositionForOverhang(const IntRect& visibleContentRect, const IntSize& totalContentsSize, const IntPoint& scrollPosition, const IntPoint& scrollOrigin, int headerHeight, int footerHeight)
 {
-    return IntPoint(constrainedScrollPosition(visibleContentRect.width(), totalContentsSize.width(), scrollPosition.x(), scrollOrigin.x(), 0, 0),
-        constrainedScrollPosition(visibleContentRect.height(), totalContentsSize.height(), scrollPosition.y(), scrollOrigin.y(), headerHeight, footerHeight));
+    // The viewport rect that we're scrolling shouldn't be larger than our document.
+    IntSize idealScrollRectSize(std::min(visibleContentRect.width(), totalContentsSize.width()), std::min(visibleContentRect.height(), totalContentsSize.height()));
+    
+    IntRect scrollRect(scrollPosition + scrollOrigin - IntSize(0, headerHeight), idealScrollRectSize);
+    IntRect documentRect(IntPoint(), IntSize(totalContentsSize.width(), totalContentsSize.height() - headerHeight - footerHeight));
+
+    // Use intersection to constrain our ideal scroll rect by the document rect.
+    scrollRect.intersect(documentRect);
+
+    if (scrollRect.size() != idealScrollRectSize) {
+        // If the rect was clipped, restore its size, effectively pushing it "down" from the top left.
+        scrollRect.setSize(idealScrollRectSize);
+
+        // If we still clip, push our rect "up" from the bottom right.
+        scrollRect.intersect(documentRect);
+        if (scrollRect.width() < idealScrollRectSize.width())
+            scrollRect.move(-(idealScrollRectSize.width() - scrollRect.width()), 0);
+        if (scrollRect.height() < idealScrollRectSize.height())
+            scrollRect.move(0, -(idealScrollRectSize.height() - scrollRect.height()));
+    }
+
+    return scrollRect.location() - toIntSize(scrollOrigin);
 }
 
 IntPoint ScrollableArea::constrainScrollPositionForOverhang(const IntPoint& scrollPosition)
 }
 
 IntPoint ScrollableArea::constrainScrollPositionForOverhang(const IntPoint& scrollPosition)