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

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

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2000 Dirk Mueller (mueller@kde.org)
  *           (C) 2004 Allan Sandfeld Jensen (kde@carewolf.com)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2009 Google Inc. All rights reserved.
  * Copyright (C) 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
  *
  * This library is free software; you can redistribute it and/or
@@ skipping 1694 matching lines @@
 {
     ASSERT(a->cursors() != b->cursors());
     return a->cursors() && b->cursors() && *a->cursors() == *b->cursors();
 }
 
 static inline bool areCursorsEqual(const ComputedStyle* a, const ComputedStyle* b)
 {
     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)
         handleDynamicFloatPositionChange(this);
 
     if (!m_parent)
         return;
 
     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
         // directly affect the containing block of this object is a change to
         // the position style.
         if (needsLayout() && oldStyle->position() != m_style->position())
             markContainerChainForLayout();
 
         // Ditto.
         if (needsOverflowRecalcAfterStyleChange() && oldStyle->position() != m_style->position())
@@ skipping 1497 matching lines @@
         const blink::LayoutObject* root = object1;
         while (root->parent())
             root = root->parent();
         root->showLayoutTreeAndMark(object1, "*", object2, "-", 0);
     } else {
         WTFLogAlways("%s", "Cannot showLayoutTree. Root is (nil)");
     }
 }
 
 #endif