Disable scroll anchoring is an element within the scroll changes its in-flow state

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

Index: third_party/WebKit/Source/core/layout/LayoutObject.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutObject.cpp b/third_party/WebKit/Source/core/layout/LayoutObject.cpp
index 37f163851d52e143dff84248748b9b67fc700f65..aec58916d2f20a8cd01cfc606bcba442100ffe5f 100644
--- a/third_party/WebKit/Source/core/layout/LayoutObject.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutObject.cpp
@@ -1712,6 +1712,24 @@ static inline bool areCursorsEqual(const ComputedStyle* a, const ComputedStyle*
     return a->cursor() == b->cursor() && (a->cursors() == b->cursors() || areNonIdenticalCursorListsEqual(a, b));
 }
 
+void LayoutObject::setScrollAnchorDisablingStyleChangedOnAncestor()

[ojan, 2016/09/06 23:36:36]

It's not clear to me that this is sufficient for nested scrollers. Can an anchor
node be the anchor for multiple scrolling ancestors?

This probably deserves a test.

[skobes, 2016/09/07 00:23:06]

I think the answer to this is "yes, but that's ok".  ScrollAnchor looks for the
SANACLAP bit on anything in the ancestor path from the anchor node, so setting
it on the inner scroller is sufficient to suppress both scrollers.

[ymalik, 2016/09/07 00:39:57]

Yes, that case is covered, but another question is, can a position change in the
inner scroller move things around on the outer scroller. This can probably
happen in some cases but rare?

For example, say that the inner scroller has an element that goes from static ->
absolute, and is now positioned relative to the outer scroller. Say that this
change impacts the height of the inner scroller (auto height and horizontal
scrolling?) and the user is scrolled past the inner scroller.

Now the anchor node for the outer scroller (somewhere below the inner scroller)
will move, and we won't suppress anchoring.

It's probably more complete to suppress all scrollers on the path to root, but
the rarity of the scenarios where it will help may make this optimization worth
it? 

WDYT Ojan/Steve?

[skobes, 2016/09/07 00:49:31]

My gut feeling is we don't gain anything by suppressing all scrollers to the
root.  There are theoretical ways to break our fix, but that's always the case. 
If we encounter nested-scroller compat issues in the wild we can revisit.

[ymalik, 2016/09/07 18:13:22]

Acknowledged.

+{
+    // Walk up the parent chain and find the first scrolling block to disable
+    // scroll anchoring on.
+    LayoutObject* object = this;
+    Element* viewportDefiningElement = document().viewportDefiningElement();
+    do {
+        object = object->parent();
+        if (LayoutBlock* block = toLayoutBlock(object)) {
+            bool hasScrollableOverflow = block->hasScrollableOverflowX() || block->hasScrollableOverflowY();

[skobes, 2016/09/07 00:02:11]

I don't think we should use this since it depends on layout (presence of
overflow).  Would it work to pick the first block that hasOverflowClip()?

This seems safe because if a block clips overflow but doesn't actually have
overflow, position changes in its descendants still won't affect things outside
it.  (I suppose there are exceptions to this.)

[ymalik, 2016/09/07 00:39:57]

I agree that we shouldn't depend on layout during layout. Will make this change.

[ymalik, 2016/09/07 18:13:22]

Done.

+            if (hasScrollableOverflow || (block->node() == viewportDefiningElement && block->view()->frameView()->isScrollable())) {
+                block->setScrollAnchorDisablingStyleChanged(true);
+                return;
+            }
+        }
+    } while (object);
+}
+
 void LayoutObject::styleDidChange(StyleDifference diff, const ComputedStyle* oldStyle)
 {
     if (s_affectsParentBlock)
@@ -1723,6 +1741,14 @@ void LayoutObject::styleDidChange(StyleDifference diff, const ComputedStyle* old
     if (diff.needsFullLayout()) {
         LayoutCounter::layoutObjectStyleChanged(*this, oldStyle, *m_style);
 
+        // If the in-flow state of an element is changed, disable scroll
+        // anchoring on the containing scroller.
+        if (oldStyle->position() != m_style->position()
+            && ((oldStyle->position() == StaticPosition || m_style->position() == StaticPosition)

[ojan, 2016/09/06 23:36:36]

Why do you need to check for StaticPosition? That's a subset of in-flow
position, right?

[ymalik, 2016/09/07 00:39:57]

For reasons that I don't understand, StaticPosition is not referred to as being
in flow.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/Com...

There's probably a good reason for this?

[ojan, 2016/09/07 00:56:44]

Hmmm...I guess I'm misremembering how this code worked. I think we have a
different set of checks in the layout code.

In either case, I don't think you need all four of these checks. All you need to
check is if oldStyle->hasOutOfFlowPosition() != m_style->hasOutOfFlowPosition().

You just want to know if the thing changes its out of flow position state,
right?

[ymalik, 2016/09/07 18:13:22]

You're totally right. This works.

+            || (oldStyle->hasInFlowPosition() && !m_style->hasInFlowPosition())
+            || (oldStyle->hasOutOfFlowPosition() && !m_style->hasOutOfFlowPosition())))
+            setScrollAnchorDisablingStyleChangedOnAncestor();
+
         // If the object already needs layout, then setNeedsLayout won't do
         // any work. But if the containing block has changed, then we may need
         // to mark the new containing blocks for layout. The change that can