Fix reload and programmatic scrolling in RTL documents
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 2 Jan 2016 04:45:36 +0000 (04:45 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 2 Jan 2016 04:45:36 +0000 (04:45 +0000)
https://bugs.webkit.org/show_bug.cgi?id=152639

Reviewed by Zalan Bujtas.
Source/WebCore:

Reloading a left-scrolled RTL document would cause the content to appear
at an odd offset, and programmatic sideways scrolls in RTL documents also
jumped to the wrong location.

Fix by resolving offset/position confusion in ScrollableArea::scrollPositionChanged()
and the scrolling tree.

ScrollableArea::scrollPositionChanged() was erroneously passing a scrollPosition
to setScrollOffset().

ScrollingTreeFrameScrollingNode* were confused about offsets and positions. It
turns out that the layer position is just -scrollPosition, but minimumScrollPosition()
and maximumScrollPosition() need fixing to return positions, not offsets.

ScrollingTreeFrameScrollingNode::viewToContentsOffset() was also doing incorrect
math with scrollOrigin, which was detected by a failing test.

Add more logging to the Scrolling channel.

Tests: fast/scrolling/programmatic-document-rtl-scroll.html
       fast/scrolling/programmatic-document-rtl-scrollIntoView.html
       fast/scrolling/scroll-position-on-reload-rtl.html

* page/FrameView.cpp:
(WebCore::FrameView::requestScrollPositionUpdate):
* page/scrolling/ScrollingTree.cpp:
(WebCore::ScrollingTree::shouldHandleWheelEventSynchronously):
* page/scrolling/ScrollingTreeFrameScrollingNode.cpp:
(WebCore::ScrollingTreeFrameScrollingNode::scrollBy):
(WebCore::ScrollingTreeFrameScrollingNode::viewToContentsOffset):
* page/scrolling/ScrollingTreeFrameScrollingNode.h:
* page/scrolling/ios/ScrollingTreeFrameScrollingNodeIOS.mm:
(WebCore::ScrollingTreeFrameScrollingNodeIOS::scrollPosition):
(WebCore::ScrollingTreeFrameScrollingNodeIOS::setScrollLayerPosition):
(WebCore::ScrollingTreeFrameScrollingNodeIOS::updateChildNodesAfterScroll):
(WebCore::ScrollingTreeFrameScrollingNodeIOS::minimumScrollPosition):
(WebCore::ScrollingTreeFrameScrollingNodeIOS::maximumScrollPosition):
* page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:
(WebCore::ScrollingTreeFrameScrollingNodeMac::immediateScrollBy):
(WebCore::ScrollingTreeFrameScrollingNodeMac::scrollPosition):
(WebCore::ScrollingTreeFrameScrollingNodeMac::setScrollPosition):
(WebCore::ScrollingTreeFrameScrollingNodeMac::setScrollLayerPosition):
(WebCore::ScrollingTreeFrameScrollingNodeMac::minimumScrollPosition):
(WebCore::ScrollingTreeFrameScrollingNodeMac::maximumScrollPosition):
* platform/ScrollView.cpp:
(WebCore::ScrollView::setScrollOffset):
(WebCore::ScrollView::scrollTo):
(WebCore::ScrollView::setScrollPosition):
(WebCore::ScrollView::updateScrollbars):
* platform/ScrollView.h:
* platform/ScrollableArea.cpp:
(WebCore::ScrollableArea::scrollPositionChanged):
(WebCore::ScrollableArea::setScrollOffsetFromAnimation):
(WebCore::ScrollableArea::scrollPositionFromOffset):
(WebCore::ScrollableArea::scrollOffsetFromPosition):
* platform/ScrollableArea.h:
(WebCore::ScrollableArea::scrollPositionFromOffset):
(WebCore::ScrollableArea::scrollOffsetFromPosition):
* rendering/RenderListBox.cpp:
(WebCore::RenderListBox::setScrollOffset):
* rendering/RenderListBox.h:

Source/WebKit2:

Reloading a left-scrolled RTL document would cause the content to appear
at an odd offset, and programmatic sideways scrolls in RTL documents also
jumped to the wrong location.

Fix by resolving offset/position confusion in ScrollableArea::scrollPositionChanged()
and the scrolling tree.

ScrollableArea::scrollPositionChanged() was erroneously passing a scrollPosition
to setScrollOffset().

ScrollingTreeFrameScrollingNode* were confused about offsets and positions. It
turns out that the layer position is just -scrollPosition, but minimumScrollPosition()
and maximumScrollPosition() need fixing to return positions, not offsets.

ScrollingTreeFrameScrollingNode::viewToContentsOffset() was also doing incorrect
math with scrollOrigin, which was detected by a failing test.

Add more logging to the Scrolling channel.

* WebProcess/Plugins/PDF/DeprecatedPDFPlugin.h:
* WebProcess/Plugins/PDF/DeprecatedPDFPlugin.mm:
(WebKit::PDFPlugin::setScrollOffset):

LayoutTests:

New tests for programmatic scrolling (2 kinds!) in RTL documents, and
reloading a scrolled RTL document.

* fast/scrolling/programmatic-document-rtl-scroll-expected.html: Added.
* fast/scrolling/programmatic-document-rtl-scroll.html: Added.
* fast/scrolling/programmatic-document-rtl-scrollIntoView-expected.txt: Added.
* fast/scrolling/programmatic-document-rtl-scrollIntoView.html: Added.
* fast/scrolling/scroll-position-on-reload-rtl-expected.txt: Added.
* fast/scrolling/scroll-position-on-reload-rtl.html: Added.

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

23 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/scrolling/programmatic-document-rtl-scroll-expected.html [new file with mode: 0644]
LayoutTests/fast/scrolling/programmatic-document-rtl-scroll.html [new file with mode: 0644]
LayoutTests/fast/scrolling/programmatic-document-rtl-scrollIntoView-expected.txt [new file with mode: 0644]
LayoutTests/fast/scrolling/programmatic-document-rtl-scrollIntoView.html [new file with mode: 0644]
LayoutTests/fast/scrolling/scroll-position-on-reload-rtl-expected.txt [new file with mode: 0644]
LayoutTests/fast/scrolling/scroll-position-on-reload-rtl.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/FrameView.cpp
Source/WebCore/page/scrolling/ScrollingTree.cpp
Source/WebCore/page/scrolling/ScrollingTreeFrameScrollingNode.cpp
Source/WebCore/page/scrolling/ScrollingTreeFrameScrollingNode.h
Source/WebCore/page/scrolling/ios/ScrollingTreeFrameScrollingNodeIOS.mm
Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm
Source/WebCore/platform/ScrollView.cpp
Source/WebCore/platform/ScrollView.h
Source/WebCore/platform/ScrollableArea.cpp
Source/WebCore/platform/ScrollableArea.h
Source/WebCore/rendering/RenderListBox.cpp
Source/WebCore/rendering/RenderListBox.h
Source/WebKit2/ChangeLog
Source/WebKit2/WebProcess/Plugins/PDF/DeprecatedPDFPlugin.h
Source/WebKit2/WebProcess/Plugins/PDF/DeprecatedPDFPlugin.mm

index 018bba1..e472399 100644 (file)
@@ -1,5 +1,22 @@
 2016-01-01  Simon Fraser  <simon.fraser@apple.com>
 
+        Fix reload and programmatic scrolling in RTL documents
+        https://bugs.webkit.org/show_bug.cgi?id=152639
+
+        Reviewed by Zalan Bujtas.
+        
+        New tests for programmatic scrolling (2 kinds!) in RTL documents, and
+        reloading a scrolled RTL document.
+
+        * fast/scrolling/programmatic-document-rtl-scroll-expected.html: Added.
+        * fast/scrolling/programmatic-document-rtl-scroll.html: Added.
+        * fast/scrolling/programmatic-document-rtl-scrollIntoView-expected.txt: Added.
+        * fast/scrolling/programmatic-document-rtl-scrollIntoView.html: Added.
+        * fast/scrolling/scroll-position-on-reload-rtl-expected.txt: Added.
+        * fast/scrolling/scroll-position-on-reload-rtl.html: Added.
+
+2016-01-01  Simon Fraser  <simon.fraser@apple.com>
+
         REGRESSION (r194448): Scrolling overflow:scroll goes too far
         https://bugs.webkit.org/show_bug.cgi?id=152645
 
diff --git a/LayoutTests/fast/scrolling/programmatic-document-rtl-scroll-expected.html b/LayoutTests/fast/scrolling/programmatic-document-rtl-scroll-expected.html
new file mode 100644 (file)
index 0000000..c12258f
--- /dev/null
@@ -0,0 +1,45 @@
+<html>
+<head>
+    <style>
+        body {
+            margin: 0;
+        }
+        .wide {
+            width: 2000px;
+            height: 10px;
+            background-color: silver;
+        }
+        .origin {
+            position: absolute;
+            top: 0;
+            left: 1200px;
+            width: 10px;
+            height: 10px;
+            background-color: blue;
+        }
+        
+        #lefty {
+            position: absolute;
+            left: 1000px;
+            width: 100px;
+            height: 100px;
+            background-color: orange;
+        }
+    </style>
+    <script>
+    function runTest()
+    {
+        document.scrollingElement.scrollLeft = 1000;
+    }
+    
+    window.addEventListener('load', runTest, false);
+    </script>
+</head>
+<body>
+    <div class="wide"></div>
+    <div class="origin"></div>
+    <div id="lefty"></div>
+    <div id="result">
+    </div>
+</body>
+</html>
diff --git a/LayoutTests/fast/scrolling/programmatic-document-rtl-scroll.html b/LayoutTests/fast/scrolling/programmatic-document-rtl-scroll.html
new file mode 100644 (file)
index 0000000..329b2ca
--- /dev/null
@@ -0,0 +1,45 @@
+<html dir="rtl">
+<head>
+    <style>
+        body {
+            margin: 0;
+        }
+        .wide {
+            width: 2000px;
+            height: 10px;
+            background-color: silver;
+        }
+        .origin {
+            position: absolute;
+            top: 0;
+            left: 0;
+            width: 10px;
+            height: 10px;
+            background-color: blue;
+        }
+        
+        #lefty {
+            position: absolute;
+            left: -200px;
+            width: 100px;
+            height: 100px;
+            background-color: orange;
+        }
+    </style>
+    <script>
+    function runTest()
+    {
+        document.scrollingElement.scrollLeft = -200;
+    }
+    
+    window.addEventListener('load', runTest, false);
+    </script>
+</head>
+<body>
+    <div class="wide"></div>
+    <div class="origin"></div>
+    <div id="lefty"></div>
+    <div id="result">
+    </div>
+</body>
+</html>
diff --git a/LayoutTests/fast/scrolling/programmatic-document-rtl-scrollIntoView-expected.txt b/LayoutTests/fast/scrolling/programmatic-document-rtl-scrollIntoView-expected.txt
new file mode 100644 (file)
index 0000000..fa8f904
--- /dev/null
@@ -0,0 +1 @@
+document.scrollingElement.scrollLeft = -200
diff --git a/LayoutTests/fast/scrolling/programmatic-document-rtl-scrollIntoView.html b/LayoutTests/fast/scrolling/programmatic-document-rtl-scrollIntoView.html
new file mode 100644 (file)
index 0000000..0a00cd3
--- /dev/null
@@ -0,0 +1,46 @@
+<html dir="rtl">
+<head>
+    <style>
+        .wide {
+            width: 2000px;
+            height: 10px;
+            background-color: silver;
+        }
+        .origin {
+            position: absolute;
+            top: 0;
+            left: 0;
+            width: 10px;
+            height: 10px;
+            background-color: blue;
+        }
+        
+        #lefty {
+            position: absolute;
+            left: -200px;
+            width: 100px;
+            height: 100px;
+            background-color: orange;
+        }
+    </style>
+    <script>
+    if (window.testRunner)
+        testRunner.dumpAsText();
+
+    function runTest()
+    {
+        document.getElementById('lefty').scrollIntoView();
+        document.getElementById('result').textContent = 'document.scrollingElement.scrollLeft = ' + document.scrollingElement.scrollLeft;
+    }
+    
+    window.addEventListener('load', runTest, false);
+    </script>
+</head>
+<body>
+    <div class="wide"></div>
+    <div class="origin"></div>
+    <div id="lefty"></div>
+    <div id="result">
+    </div>
+</body>
+</html>
diff --git a/LayoutTests/fast/scrolling/scroll-position-on-reload-rtl-expected.txt b/LayoutTests/fast/scrolling/scroll-position-on-reload-rtl-expected.txt
new file mode 100644 (file)
index 0000000..2ac7ae9
--- /dev/null
@@ -0,0 +1 @@
+PASS: scrollLeft after reload is -200
diff --git a/LayoutTests/fast/scrolling/scroll-position-on-reload-rtl.html b/LayoutTests/fast/scrolling/scroll-position-on-reload-rtl.html
new file mode 100644 (file)
index 0000000..610f852
--- /dev/null
@@ -0,0 +1,66 @@
+<html dir="rtl">
+<head>
+    <style>
+        .wide {
+            width: 2000px;
+            height: 10px;
+            background-color: silver;
+        }
+        .origin {
+            position: absolute;
+            top: 0;
+            left: 0;
+            width: 10px;
+            height: 10px;
+            background-color: blue;
+        }
+        
+        #lefty {
+            position: absolute;
+            left: -200px;
+            width: 100px;
+            height: 100px;
+            background-color: orange;
+        }
+    </style>
+    <script>
+    var desiredScrollOffset = -200;
+    function runTest()
+    {
+        if (!window.sessionStorage)
+            return;
+
+        if (window.testRunner) {
+            testRunner.waitUntilDone();
+            testRunner.dumpAsText();
+        }
+
+        if (sessionStorage.testCompleted) {
+            window.setTimeout(function() {
+                var currentOffset = document.scrollingElement.scrollLeft;
+                if (currentOffset == desiredScrollOffset)
+                    document.getElementById('result').textContent = 'PASS: scrollLeft after reload is ' + desiredScrollOffset;
+                else
+                    document.getElementById('result').textContent = 'FAIL: scrollLeft after reload is ' + currentOffset + ', should be' + desiredScrollOffset;
+
+                delete sessionStorage.testCompleted;
+                if (window.testRunner)
+                    testRunner.notifyDone();
+            }, 0);
+        } else {
+            document.scrollingElement.scrollLeft = desiredScrollOffset;
+            sessionStorage.testCompleted = true;
+            document.location.reload(true);
+        }
+    }
+
+    window.addEventListener('load', runTest, false);
+    </script>
+</head>
+<body>
+    <div class="wide"></div>
+    <div class="origin"></div>
+    <div id="lefty"></div>
+    <div id="result"></div>
+</body>
+</html>
index 9c45663..857a90a 100644 (file)
@@ -1,5 +1,74 @@
 2016-01-01  Simon Fraser  <simon.fraser@apple.com>
 
