REGRESSION (topContentInset): Searching through Facebook Messenger's chat causes
authorbdakin@apple.com <bdakin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 14 May 2014 00:13:56 +0000 (00:13 +0000)
committerbdakin@apple.com <bdakin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 14 May 2014 00:13:56 +0000 (00:13 +0000)
scrolling in News Feed
https://bugs.webkit.org/show_bug.cgi?id=132889
-and corresponding-
<rdar://problem/16715716>

Reviewed by Simon Fraser.

Source/WebCore:
First of all, scrollOffsetRelativeToDocument() was very poorly named. This patch
re-names it to the much more accurate documentScrollOffsetRelativeToViewOrigin().
Re-naming it makes it clear that ONE call site was not getting the right offset.
That call site does not want to know the document’s position relative to the view
origin, but rather it wants to know the Document’s position relative to the
scrolling origin.

Export new name.
* WebCore.exp.in:

Use newly re-named documentScrollPositionRelativeToViewOrigin().
* page/FrameView.cpp:
(WebCore::FrameView::convertToRenderer):
* platform/ScrollView.cpp:
(WebCore::ScrollView::documentScrollOffsetRelativeToViewOrigin):
(WebCore::ScrollView::documentScrollPositionRelativeToViewOrigin):
(WebCore::ScrollView::documentScrollOffsetRelativeToScrollableAreaOrigin):
(WebCore::ScrollView::rootViewToContents):
(WebCore::ScrollView::windowToContents):
(WebCore::ScrollView::scrollOffsetRelativeToDocument): Deleted.
(WebCore::ScrollView::scrollPositionRelativeToDocument): Deleted.
* platform/ScrollView.h:

THIS is the spot that needs the new function,
documentScrollOffsetRelativeToScrollableAreaOrigin()()
* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::scrollRectToVisible):

Source/WebKit2:
Re-name scrollOffsetRelativeToDocument() to
documentScrollPositionRelativeToViewOrigin().
* WebProcess/WebPage/FindController.cpp:
(WebKit::FindController::rectsForTextMatches):
* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::determinePrimarySnapshottedPlugIn):

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

Source/WebCore/ChangeLog
Source/WebCore/WebCore.exp.in
Source/WebCore/page/FrameView.cpp
Source/WebCore/platform/ScrollView.cpp
Source/WebCore/platform/ScrollView.h
Source/WebCore/rendering/RenderLayer.cpp
Source/WebKit2/ChangeLog
Source/WebKit2/WebProcess/WebPage/FindController.cpp
Source/WebKit2/WebProcess/WebPage/WebPage.cpp

index cb63ab0..47b34b3 100644 (file)
@@ -1,3 +1,41 @@
+2014-05-13  Beth Dakin  <bdakin@apple.com>
+
+        REGRESSION (topContentInset): Searching through Facebook Messenger's chat causes 
+        scrolling in News Feed
+        https://bugs.webkit.org/show_bug.cgi?id=132889
+        -and corresponding-
+        <rdar://problem/16715716>
+
+        Reviewed by Simon Fraser.
+
+        First of all, scrollOffsetRelativeToDocument() was very poorly named. This patch 
+        re-names it to the much more accurate documentScrollOffsetRelativeToViewOrigin(). 
+        Re-naming it makes it clear that ONE call site was not getting the right offset. 
+        That call site does not want to know the document’s position relative to the view 
+        origin, but rather it wants to know the Document’s position relative to the 
+        scrolling origin.
+
+        Export new name.
+        * WebCore.exp.in:
+
+        Use newly re-named documentScrollPositionRelativeToViewOrigin().
+        * page/FrameView.cpp:
+        (WebCore::FrameView::convertToRenderer):
+        * platform/ScrollView.cpp:
+        (WebCore::ScrollView::documentScrollOffsetRelativeToViewOrigin):
+        (WebCore::ScrollView::documentScrollPositionRelativeToViewOrigin):
+        (WebCore::ScrollView::documentScrollOffsetRelativeToScrollableAreaOrigin):
+        (WebCore::ScrollView::rootViewToContents):
+        (WebCore::ScrollView::windowToContents):
+        (WebCore::ScrollView::scrollOffsetRelativeToDocument): Deleted.
+        (WebCore::ScrollView::scrollPositionRelativeToDocument): Deleted.
+        * platform/ScrollView.h:
+
+        THIS is the spot that needs the new function, 
+        documentScrollOffsetRelativeToScrollableAreaOrigin()()
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::scrollRectToVisible):
+
 2014-05-13  Dean Jackson  <dino@apple.com>
 
         [iOS] Page scale update messages for media controls should only fire at the end of zooming
index 3b81536..6dd059f 100644 (file)
@@ -1507,7 +1507,7 @@ __ZNK7WebCore10ScrollView18contentsToRootViewERKNS_7IntRectE
 __ZNK7WebCore10ScrollView18contentsToRootViewERKNS_8IntPointE
 __ZNK7WebCore10ScrollView18rootViewToContentsERKNS_8IntPointE
 __ZNK7WebCore10ScrollView23rootViewToTotalContentsERKNS_8IntPointE
