LayoutBox::scrollRectToVisible doesn't respect overflow:hidden property.

third_party/WebKit/Source/core/layout/LayoutBox.cpp

Index: third_party/WebKit/Source/core/layout/LayoutBox.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutBox.cpp b/third_party/WebKit/Source/core/layout/LayoutBox.cpp
index 0272e0ac8552e1f0090d5ad84d5e849afad17c0b..44b287bc320d8ba6f8bc12d9fcb12d452816862e 100644
--- a/third_party/WebKit/Source/core/layout/LayoutBox.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutBox.cpp
@@ -88,6 +88,7 @@ static OverrideSizeMap* gExtraBlockOffsetMap = nullptr;
 // autoscroll is started.
 static const int autoscrollBeltSize = 20;
 static const unsigned backgroundObscurationTestMaxDepth = 4;
+static const ScrollAlignment alignNoScroll = { ScrollAlignmentNoScroll, ScrollAlignmentNoScroll, ScrollAlignmentNoScroll };
 
 LayoutBox::LayoutBox(ContainerNode* node)
     : LayoutBoxModelObject(node)
@@ -503,7 +504,12 @@ static bool isDisallowedAutoscroll(HTMLFrameOwnerElement* ownerElement, FrameVie
     return false;
 }
 
-void LayoutBox::scrollRectToVisible(const LayoutRect& rect, const ScrollAlignment& alignX, const ScrollAlignment& alignY)
+bool LayoutBox::scrollRestrictedByLineClamp() const
+{
+    return parent() ? !parent()->style()->lineClamp().isNone() : false;
+}
+
+void LayoutBox::scrollRectToVisible(const LayoutRect& rect, const ScrollAlignment& alignX, const ScrollAlignment& alignY, bool programmaticScroll)
 {
     // Presumably the same issue as in setScrollTop. See crbug.com/343132.
     DisableCompositingQueryAsserts disabler;
@@ -511,31 +517,38 @@ void LayoutBox::scrollRectToVisible(const LayoutRect& rect, const ScrollAlignmen
     LayoutBox* parentBox = nullptr;
     LayoutRect newRect = rect;
 
-    bool restrictedByLineClamp = false;
     if (parent()) {
         parentBox = parent()->enclosingBox();
-        restrictedByLineClamp = !parent()->style()->lineClamp().isNone();
     }
 
-    if (hasOverflowClip() && !restrictedByLineClamp) {
-        // Don't scroll to reveal an overflow layer that is restricted by the -webkit-line-clamp property.
-        // This will prevent us from revealing text hidden by the slider in Safari RSS.
-        newRect = layer()->scrollableArea()->scrollIntoView(rect, alignX, alignY);
-    } else if (!parentBox && canBeProgramaticallyScrolled()) {
-        if (FrameView* frameView = this->frameView()) {
-            HTMLFrameOwnerElement* ownerElement = document().ownerElement();
-            if (!isDisallowedAutoscroll(ownerElement, frameView)) {
-                frameView->scrollableArea()->scrollIntoView(rect, alignX, alignY);
-
-                if (ownerElement && ownerElement->layoutObject()) {
-                    if (frameView->safeToPropagateScrollToParent()) {
-                        parentBox = ownerElement->layoutObject()->enclosingBox();
-                        // FIXME: This doesn't correctly convert the rect to
-                        // absolute coordinates in the parent.
-                        newRect.setX(rect.x() - frameView->scrollX() + frameView->x());
-                        newRect.setY(rect.y() - frameView->scrollY() + frameView->y());
-                    } else {
-                        parentBox = nullptr;
+    // If the computed style has overflowX/Y set to hidden, we shouldn't scroll
+    // into view unless its a programmatic scroll. Note that some LayoutObjects
+    // have overflow:hidden set by default (input fields), and so we need to
+    // override this behavior subclasses.
+    if (programmaticScroll || style()->overflowX() != OHIDDEN || style()->overflowY() != OHIDDEN) {

[bokan, 2015/09/25 22:26:26]

Hmm...I don't like that the knowledge of when scrolling is allowed or not is
enshrined here and in ScrollableArea (and likely lots of other places). We
should start trying to consolidate it. (this predates you....and me...)

How about this: lets make the ScrollableArea::scrollIntoView method also take
whether the scroll is a user/programmatic scroll. ScrollableArea already has
this notion on most of its other methods and there's an enum ScrollType. From
what I can tell, all the overrides of scrollIntoView call into setScrollPosition
with scrollType=Programmatic so you'd just have to use the parameter there.
Unfortunately, setScrollPosition doesn't do a check whether the area has
overflow:hidden (ScrollableArea::userScroll does - it should eventually be
merged with setScrollPosition anyway, but that's a story for another day) so
you'll have to add that. That should hopefully remove much of the complexity
from this method.

What do you think?

+        ScrollAlignment newAlignX, newAlignY;
+        newAlignX = !programmaticScroll && style()->overflowX() == OHIDDEN ? alignNoScroll : alignX;
+        newAlignY = !programmaticScroll && style()->overflowY() == OHIDDEN ? alignNoScroll : alignY;
+        if (hasOverflowClip() && !scrollRestrictedByLineClamp()) {
+            // Don't scroll to reveal an overflow layer that is restricted by the -webkit-line-clamp property.
+            // This will prevent us from revealing text hidden by the slider in Safari RSS.
+            newRect = layer()->scrollableArea()->scrollIntoView(rect, newAlignX, newAlignY);
+        } else if (!parentBox && canBeProgramaticallyScrolled()) {

[bokan, 2015/09/25 22:26:26]

canBeProgrammaticallyScrolled seems like it should only matter if your
programmaticScroll param is true.

+            if (FrameView* frameView = this->frameView()) {
+                HTMLFrameOwnerElement* ownerElement = document().ownerElement();
+                if (!isDisallowedAutoscroll(ownerElement, frameView)) {
+                    frameView->scrollableArea()->scrollIntoView(rect, newAlignX, newAlignY);
+
+                    if (ownerElement && ownerElement->layoutObject()) {
+                        if (frameView->safeToPropagateScrollToParent()) {
+                            parentBox = ownerElement->layoutObject()->enclosingBox();
+                            // FIXME: This doesn't correctly convert the rect to
+                            // absolute coordinates in the parent.
+                            newRect.setX(rect.x() - frameView->scrollX() + frameView->x());
+                            newRect.setY(rect.y() - frameView->scrollY() + frameView->y());
+                        } else {
+                            parentBox = nullptr;
+                        }
                     }
                 }
             }
@@ -550,7 +563,7 @@ void LayoutBox::scrollRectToVisible(const LayoutRect& rect, const ScrollAlignmen
         parentBox = enclosingScrollableBox();
 
     if (parentBox)
-        parentBox->scrollRectToVisible(newRect, alignX, alignY);
+        parentBox->scrollRectToVisible(newRect, alignX, alignY, programmaticScroll);
 }
 
 void LayoutBox::absoluteRects(Vector<IntRect>& rects, const LayoutPoint& accumulatedOffset) const
@@ -773,7 +786,7 @@ void LayoutBox::autoscroll(const IntPoint& positionInRootFrame)
         return;
 
     IntPoint positionInContent = frameView->rootFrameToContents(positionInRootFrame);
-    scrollRectToVisible(LayoutRect(positionInContent, LayoutSize(1, 1)), ScrollAlignment::alignToEdgeIfNeeded, ScrollAlignment::alignToEdgeIfNeeded);
+    scrollRectToVisible(LayoutRect(positionInContent, LayoutSize(1, 1)), ScrollAlignment::alignToEdgeIfNeeded, ScrollAlignment::alignToEdgeIfNeeded, false);
 }
 
 // There are two kinds of layoutObject that can autoscroll.