+        Fix reload and programmatic scrolling in RTL documents
+        https://bugs.webkit.org/show_bug.cgi?id=152639
+
+        Reviewed by Zalan Bujtas.
+        
+        Reloading a left-scrolled RTL document would cause the content to appear
+        at an odd offset, and programmatic sideways scrolls in RTL documents also
+        jumped to the wrong location.
+        
+        Fix by resolving offset/position confusion in ScrollableArea::scrollPositionChanged()
+        and the scrolling tree.
+        
+        ScrollableArea::scrollPositionChanged() was erroneously passing a scrollPosition
+        to setScrollOffset(). 
+        
+        ScrollingTreeFrameScrollingNode* were confused about offsets and positions. It
+        turns out that the layer position is just -scrollPosition, but minimumScrollPosition()
+        and maximumScrollPosition() need fixing to return positions, not offsets.
+        
+        ScrollingTreeFrameScrollingNode::viewToContentsOffset() was also doing incorrect
+        math with scrollOrigin, which was detected by a failing test.
+        
+        Add more logging to the Scrolling channel.
+
+        Tests: fast/scrolling/programmatic-document-rtl-scroll.html
+               fast/scrolling/programmatic-document-rtl-scrollIntoView.html
+               fast/scrolling/scroll-position-on-reload-rtl.html
+
+        * page/FrameView.cpp:
+        (WebCore::FrameView::requestScrollPositionUpdate):
+        * page/scrolling/ScrollingTree.cpp:
+        (WebCore::ScrollingTree::shouldHandleWheelEventSynchronously):
+        * page/scrolling/ScrollingTreeFrameScrollingNode.cpp:
+        (WebCore::ScrollingTreeFrameScrollingNode::scrollBy):
+        (WebCore::ScrollingTreeFrameScrollingNode::viewToContentsOffset):
+        * page/scrolling/ScrollingTreeFrameScrollingNode.h:
+        * page/scrolling/ios/ScrollingTreeFrameScrollingNodeIOS.mm:
+        (WebCore::ScrollingTreeFrameScrollingNodeIOS::scrollPosition):
+        (WebCore::ScrollingTreeFrameScrollingNodeIOS::setScrollLayerPosition):
+        (WebCore::ScrollingTreeFrameScrollingNodeIOS::updateChildNodesAfterScroll):
+        (WebCore::ScrollingTreeFrameScrollingNodeIOS::minimumScrollPosition):
+        (WebCore::ScrollingTreeFrameScrollingNodeIOS::maximumScrollPosition):
+        * page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:
+        (WebCore::ScrollingTreeFrameScrollingNodeMac::immediateScrollBy):
+        (WebCore::ScrollingTreeFrameScrollingNodeMac::scrollPosition):
+        (WebCore::ScrollingTreeFrameScrollingNodeMac::setScrollPosition):
+        (WebCore::ScrollingTreeFrameScrollingNodeMac::setScrollLayerPosition):
+        (WebCore::ScrollingTreeFrameScrollingNodeMac::minimumScrollPosition):
+        (WebCore::ScrollingTreeFrameScrollingNodeMac::maximumScrollPosition):
+        * platform/ScrollView.cpp:
+        (WebCore::ScrollView::setScrollOffset):
+        (WebCore::ScrollView::scrollTo):
+        (WebCore::ScrollView::setScrollPosition):
+        (WebCore::ScrollView::updateScrollbars):
+        * platform/ScrollView.h:
+        * platform/ScrollableArea.cpp:
+        (WebCore::ScrollableArea::scrollPositionChanged):
+        (WebCore::ScrollableArea::setScrollOffsetFromAnimation):
+        (WebCore::ScrollableArea::scrollPositionFromOffset):
+        (WebCore::ScrollableArea::scrollOffsetFromPosition):
+        * platform/ScrollableArea.h:
+        (WebCore::ScrollableArea::scrollPositionFromOffset):
+        (WebCore::ScrollableArea::scrollOffsetFromPosition):
+        * rendering/RenderListBox.cpp:
+        (WebCore::RenderListBox::setScrollOffset):
+        * rendering/RenderListBox.h:
+
+2016-01-01  Simon Fraser  <simon.fraser@apple.com>
+
         REGRESSION (r194448): Scrolling overflow:scroll goes too far
         https://bugs.webkit.org/show_bug.cgi?id=152645
 