-__ZNK7WebCore10ScrollView30scrollOffsetRelativeToDocumentEv
+__ZNK7WebCore10ScrollView40documentScrollOffsetRelativeToViewOriginEv
 __ZNK7WebCore10StorageMap6lengthEv
 __ZNK7WebCore10StorageMap7getItemERKN3WTF6StringE
 __ZNK7WebCore10StorageMap8containsERKN3WTF6StringE
index e3ba626..fa05a0c 100644 (file)
@@ -3879,7 +3879,7 @@ IntRect FrameView::convertToRenderer(const RenderElement* renderer, const IntRec
     
     // Convert from FrameView coords into page ("absolute") coordinates.
     if (!delegatesScrolling())
-        rect.moveBy(scrollPositionRelativeToDocument());
+        rect.moveBy(documentScrollPositionRelativeToViewOrigin());
 
     // FIXME: we don't have a way to map an absolute rect down to a local quad, so just
     // move the rect for now.
@@ -3903,7 +3903,7 @@ IntPoint FrameView::convertToRenderer(const RenderElement* renderer, const IntPo
 
     // Convert from FrameView coords into page ("absolute") coordinates.
     if (!delegatesScrolling())
-        point = point + scrollPositionRelativeToDocument();
+        point = point + documentScrollPositionRelativeToViewOrigin();
 
     return roundedIntPoint(renderer->absoluteToLocal(point, UseTransforms));
 }
index 475437f..05f5d8a 100644 (file)
@@ -412,18 +412,22 @@ IntPoint ScrollView::adjustScrollPositionWithinRange(const IntPoint& scrollPoint
     return newScrollPosition;
 }
 
-IntSize ScrollView::scrollOffsetRelativeToDocument() const
+IntSize ScrollView::documentScrollOffsetRelativeToViewOrigin() const
 {
-    IntSize scrollOffset = this->scrollOffset();
-    return IntSize(scrollOffset.width(), scrollOffset.height() - headerHeight() - topContentInset());
+    return scrollOffset() - IntSize(0, headerHeight() + topContentInset());
 }
 
-IntPoint ScrollView::scrollPositionRelativeToDocument() const
+IntPoint ScrollView::documentScrollPositionRelativeToViewOrigin() const
 {
     IntPoint scrollPosition = this->scrollPosition();
     return IntPoint(scrollPosition.x(), scrollPosition.y() - headerHeight() - topContentInset());
 }
 
+IntSize ScrollView::documentScrollOffsetRelativeToScrollableAreaOrigin() const
+{
+    return scrollOffset() - IntSize(0, headerHeight());
+}
+
 int ScrollView::scrollSize(ScrollbarOrientation orientation) const
 {
     // If no scrollbars are present, it does not indicate content is not be scrollable.
@@ -822,7 +826,7 @@ IntPoint ScrollView::rootViewToContents(const IntPoint& rootViewPoint) const
         return convertFromRootView(rootViewPoint);
 
     IntPoint viewPoint = convertFromRootView(rootViewPoint);
-    return viewPoint + scrollOffsetRelativeToDocument();
+    return viewPoint + documentScrollOffsetRelativeToViewOrigin();
 }
 
 IntPoint ScrollView::contentsToRootView(const IntPoint& contentsPoint) const
@@ -840,7 +844,7 @@ IntRect ScrollView::rootViewToContents(const IntRect& rootViewRect) const
         return convertFromRootView(rootViewRect);
 
     IntRect viewRect = convertFromRootView(rootViewRect);
-    viewRect.move(scrollOffsetRelativeToDocument());
+    viewRect.move(documentScrollOffsetRelativeToViewOrigin());
     return viewRect;
 }
 
@@ -869,7 +873,7 @@ IntPoint ScrollView::windowToContents(const IntPoint& windowPoint) const
         return convertFromContainingWindow(windowPoint);
 
     IntPoint viewPoint = convertFromContainingWindow(windowPoint);
-    return viewPoint + scrollOffsetRelativeToDocument();
+    return viewPoint + documentScrollOffsetRelativeToViewOrigin();
 }
 
 IntPoint ScrollView::contentsToWindow(const IntPoint& contentsPoint) const
@@ -887,7 +891,7 @@ IntRect ScrollView::windowToContents(const IntRect& windowRect) const
         return convertFromContainingWindow(windowRect);
 
     IntRect viewRect = convertFromContainingWindow(windowRect);
-    viewRect.move(scrollOffsetRelativeToDocument());
+    viewRect.move(documentScrollOffsetRelativeToViewOrigin());
     return viewRect;
 }
 
index 1493e78..8f3d876 100644 (file)
@@ -241,12 +241,21 @@ public:
     IntPoint actualScrollPosition() const { return unobscuredContentRect().location(); }
 #endif
 
