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

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

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2005 Allan Sandfeld Jensen (kde@carewolf.com)
  *           (C) 2005, 2006 Samuel Weinig (sam.weinig@gmail.com)
  * Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010 Apple Inc. All rights reserved.
  * Copyright (C) 2013 Adobe Systems Incorporated. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ skipping 70 matching lines @@
 static OverrideSizeMap* gOverrideContainingBlockLogicalHeightMap = nullptr;
 static OverrideSizeMap* gOverrideContainingBlockLogicalWidthMap = nullptr;
 static OverrideSizeMap* gExtraInlineOffsetMap = nullptr;
 static OverrideSizeMap* gExtraBlockOffsetMap = nullptr;
 
 
 // Size of border belt for autoscroll. When mouse pointer in border belt,
 // 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)
     , m_intrinsicContentLogicalHeight(-1)
     , m_minPreferredLogicalWidth(-1)
     , m_maxPreferredLogicalWidth(-1)
 {
     setIsBox();
 }
 
@@ skipping 395 matching lines @@
     if (ownerElement && isHTMLFrameElementBase(*ownerElement)) {
         HTMLFrameElementBase* frameElementBase = toHTMLFrameElementBase(ownerElement);
         if (Page* page = frameView->frame().page()) {
             return page->autoscrollController().autoscrollInProgress()
                 && frameElementBase->scrollingMode() == ScrollbarAlwaysOff;
         }
     }
     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;
 
     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 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;
+                    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 we are fixed-position, it is useless to scroll the parent.
     if (hasLayer() && layer()->scrollsWithViewport())
         return;
 
     if (frame()->page()->autoscrollController().autoscrollInProgress())
         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
 {
     rects.append(pixelSnappedIntRect(accumulatedOffset, size()));
 }
 
 void LayoutBox::absoluteQuads(Vector<FloatQuad>& quads, bool* wasFixed) const
 {
     quads.append(localToAbsoluteQuad(FloatRect(0, 0, m_frameRect.width().toFloat(), m_frameRect.height().toFloat()), 0 /* mode */, wasFixed));
@@ skipping 202 matching lines @@
 {
     LocalFrame* frame = this->frame();
     if (!frame)
         return;
 
     FrameView* frameView = frame->view();
     if (!frameView)
         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.
 bool LayoutBox::canAutoscroll() const
 {
     if (node() && node()->isDocumentNode())
         return view()->frameView()->isScrollable();
 
     // Check for a box that can be scrolled in its own right.
     return canBeScrolledAndHasScrollableArea();
@@ skipping 3961 matching lines @@
     StyleImage* borderImage = style()->borderImage().image();
     return borderImage && borderImage->canRender(*this, style()->effectiveZoom()) && borderImage->isLoaded();
 }
 
 ShapeOutsideInfo* LayoutBox::shapeOutsideInfo() const
 {
     return ShapeOutsideInfo::isEnabledFor(*this) ? ShapeOutsideInfo::info(*this) : nullptr;
 }
 
 } // namespace blink