index a5f4b82..7b7c311 100644 (file)
@@ -2353,6 +2353,8 @@ bool FrameView::isRubberBandInProgress() const
 
 bool FrameView::requestScrollPositionUpdate(const ScrollPosition& position)
 {
+    LOG_WITH_STREAM(Scrolling, stream << "FrameView::requestScrollPositionUpdate " << position);
+
 #if ENABLE(ASYNC_SCROLLING)
     if (TiledBacking* tiledBacking = this->tiledBacking())
         tiledBacking->prepopulateRect(FloatRect(position, visibleContentRect().size()));
index 0f41fea..476b4c2 100644 (file)
 
 #if ENABLE(ASYNC_SCROLLING)
 
+#include "Logging.h"
 #include "PlatformWheelEvent.h"
 #include "ScrollingStateTree.h"
 #include "ScrollingTreeFrameScrollingNode.h"
 #include "ScrollingTreeNode.h"
 #include "ScrollingTreeOverflowScrollingNode.h"
 #include "ScrollingTreeScrollingNode.h"
+#include "TextStream.h"
 #include <wtf/TemporaryChange.h>
 
 namespace WebCore {
@@ -61,9 +63,11 @@ bool ScrollingTree::shouldHandleWheelEventSynchronously(const PlatformWheelEvent
     
     if (!m_nonFastScrollableRegion.isEmpty() && m_rootNode) {
         ScrollingTreeFrameScrollingNode& frameScrollingNode = downcast<ScrollingTreeFrameScrollingNode>(*m_rootNode);
-        // FIXME: This is not correct for non-default scroll origins.
         FloatPoint position = wheelEvent.position();
         position.move(frameScrollingNode.viewToContentsOffset(m_mainFrameScrollPosition));
+
+        LOG_WITH_STREAM(Scrolling, stream << "ScrollingTree::shouldHandleWheelEventSynchronously: wheelEvent at " << wheelEvent.position() << " mapped to content point " << position << ", in non-fast region " << m_nonFastScrollableRegion.contains(roundedIntPoint(position)));
+
         if (m_nonFastScrollableRegion.contains(roundedIntPoint(position)))
             return true;
     }
index 8ad334a..ac3dba0 100644 (file)
@@ -70,9 +70,9 @@ void ScrollingTreeFrameScrollingNode::updateBeforeChildren(const ScrollingStateN
         m_fixedElementsLayoutRelativeToFrame = state.fixedElementsLayoutRelativeToFrame();
 }
 
-void ScrollingTreeFrameScrollingNode::scrollBy(const FloatSize& offset)
+void ScrollingTreeFrameScrollingNode::scrollBy(const FloatSize& delta)
 {
-    setScrollPosition(scrollPosition() + offset);
+    setScrollPosition(scrollPosition() + delta);
 }
 
 void ScrollingTreeFrameScrollingNode::scrollByWithoutContentEdgeConstraints(const FloatSize& offset)
@@ -86,9 +86,9 @@ void ScrollingTreeFrameScrollingNode::setScrollPosition(const FloatPoint& scroll
     setScrollPositionWithoutContentEdgeConstraints(newScrollPosition);
 }
 
-FloatSize ScrollingTreeFrameScrollingNode::viewToContentsOffset(const FloatPoint& scrollOffset) const
+FloatSize ScrollingTreeFrameScrollingNode::viewToContentsOffset(const FloatPoint& scrollPosition) const
 {
-    return toFloatSize(scrollOffset) - toFloatSize(scrollOrigin()) - FloatSize(0, headerHeight() + topContentInset());
+    return toFloatSize(scrollPosition) - FloatSize(0, headerHeight() + topContentInset());
 }
 
 } // namespace WebCore
index ac44905..92b01f0 100644 (file)
@@ -56,7 +56,7 @@ public:
     bool shouldUpdateScrollLayerPositionSynchronously() const { return m_synchronousScrollingReasons; }
     bool fixedElementsLayoutRelativeToFrame() const { return m_fixedElementsLayoutRelativeToFrame; }
 
-    FloatSize viewToContentsOffset(const FloatPoint& scrollOffset) const;
+    FloatSize viewToContentsOffset(const FloatPoint& scrollPosition) const;
 
 protected:
     ScrollingTreeFrameScrollingNode(ScrollingTree&, ScrollingNodeID);
index 32e5da4..80c2257 100644 (file)
@@ -102,8 +102,7 @@ FloatPoint ScrollingTreeFrameScrollingNodeIOS::scrollPosition() const
     if (shouldUpdateScrollLayerPositionSynchronously())
         return m_probableMainThreadScrollPosition;
 
-    CGPoint scrollLayerPosition = m_scrollLayer.get().position;
-    return IntPoint(-scrollLayerPosition.x + scrollOrigin().x(), -scrollLayerPosition.y + scrollOrigin().y());
+    return -m_scrollLayer.get().position;
 }
 
 void ScrollingTreeFrameScrollingNodeIOS::setScrollPositionWithoutContentEdgeConstraints(const FloatPoint& scrollPosition)
@@ -121,7 +120,7 @@ void ScrollingTreeFrameScrollingNodeIOS::setScrollPositionWithoutContentEdgeCons
 void ScrollingTreeFrameScrollingNodeIOS::setScrollLayerPosition(const FloatPoint& scrollPosition)
 {
     ASSERT(!shouldUpdateScrollLayerPositionSynchronously());
-    [m_scrollLayer setPosition:CGPointMake(-scrollPosition.x() + scrollOrigin().x(), -scrollPosition.y() + scrollOrigin().y())];
+    [m_scrollLayer setPosition:-scrollPosition];
 
     updateChildNodesAfterScroll(scrollPosition);
 }
@@ -146,9 +145,8 @@ void ScrollingTreeFrameScrollingNodeIOS::updateLayersAfterDelegatedScroll(const
 void ScrollingTreeFrameScrollingNodeIOS::updateChildNodesAfterScroll(const FloatPoint& scrollPosition)
 {
     ScrollBehaviorForFixedElements behaviorForFixed = scrollBehaviorForFixedElements();
-    FloatPoint scrollOffset = scrollPosition - toIntSize(scrollOrigin());
-    FloatRect viewportRect(FloatPoint(), scrollableAreaSize());
-    FloatPoint scrollPositionForFixedChildren = FrameView::scrollPositionForFixedPosition(enclosingLayoutRect(viewportRect), LayoutSize(totalContentsSize()), LayoutPoint(scrollOffset), scrollOrigin(), frameScaleFactor(), fixedElementsLayoutRelativeToFrame(), behaviorForFixed, headerHeight(), footerHeight());
+    FloatRect viewportRect(scrollPosition, scrollableAreaSize());
+    FloatPoint scrollPositionForFixedChildren = FrameView::scrollPositionForFixedPosition(enclosingLayoutRect(viewportRect), LayoutSize(totalContentsSize()), LayoutPoint(scrollPosition), scrollOrigin(), frameScaleFactor(), fixedElementsLayoutRelativeToFrame(), behaviorForFixed, headerHeight(), footerHeight());
 
     [m_counterScrollingLayer setPosition:scrollPositionForFixedChildren];
 
@@ -158,7 +156,7 @@ void ScrollingTreeFrameScrollingNodeIOS::updateChildNodesAfterScroll(const Float
         // then we should recompute scrollPositionForFixedChildren for the banner with a scale factor of 1.
         float horizontalScrollOffsetForBanner = scrollPositionForFixedChildren.x();
         if (frameScaleFactor() != 1)
-            horizontalScrollOffsetForBanner = FrameView::scrollPositionForFixedPosition(enclosingLayoutRect(viewportRect), LayoutSize(totalContentsSize()), LayoutPoint(scrollOffset), scrollOrigin(), 1, fixedElementsLayoutRelativeToFrame(), behaviorForFixed, headerHeight(), footerHeight()).x();
+            horizontalScrollOffsetForBanner = FrameView::scrollPositionForFixedPosition(enclosingLayoutRect(viewportRect), LayoutSize(totalContentsSize()), LayoutPoint(scrollPosition), scrollOrigin(), 1, fixedElementsLayoutRelativeToFrame(), behaviorForFixed, headerHeight(), footerHeight()).x();
 
         if (m_headerLayer)
             [m_headerLayer setPosition:FloatPoint(horizontalScrollOffsetForBanner, 0)];
@@ -175,7 +173,7 @@ void ScrollingTreeFrameScrollingNodeIOS::updateChildNodesAfterScroll(const Float
     if (!parent())
         fixedPositionRect = scrollingTree().fixedPositionRect();
     else
-        fixedPositionRect = FloatRect(scrollOffset, scrollableAreaSize());
+        fixedPositionRect = FloatRect(scrollPosition, scrollableAreaSize());
 
     for (auto& child : *m_children)
         child->updateLayersAfterAncestorChange(*this, fixedPositionRect, FloatSize());
@@ -183,7 +181,7 @@ void ScrollingTreeFrameScrollingNodeIOS::updateChildNodesAfterScroll(const Float
 
 FloatPoint ScrollingTreeFrameScrollingNodeIOS::minimumScrollPosition() const
 {
-    FloatPoint position;
+    FloatPoint position = ScrollableArea::scrollPositionFromOffset(FloatPoint(), toFloatSize(scrollOrigin()));
     
     if (scrollingTree().rootNode() == this && scrollingTree().scrollPinningBehavior() == PinToBottom)
         position.setY(maximumScrollPosition().y());
@@ -193,9 +191,7 @@ FloatPoint ScrollingTreeFrameScrollingNodeIOS::minimumScrollPosition() const
 
 FloatPoint ScrollingTreeFrameScrollingNodeIOS::maximumScrollPosition() const
 {
-    FloatPoint position(totalContentsSizeForRubberBand().width() - scrollableAreaSize().width(),
-        totalContentsSizeForRubberBand().height() - scrollableAreaSize().height());
-
+    FloatPoint position = ScrollableArea::scrollPositionFromOffset(FloatPoint(totalContentsSizeForRubberBand() - scrollableAreaSize()), toFloatSize(scrollOrigin()));
     position = position.expandedTo(FloatPoint());
 
     if (scrollingTree().rootNode() == this && scrollingTree().scrollPinningBehavior() == PinToTop)
index cad8994..681f324 100644 (file)
 #import "Logging.h"
 #import "NSScrollerImpDetails.h"
 #import "PlatformWheelEvent.h"
+#import "ScrollableArea.h"
 #import "ScrollingCoordinator.h"
 #import "ScrollingTree.h"
 #import "ScrollingStateTree.h"
 #import "Settings.h"
+#import "TextStream.h"
 #import "TileController.h"
 #import "WebLayer.h"
 
@@ -328,9 +330,9 @@ IntPoint ScrollingTreeFrameScrollingNodeMac::absoluteScrollPosition()
     return roundedIntPoint(scrollPosition());
 }
 
-void ScrollingTreeFrameScrollingNodeMac::immediateScrollBy(const FloatSize& offset)
+void ScrollingTreeFrameScrollingNodeMac::immediateScrollBy(const FloatSize& delta)
 {
-    scrollBy(offset);
+    scrollBy(delta);
 }
 
 void ScrollingTreeFrameScrollingNodeMac::immediateScrollByWithoutContentEdgeConstraints(const FloatSize& offset)
@@ -364,12 +366,13 @@ FloatPoint ScrollingTreeFrameScrollingNodeMac::scrollPosition() const
     if (shouldUpdateScrollLayerPositionSynchronously())
         return m_probableMainThreadScrollPosition;
 
-    CGPoint scrollLayerPosition = m_scrollLayer.get().position;
-    return FloatPoint(-scrollLayerPosition.x + scrollOrigin().x(), -scrollLayerPosition.y + scrollOrigin().y());
+    return -m_scrollLayer.get().position;
 }
 
 void ScrollingTreeFrameScrollingNodeMac::setScrollPosition(const FloatPoint& scrollPosition)
 {
+    LOG_WITH_STREAM(Scrolling, stream << "ScrollingTreeFrameScrollingNodeMac::setScrollPosition " << scrollPosition << " scrollPosition(): " << this->scrollPosition() << " min: " << minimumScrollPosition() << " max: " << maximumScrollPosition());
+
     // Scroll deltas can be non-integral with some input devices, so scrollPosition may not be integral.
     // FIXME: when we support half-pixel scroll positions on Retina displays, this will need to round to half pixels.
     FloatPoint roundedPosition(roundf(scrollPosition.x()), roundf(scrollPosition.y()));
@@ -397,13 +400,13 @@ void ScrollingTreeFrameScrollingNodeMac::setScrollPositionWithoutContentEdgeCons
 void ScrollingTreeFrameScrollingNodeMac::setScrollLayerPosition(const FloatPoint& position)
 {
     ASSERT(!shouldUpdateScrollLayerPositionSynchronously());
-    m_scrollLayer.get().position = CGPointMake(-position.x() + scrollOrigin().x(), -position.y() + scrollOrigin().y());
+
+    m_scrollLayer.get().position = -position;
 
     ScrollBehaviorForFixedElements behaviorForFixed = scrollBehaviorForFixedElements();
-    LayoutPoint scrollOffset = LayoutPoint(position) - toLayoutSize(scrollOrigin());
-    FloatRect viewportRect(FloatPoint(), scrollableAreaSize());
+    FloatRect viewportRect(position, scrollableAreaSize());
     
-    FloatPoint scrollPositionForFixedChildren = FrameView::scrollPositionForFixedPosition(enclosingLayoutRect(viewportRect), LayoutSize(totalContentsSize()), scrollOffset, scrollOrigin(), frameScaleFactor(),
+    FloatPoint scrollPositionForFixedChildren = FrameView::scrollPositionForFixedPosition(enclosingLayoutRect(viewportRect), LayoutSize(totalContentsSize()), LayoutPoint(position), scrollOrigin(), frameScaleFactor(),
         fixedElementsLayoutRelativeToFrame(), behaviorForFixed, headerHeight(), footerHeight());
     
     if (m_counterScrollingLayer)
@@ -424,7 +427,7 @@ void ScrollingTreeFrameScrollingNodeMac::setScrollLayerPosition(const FloatPoint
         // then we should recompute scrollOffsetForFixedChildren for the banner with a scale factor of 1.
         float horizontalScrollOffsetForBanner = scrollPositionForFixedChildren.x();
         if (frameScaleFactor() != 1)
-            horizontalScrollOffsetForBanner = FrameView::scrollPositionForFixedPosition(enclosingLayoutRect(viewportRect), LayoutSize(totalContentsSize()), scrollOffset, scrollOrigin(), 1, fixedElementsLayoutRelativeToFrame(), behaviorForFixed, headerHeight(), footerHeight()).x();
+            horizontalScrollOffsetForBanner = FrameView::scrollPositionForFixedPosition(enclosingLayoutRect(viewportRect), LayoutSize(totalContentsSize()), LayoutPoint(position), scrollOrigin(), 1, fixedElementsLayoutRelativeToFrame(), behaviorForFixed, headerHeight(), footerHeight()).x();
 
         if (m_headerLayer)
             m_headerLayer.get().position = FloatPoint(horizontalScrollOffsetForBanner, FrameView::yPositionForHeaderLayer(position, topContentInset));
@@ -472,7 +475,7 @@ void ScrollingTreeFrameScrollingNodeMac::updateLayersAfterViewportChange(const F
 
 FloatPoint ScrollingTreeFrameScrollingNodeMac::minimumScrollPosition() const
 {
-    FloatPoint position;
+    FloatPoint position = ScrollableArea::scrollPositionFromOffset(FloatPoint(), toFloatSize(scrollOrigin()));
     
     if (scrollingTree().rootNode() == this && scrollingTree().scrollPinningBehavior() == PinToBottom)
         position.setY(maximumScrollPosition().y());
@@ -482,7 +485,7 @@ FloatPoint ScrollingTreeFrameScrollingNodeMac::minimumScrollPosition() const
 
 FloatPoint ScrollingTreeFrameScrollingNodeMac::maximumScrollPosition() const
 {
-    FloatPoint position(totalContentsSizeForRubberBand() - scrollableAreaSize());
+    FloatPoint position = ScrollableArea::scrollPositionFromOffset(FloatPoint(totalContentsSizeForRubberBand() - scrollableAreaSize()), toFloatSize(scrollOrigin()));
     position = position.expandedTo(FloatPoint());
 
     if (scrollingTree().rootNode() == this && scrollingTree().scrollPinningBehavior() == PinToTop)
index a35a341..bb79aba 100644 (file)
@@ -429,8 +429,10 @@ void ScrollView::notifyPageThatContentAreaWillPaint() const
 {
 }
 
-void ScrollView::setScrollOffset(const IntPoint& offset)
+void ScrollView::setScrollOffset(const ScrollOffset& offset)
 {
+    LOG_WITH_STREAM(Scrolling, stream << "ScrollView::setScrollOffset " << offset);
+
     IntPoint constrainedOffset = offset;
     if (constrainsScrollingToContentEdge())
         constrainedOffset = constrainedOffset.constrainedBetween(IntPoint(), maximumScrollOffset());
@@ -471,6 +473,8 @@ void ScrollView::handleDeferredScrollUpdateAfterContentSizeChange()
 
 void ScrollView::scrollTo(const ScrollPosition& newPosition)
 {
+    LOG_WITH_STREAM(Scrolling, stream << "ScrollView::scrollTo " << newPosition << " min: " << minimumScrollPosition() << " max: " << maximumScrollPosition());
+
     IntSize scrollDelta = newPosition - m_scrollPosition;
     if (scrollDelta.isZero())
         return;
@@ -515,6 +519,8 @@ int ScrollView::scrollPosition(Scrollbar* scrollbar) const
 
 void ScrollView::setScrollPosition(const ScrollPosition& scrollPosition)
 {
+    LOG_WITH_STREAM(Scrolling, stream << "ScrollView::setScrollPosition " << scrollPosition);
+
     if (prohibitsScrolling())
         return;
 
@@ -568,6 +574,8 @@ IntSize ScrollView::overhangAmount() const
 
 void ScrollView::updateScrollbars(const ScrollPosition& desiredPosition)
 {
+    LOG_WITH_STREAM(Scrolling, stream << "ScrollView::updateScrollbars " << desiredPosition);
+
     if (m_inUpdateScrollbars || prohibitsScrolling() || platformWidget() || delegatesScrolling())
         return;
 
index 641f6f4..c2690ef 100644 (file)
@@ -67,7 +67,7 @@ public:
     // ScrollableArea functions.
     virtual int scrollSize(ScrollbarOrientation) const override;
     virtual int scrollPosition(Scrollbar*) const override;
-    WEBCORE_EXPORT virtual void setScrollOffset(const IntPoint&) override;
+    WEBCORE_EXPORT virtual void setScrollOffset(const ScrollOffset&) override;
     virtual bool isScrollCornerVisible() const override;
     virtual void scrollbarStyleChanged(ScrollbarStyle, bool forceUpdate) override;
 
index d433422..05e881b 100644 (file)
@@ -151,11 +151,11 @@ void ScrollableArea::notifyScrollPositionChanged(const ScrollPosition& position)
     scrollAnimator().setCurrentPosition(position);
 }
 
-void ScrollableArea::scrollPositionChanged(const IntPoint& position)
+void ScrollableArea::scrollPositionChanged(const ScrollPosition& position)
 {
     IntPoint oldPosition = scrollPosition();
     // Tell the derived class to scroll its contents.
-    setScrollOffset(position);
+    setScrollOffset(scrollOffsetFromPosition(position));
 
     Scrollbar* verticalScrollbar = this->verticalScrollbar();
 
@@ -207,17 +207,18 @@ bool ScrollableArea::handleTouchEvent(const PlatformTouchEvent& touchEvent)
 #endif
 
 // NOTE: Only called from Internals for testing.
-void ScrollableArea::setScrollOffsetFromInternals(const IntPoint& offset)
+void ScrollableArea::setScrollOffsetFromInternals(const ScrollOffset& offset)
 {
     setScrollOffsetFromAnimation(offset);
 }
 
-void ScrollableArea::setScrollOffsetFromAnimation(const IntPoint& offset)
+void ScrollableArea::setScrollOffsetFromAnimation(const ScrollOffset& offset)
 {
-    if (requestScrollPositionUpdate(offset))
+    ScrollPosition position = scrollPositionFromOffset(offset);
+    if (requestScrollPositionUpdate(position))
         return;
 
-    scrollPositionChanged(offset);
+    scrollPositionChanged(position);
 }
 
 void ScrollableArea::willStartLiveResize()
@@ -549,12 +550,12 @@ ScrollOffset ScrollableArea::maximumScrollOffset() const
 
 ScrollPosition ScrollableArea::scrollPositionFromOffset(ScrollOffset offset) const
 {
-    return IntPoint(toIntSize(offset) - toIntSize(m_scrollOrigin));
+    return scrollPositionFromOffset(offset, toIntSize(m_scrollOrigin));
 }
 
 ScrollOffset ScrollableArea::scrollOffsetFromPosition(ScrollPosition position) const
 {
-    return IntPoint(toIntSize(position) + toIntSize(m_scrollOrigin));
+    return scrollOffsetFromPosition(position, toIntSize(m_scrollOrigin));
 }
 
 bool ScrollableArea::scrolledToTop() const
index 3c71cc8..02bffbd 100644 (file)
@@ -193,6 +193,18 @@ public:
     WEBCORE_EXPORT ScrollPosition scrollPositionFromOffset(ScrollOffset) const;
     WEBCORE_EXPORT ScrollOffset scrollOffsetFromPosition(ScrollPosition) const;
 
+    template<typename PositionType, typename SizeType>
+    static PositionType scrollPositionFromOffset(PositionType offset, SizeType scrollOrigin)
+    {
+        return offset - scrollOrigin;
+    }
+
+    template<typename PositionType, typename SizeType>
+    static PositionType scrollOffsetFromPosition(PositionType position, SizeType scrollOrigin)
+    {
+        return position + scrollOrigin;
+    }
+
     WEBCORE_EXPORT virtual bool scrolledToTop() const;
     WEBCORE_EXPORT virtual bool scrolledToBottom() const;
     WEBCORE_EXPORT virtual bool scrolledToLeft() const;
@@ -249,7 +261,7 @@ public:
     virtual bool scrollAnimatorEnabled() const { return false; }
 
     // NOTE: Only called from Internals for testing.
-    WEBCORE_EXPORT void setScrollOffsetFromInternals(const IntPoint&);
+    WEBCORE_EXPORT void setScrollOffsetFromInternals(const ScrollOffset&);
 
     WEBCORE_EXPORT static LayoutPoint constrainScrollPositionForOverhang(const LayoutRect& visibleContentRect, const LayoutSize& totalContentsSize, const LayoutPoint& scrollPosition, const LayoutPoint& scrollOrigin, int headerHeight, int footetHeight);
     LayoutPoint constrainScrollPositionForOverhang(const LayoutPoint& scrollPosition);
@@ -312,15 +324,15 @@ protected:
 
 private:
     WEBCORE_EXPORT virtual IntRect visibleContentRectInternal(VisibleContentRectIncludesScrollbars, VisibleContentRectBehavior) const;
-    void scrollPositionChanged(const IntPoint&);
+    void scrollPositionChanged(const ScrollPosition&);
     
     // NOTE: Only called from the ScrollAnimator.
     friend class ScrollAnimator;
-    void setScrollOffsetFromAnimation(const IntPoint&);
+    void setScrollOffsetFromAnimation(const ScrollOffset&);
 
     // This function should be overriden by subclasses to perform the actual
     // scroll of the content.
-    virtual void setScrollOffset(const IntPoint&) = 0;
+    virtual void setScrollOffset(const ScrollOffset&) = 0;
 
     mutable std::unique_ptr<ScrollAnimator> m_scrollAnimator;
 
index e0337f4..384b188 100644 (file)
@@ -608,7 +608,7 @@ int RenderListBox::scrollPosition(Scrollbar*) const
     return m_indexOffset;
 }
 
-void RenderListBox::setScrollOffset(const IntPoint& offset)
+void RenderListBox::setScrollOffset(const ScrollOffset& offset)
 {
     scrollTo(offset.y());
 }
index 4cdf876..cce33be 100644 (file)
@@ -111,7 +111,7 @@ private:
     // ScrollableArea interface.
     virtual int scrollSize(ScrollbarOrientation) const override;
     virtual int scrollPosition(Scrollbar*) const override;
-    virtual void setScrollOffset(const IntPoint&) override;
+    virtual void setScrollOffset(const ScrollOffset&) override;
     virtual void invalidateScrollbarRect(Scrollbar*, const IntRect&) override;
     virtual bool isActive() const override;
     virtual bool isScrollCornerVisible() const override { return false; } // We don't support resize on list boxes yet. If we did these would have to change.
index 153cf94..0b66b14 100644 (file)
@@ -1,3 +1,33 @@
+2016-01-01  Simon Fraser  <simon.fraser@apple.com>
+
+        Fix reload and programmatic scrolling in RTL documents
+        https://bugs.webkit.org/show_bug.cgi?id=152639
+
+        Reviewed by Zalan Bujtas.
+
+        Reloading a left-scrolled RTL document would cause the content to appear
+        at an odd offset, and programmatic sideways scrolls in RTL documents also
+        jumped to the wrong location.
+        
+        Fix by resolving offset/position confusion in ScrollableArea::scrollPositionChanged()
+        and the scrolling tree.
+        
+        ScrollableArea::scrollPositionChanged() was erroneously passing a scrollPosition
+        to setScrollOffset(). 
+        
+        ScrollingTreeFrameScrollingNode* were confused about offsets and positions. It
+        turns out that the layer position is just -scrollPosition, but minimumScrollPosition()
+        and maximumScrollPosition() need fixing to return positions, not offsets.
+        
+        ScrollingTreeFrameScrollingNode::viewToContentsOffset() was also doing incorrect
+        math with scrollOrigin, which was detected by a failing test.
+        
+        Add more logging to the Scrolling channel.
+
+        * WebProcess/Plugins/PDF/DeprecatedPDFPlugin.h:
+        * WebProcess/Plugins/PDF/DeprecatedPDFPlugin.mm:
+        (WebKit::PDFPlugin::setScrollOffset):
+
 2016-01-01  Michael Catanzaro  <mcatanzaro@igalia.com>
 
         [SOUP] REGRESSION(r192761): Broke resource URIs for applications that use g_resource_load in a web extension
index d29ac94..17334d9 100644 (file)
@@ -189,7 +189,7 @@ private:
     virtual bool isScrollableOrRubberbandable() override { return true; }
     virtual bool hasScrollableOrRubberbandableAncestor() override { return true; }
     virtual WebCore::IntRect scrollableAreaBoundingBox(bool* = nullptr) const override;
-    virtual void setScrollOffset(const WebCore::IntPoint&) override;
+    virtual void setScrollOffset(const WebCore::ScrollOffset&) override;
     virtual void invalidateScrollbarRect(WebCore::Scrollbar*, const WebCore::IntRect&) override;
     virtual void invalidateScrollCornerRect(const WebCore::IntRect&) override;
     virtual WebCore::IntPoint lastKnownMousePosition() const override { return m_lastMousePositionInPluginCoordinates; }
index 9467459..909c406 100644 (file)
@@ -1538,7 +1538,7 @@ bool PDFPlugin::isEditingCommandEnabled(const String& commandName)
     return false;
 }
 
-void PDFPlugin::setScrollOffset(const IntPoint& offset)
+void PDFPlugin::setScrollOffset(const ScrollOffset& offset)
 {
     m_scrollOffset = IntSize(offset.x(), offset.y());