Fix geometry mapping issues for float under inline

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/)
@@ skipping 594 matching lines @@
     if (current->hasLayer())
       return toLayoutBoxModelObject(current)->layer();
   }
   // TODO(crbug.com/365897): we should get rid of detached layout subtrees, at
   // which point this code should not be reached.
   return nullptr;
 }
 
 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();
   }
   // TODO(crbug.com/365897): we should get rid of detached layout subtrees, at
   // which point this code should not be reached.
   return nullptr;
 }
 
 bool LayoutObject::scrollRectToVisible(const LayoutRect& rect,
@@ skipping 254 matching lines @@
       *ancestorSkipped = true;
 
     if (filterSkipped && object->hasFilterInducingProperty())
       *filterSkipped = true;
   }
 
   ASSERT(!object || !object->isAnonymousBlock());
   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 {
   Vector<FloatQuad> quads;
   absoluteQuads(quads);
 
   size_t n = quads.size();
   if (n == 0)
     return FloatRect();
 
@@ skipping 70 matching lines @@
   for (LayoutObject* current = slowFirstChild(); current;
        current = current->nextSibling())
     current->addAbsoluteRectForLayer(result);
   return result;
 }
 
 void LayoutObject::paint(const PaintInfo&, const LayoutPoint&) const {}
 
 const LayoutBoxModelObject& LayoutObject::containerForPaintInvalidation()
     const {
-  RELEASE_ASSERT(isRooted());
+  CHECK(isRooted());
 
   if (const LayoutBoxModelObject* paintInvalidationContainer =
           enclosingCompositedContainer())
     return *paintInvalidationContainer;
 
   // 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;
 }
 
 const LayoutBoxModelObject* LayoutObject::enclosingCompositedContainer() const {
   LayoutBoxModelObject* container = nullptr;
   // FIXME: CompositingState is not necessarily up to date for many callers of
   // this function.
   DisableCompositingQueryAsserts disabler;
 
   if (PaintLayer* paintingLayer = this->paintingLayer()) {
@@ skipping 1463 matching lines @@
            walker && walker != multicolContainer; walker = walker->parent()) {
         if (ancestorSkipped && walker == ancestor)
           *ancestorSkipped = true;
         if (filterSkipped && walker->hasFilterInducingProperty())
           *filterSkipped = true;
       }
     }
     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;
 }
 
 inline LayoutObject* LayoutObject::paintInvalidationParent() const {
   if (isLayoutView())
     return LayoutAPIShim::layoutObjectFrom(frame()->ownerLayoutItem());
   if (isColumnSpanAll())
     return spannerPlaceholder();
   return parent();
 }
@@ skipping 979 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