Fix geometry mapping issues for float under inline

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 4bb1a9269d1804bc20db1ce51fa2ea7f888a0d57..4af33f6fce1caf8b5a702cdad435e6b1c617937b 100644
--- a/third_party/WebKit/Source/core/layout/LayoutObject.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutObject.cpp
@@ -612,7 +612,11 @@ PaintLayer* LayoutObject::enclosingLayer() const {
 
 PaintLayer* LayoutObject::paintingLayer() const {
   for (const LayoutObject* current = this; current;
-       current = current->paintInvalidationParent()) {
+       // Use containingBlock instead of paintInvalidationParent for floating
+       // object to omit any self-painting layers of inline objects that don't
+       // paint the floating object.
+       current = current->isFloating() ? current->containingBlock()
+                                       : current->paintInvalidationParent()) {
     if (current->hasLayer() &&
         toLayoutBoxModelObject(current)->layer()->isSelfPaintingLayer())
       return toLayoutBoxModelObject(current)->layer();
@@ -887,53 +891,69 @@ LayoutBlock* LayoutObject::containerForFixedPosition(
   return toLayoutBlock(object);
 }
 
-LayoutBlock* LayoutObject::containingBlockForAbsolutePosition() const {
-  LayoutObject* o = containerForAbsolutePosition();
+LayoutBlock* LayoutObject::containingBlockForAbsolutePosition(
+    const LayoutBoxModelObject* ancestor,
+    bool* ancestorSkipped,
+    bool* filterSkipped) const {
+  LayoutObject* object =
+      containerForAbsolutePosition(ancestor, ancestorSkipped, filterSkipped);
 
   // For relpositioned inlines, we return the nearest non-anonymous enclosing
   // block. We don't try to return the inline itself. This allows us to avoid
   // having a positioned objects list in all LayoutInlines and lets us return a
   // strongly-typed LayoutBlock* result from this method. The container() method
   // can actually be used to obtain the inline directly.
-  if (o && o->isInline() && !o->isAtomicInlineLevel()) {
-    ASSERT(o->style()->hasInFlowPosition());
-    o = o->containingBlock();
+  if (object && object->isInline() && !object->isAtomicInlineLevel()) {
+    DCHECK(object->style()->hasInFlowPosition());
+    object = object->containingBlock(ancestor, ancestorSkipped, filterSkipped);
   }
 
-  if (o && !o->isLayoutBlock())
-    o = o->containingBlock();
+  if (object && !object->isLayoutBlock())
+    object = object->containingBlock(ancestor, ancestorSkipped, filterSkipped);
 
-  while (o && o->isAnonymousBlock())
-    o = o->containingBlock();
+  while (object && object->isAnonymousBlock())
+    object = object->containingBlock(ancestor, ancestorSkipped, filterSkipped);
 
-  if (!o || !o->isLayoutBlock())
+  if (!object || !object->isLayoutBlock())
     return nullptr;  // This can still happen in case of an orphaned tree
 
-  return toLayoutBlock(o);
+  return toLayoutBlock(object);
 }
 
-LayoutBlock* LayoutObject::containingBlock() const {
-  LayoutObject* o = parent();
-  if (!o && isLayoutScrollbarPart())
-    o = toLayoutScrollbarPart(this)->layoutObjectOwningScrollbar();
+LayoutBlock* LayoutObject::containingBlock(const LayoutBoxModelObject* ancestor,
+                                           bool* ancestorSkipped,
+                                           bool* filterSkipped) const {
+  LayoutObject* object = parent();
+  if (!object && isLayoutScrollbarPart())
+    object = toLayoutScrollbarPart(this)->layoutObjectOwningScrollbar();
   if (!isTextOrSVGChild()) {
-    if (m_style->position() == FixedPosition)
-      return containerForFixedPosition();
-    if (m_style->position() == AbsolutePosition)
-      return containingBlockForAbsolutePosition();
+    if (m_style->position() == FixedPosition) {
+      return containerForFixedPosition(ancestor, ancestorSkipped,
+                                       filterSkipped);
+    }
+    if (m_style->position() == AbsolutePosition) {
+      return containingBlockForAbsolutePosition(ancestor, ancestorSkipped,
+                                                filterSkipped);
+    }
   }
   if (isColumnSpanAll()) {
-    o = spannerPlaceholder()->containingBlock();
+    object = spannerPlaceholder()->containingBlock();
   } else {
-    while (o && ((o->isInline() && !o->isAtomicInlineLevel()) ||
-                 !o->isLayoutBlock()))
-      o = o->parent();
+    while (object && ((object->isInline() && !object->isAtomicInlineLevel()) ||
+                      !object->isLayoutBlock())) {
+      if (ancestorSkipped && object == ancestor)
+        *ancestorSkipped = true;
+
+      if (filterSkipped && object->hasFilterInducingProperty())
+        *filterSkipped = true;
+      object = object->parent();
+    }
   }
 
-  if (!o || !o->isLayoutBlock())
+  if (!object || !object->isLayoutBlock())
     return nullptr;  // This can still happen in case of an orphaned tree
 
-  return toLayoutBlock(o);
+  return toLayoutBlock(object);
 }
 
 FloatRect LayoutObject::absoluteBoundingBoxFloatRect() const {
@@ -1024,7 +1044,7 @@ void LayoutObject::paint(const PaintInfo&, const LayoutPoint&) const {}
 
 const LayoutBoxModelObject& LayoutObject::containerForPaintInvalidation()
     const {
-  RELEASE_ASSERT(isRooted());
+  CHECK(isRooted());
 
   if (const LayoutBoxModelObject* paintInvalidationContainer =
           enclosingCompositedContainer())
@@ -1033,12 +1053,11 @@ const LayoutBoxModelObject& LayoutObject::containerForPaintInvalidation()
   // If the current frame is not composited, we send just return the main
   // frame's LayoutView so that we generate invalidations on the window.
   const LayoutView* layoutView = view();
-  while (
-      LayoutAPIShim::layoutObjectFrom(layoutView->frame()->ownerLayoutItem()))
-    layoutView =
-        LayoutAPIShim::layoutObjectFrom(layoutView->frame()->ownerLayoutItem())
-            ->view();
-  ASSERT(layoutView);
+  while (const LayoutObject* ownerObject = LayoutAPIShim::constLayoutObjectFrom(
+             layoutView->frame()->ownerLayoutItem()))
+    layoutView = ownerObject->view();
+
+  DCHECK(layoutView);
   return *layoutView;
 }
 
@@ -2522,6 +2541,9 @@ LayoutObject* LayoutObject::container(const LayoutBoxModelObject* ancestor,
     return multicolContainer;
   }
 
+  if (isFloating())

[chrishtr, 2016/12/16 18:50:20]

I'm concerned about side-effects of modifying container() like this.
Are floats just broken in this code in general? How are there not more
consequences?

[Xianzhu, 2016/12/16 19:08:49]

Paint invalidation of float-under-inline was broken in general because of wrong
container() and paintingLayer(). We computed wrong visual rect for the float
because the containing inline was treated as the container so added wrong paint
offsets; and invalidated wrong layer if the inline has a layer. We didn't have
test coverage for the case.

[wkorman, 2016/12/16 19:40:31]

Are there any ramifications to using containingBlock() from within container()
given that it is explicitly defined as differing in three ways, from
LayoutObject.h docs:

  // (1) It can be used on orphaned subtrees, i.e., it can be called safely
  //     even when the object is not part of the primary document subtree yet.
  // (2) For normal flow elements, it just returns the parent.
  // (3) For absolute positioned elements, it will return a relative
  //     positioned inline. containingBlock() simply skips relpositioned inlines
  //     and lets an enclosing block handle the layout of the positioned object.
  //     This does mean that computePositionedLogicalWidth and
  //     computePositionedLogicalHeight have to use container().

(1) seems not impactful for this issue.
(2) seems like it could be an issue in certain cases, but maybe it's ok because
it would just lead to skipping things we want skipped for the float case anyway?
Are there test cases we can add to validate that this float case you are looking
to fix works for both out of flow and normal flow?
(3) seems not impactful for this issue.

[Xianzhu, 2016/12/16 20:06:42]

According to https://www.w3.org/TR/CSS2/visuren.html#positioning-scheme,
floating objects are out of flow, so none of the three points cover floating
objects, and making container() to be the same as containingBlock() for floating
objects doesn't conflict with these points.

What do you mean "works for both out of flow and normal flow"? Can you give some
simple examples?

[wkorman, 2016/12/16 21:50:40]

I was thinking the same as the tests you've already added in this change, but
with add'l permutations of the ancestor state (abspos, relpos, composited vs.
not, inline vs box). But this change should only affect the float on which
container() is called, so ancestor state is not relevant unless we are uncertain
re: our coverage of some of those other states.

Am ok proceeding as is.

+    return containingBlock(ancestor, ancestorSkipped, filterSkipped);
+
   return o;
 }