-    // scrollOffset() anchors its (0,0) point at the top end of the header if this ScrollableArea
-    // has a header, so it is relative to the totalContentsSize(). scrollOffsetRelativeToDocument()
-    // anchors (0,0) at the top of the Document, which will be beneath any headers, so it is relative
-    // to contentsSize().
-    IntSize scrollOffsetRelativeToDocument() const;
-    IntPoint scrollPositionRelativeToDocument() const;
+    // scrollOffset() anchors its (0,0) point at the ScrollableArea's origin. When the Page has a
+    // header, the header is positioned at (0,0), ABOVE the start of the Document. So when a page with
+    // a header is pinned to the top, the scrollOffset() is (0,0), but the Document is actually at
+    // (0, -headerHeight()). documentScrollOffsetRelativeToScrollableAreaOrigin() will return this
+    // version of the offset, which tracks the top of Document relative to where scrolling was achored.
+    IntSize documentScrollOffsetRelativeToScrollableAreaOrigin() const;
+
+    // scrollOffset() anchors its (0,0) point at the ScrollableArea's origin. The top of the scrolling
+    // layer does not represent the top of the view when there is a topContentInset. Additionally, as
+    // detailed above, the origin of the scrolling layer also does not necessarily correspond with the
+    // top of the document anyway, since there could also be header. documentScrollOffsetRelativeToViewOrigin()
+    // will return a version of the current scroll offset which tracks the top of the Document
+    // relative to the very top of the view.
+    IntSize documentScrollOffsetRelativeToViewOrigin() const;
+    IntPoint documentScrollPositionRelativeToViewOrigin() const;
 
     virtual IntSize overhangAmount() const override;
 
index a93ca00..8b66240 100644 (file)
@@ -2409,8 +2409,8 @@ void RenderLayer::scrollRectToVisible(const LayoutRect& rect, const ScrollAlignm
 #if !PLATFORM(IOS)
             LayoutRect viewRect = frameView.visibleContentRect();
             LayoutRect visibleRectRelativeToDocument = viewRect;
-            IntSize scrollOffsetRelativeToDocument = frameView.scrollOffsetRelativeToDocument();
-            visibleRectRelativeToDocument.setLocation(IntPoint(scrollOffsetRelativeToDocument.width(), scrollOffsetRelativeToDocument.height()));
+            IntSize documentScrollOffsetRelativeToScrollableAreaOrigin = frameView.documentScrollOffsetRelativeToScrollableAreaOrigin();
+            visibleRectRelativeToDocument.setLocation(IntPoint(documentScrollOffsetRelativeToScrollableAreaOrigin.width(), documentScrollOffsetRelativeToScrollableAreaOrigin.height()));
 #else
             LayoutRect viewRect = frameView.unobscuredContentRect();
             LayoutRect visibleRectRelativeToDocument = viewRect;
index 9200ff6..79895b0 100644 (file)
@@ -1,3 +1,20 @@
+2014-05-13  Beth Dakin  <bdakin@apple.com>
+
+        REGRESSION (topContentInset): Searching through Facebook Messenger's chat causes 
+        scrolling in News Feed
+        https://bugs.webkit.org/show_bug.cgi?id=132889
+        -and corresponding-
+        <rdar://problem/16715716>
+
+        Reviewed by Simon Fraser.
+
+        Re-name scrollOffsetRelativeToDocument() to 
+        documentScrollPositionRelativeToViewOrigin().
+        * WebProcess/WebPage/FindController.cpp:
+        (WebKit::FindController::rectsForTextMatches):
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::determinePrimarySnapshottedPlugIn):
+
 2014-05-13  Dean Jackson  <dino@apple.com>
 
         [iOS] Page scale update messages for media controls should only fire at the end of zooming
index 4688fed..1f00005 100644 (file)
@@ -418,7 +418,7 @@ Vector<IntRect> FindController::rectsForTextMatches()
 
         IntRect visibleRect = frame->view()->visibleContentRect();
         Vector<IntRect> frameRects = document->markers().renderedRectsForMarkers(DocumentMarker::TextMatch);
-        IntPoint frameOffset(-frame->view()->scrollOffsetRelativeToDocument().width(), -frame->view()->scrollOffsetRelativeToDocument().height());
+        IntPoint frameOffset(-frame->view()->documentScrollOffsetRelativeToViewOrigin().width(), -frame->view()->documentScrollOffsetRelativeToViewOrigin().height());
         frameOffset = frame->view()->convertToContainingWindow(frameOffset);
 
         for (Vector<IntRect>::iterator it = frameRects.begin(), end = frameRects.end(); it != end; ++it) {
index ea995ee..87fb6e7 100644 (file)
@@ -4395,7 +4395,7 @@ void WebPage::determinePrimarySnapshottedPlugIn()
             IntRect plugInRectRelativeToView = plugInImageElement.clientRect();
             if (plugInRectRelativeToView.isEmpty())
                 continue;
-            IntSize scrollOffset = mainFrame.view()->scrollOffsetRelativeToDocument();
+            IntSize scrollOffset = mainFrame.view()->documentScrollOffsetRelativeToViewOrigin();
             IntRect plugInRectRelativeToTopDocument(plugInRectRelativeToView.location() + scrollOffset, plugInRectRelativeToView.size());
             if (!plugInRectRelativeToTopDocument.intersects(searchRect))
                 continue;