Rework mapToVisualRectInAncestorSpace to handle flipped blocks correctly.

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

Index: third_party/WebKit/Source/core/layout/LayoutBox.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutBox.cpp b/third_party/WebKit/Source/core/layout/LayoutBox.cpp
index 4fc4b88d0b30ee3b40066ef8b3237f68d14340cc..9ae99b8f0ad4a618bae43e4b29e791df37d09c61 100644
--- a/third_party/WebKit/Source/core/layout/LayoutBox.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutBox.cpp
@@ -991,15 +991,11 @@ bool LayoutBox::mapScrollingContentsRectToBoxSpace(LayoutRect& rect, ApplyOverfl
         return true;
 
     LayoutSize offset = LayoutSize(-scrolledContentOffset());
-    if (UNLIKELY(hasFlippedBlocksWritingMode()))
-        offset.setWidth(-offset.width());
     rect.move(offset);
 
     if (applyOverflowClip == ApplyNonScrollOverflowClip && scrollsOverflow())
         return true;
 
-    flipForWritingMode(rect);

[chrishtr, 2016/07/09 00:15:57]

Do you know what the code in this method was trying to do before? Does it have
to do with the special coordinate space that
doesn't flip blocks that mysteriously exists?

[wkorman, 2016/07/09 01:16:08]

No, I'm not sure what this was aiming to handle. Do we have test coverage for
IntersectionObserver logic? That's the main area I wonder about. Some of this
was added in http://crrev.com/1817693002 and others have been through rename
patches.

Or, what test can I manually put together to gain confidence re: this method. I
will investigate but if you have thought let me know.

[chrishtr, 2016/07/11 17:35:55]

Yes we have some test coverage for IntersectionObserver. Some of the code and
tests were added for that by Stefan, but 
as we have discovered, were implemented based on an incorrect assumption about
how flipped-blocks works.

Let's just leave the code as you have written it.

-
     LayoutRect clipRect = overflowClipRect(LayoutPoint());
 
     bool doesIntersect;
@@ -1009,8 +1005,6 @@ bool LayoutBox::mapScrollingContentsRectToBoxSpace(LayoutRect& rect, ApplyOverfl
         rect.intersect(clipRect);
         doesIntersect = !rect.isEmpty();
     }
-    if (doesIntersect)
-        flipForWritingMode(rect);
     return doesIntersect;
 }
 
@@ -2043,34 +2037,23 @@ bool LayoutBox::mapToVisualRectInAncestorSpace(const LayoutBoxModelObject* ances
 {
     inflateVisualRectForReflectionAndFilter(rect);
 
-    if (ancestor == this) {
-        // The final rect returned is always in the physical coordinate space of the ancestor.
-        flipForWritingMode(rect);
+    if (ancestor == this)
         return true;
-    }
 
     bool ancestorSkipped;
     bool filterOrReflectionSkipped;
     LayoutObject* container = this->container(ancestor, &ancestorSkipped, &filterOrReflectionSkipped);
+    LayoutBox* localContainingBlock = containingBlock();
+    if (container->isTableRow()) {

[chrishtr, 2016/07/09 00:15:57]

Add a comment explaining what's going on here: skip table row because cells and
rows are in the same coordinate space.
Also DCHECK(isTableCell()) to add clarity and enforce what we think is the
truth.

[wkorman, 2016/07/09 01:16:08]

Done.

+        localContainingBlock = toLayoutBox(container->parent());
+        container = container->parent();
+    }
     if (!container)
         return true;
 
     if (filterOrReflectionSkipped)
         inflateVisualRectForReflectionAndFilterUnderContainer(rect, *container, ancestor);
 
-    // The rect we compute at each step is shifted by our x/y offset in the parent container's coordinate space.
-    // Only when we cross a writing mode boundary will we have to possibly flipForWritingMode (to convert into a more
-    // appropriate offset corner for the enclosing container).
-    if (isWritingModeRoot()) {
-        flipForWritingMode(rect);
-        // Then flip rect currently in physical direction to container's block direction.
-        if (container->styleRef().isFlippedBlocksWritingMode())
-            rect.setX(m_frameRect.width() - rect.maxX());
-    }
-
-    LayoutPoint topLeft = rect.location();
-    topLeft.move(locationOffset());
-
     // We are now in our parent container's coordinate space.  Apply our transform to obtain a bounding box
     // in the parent's coordinate space that encloses us.
     if (hasLayer() && layer()->transform()) {
@@ -2078,9 +2061,10 @@ bool LayoutBox::mapToVisualRectInAncestorSpace(const LayoutBoxModelObject* ances
         // the transform since we don't know the desired subpixel accumulation at this point, and the transform may
         // include a scale.
         rect = LayoutRect(layer()->transform()->mapRect(enclosingIntRect(rect)));
-        topLeft = rect.location();
-        topLeft.move(locationOffset());
     }
+    LayoutPoint topLeft = rect.location();
+    if (!(isInline() && isLayoutInline()))
+        topLeft.moveBy(topLeftLocation(localContainingBlock));
 
     const ComputedStyle& styleToUse = styleRef();
     EPosition position = styleToUse.position();
@@ -2102,7 +2086,7 @@ bool LayoutBox::mapToVisualRectInAncestorSpace(const LayoutBoxModelObject* ances
         return false;
 
     if (ancestorSkipped) {
-        // If the ancestor is below o, then we need to map the rect into ancestor's coordinates.
+        // If the ancestor is below the container, then we need to map the rect into ancestor's coordinates.
         LayoutSize containerOffset = ancestor->offsetFromAncestorContainer(container);
         rect.move(-containerOffset);
         // If the ancestor is fixed, then the rect is already in its coordinates so doesn't need viewport-adjusting.
@@ -4475,12 +4459,12 @@ LayoutPoint LayoutBox::flipForWritingModeForChild(const LayoutBox* child, const
     return LayoutPoint(point.x() + size().width() - child->size().width() - (2 * child->location().x()), point.y());
 }
 
-LayoutPoint LayoutBox::topLeftLocation() const
+LayoutPoint LayoutBox::topLeftLocation(const LayoutBox* container) const
 {

[chrishtr, 2016/07/09 00:15:57]

Just noticed that you can put in an early out:

if (!style()->isFlippedBlocksWritingMode())
  return location()


and avoid computation of containingBlock for 99.99% of page loads.

[wkorman, 2016/07/09 01:16:08]

Wouldn't we need instead:

if (container && !container->style()->isFlippedBlocksWritingMode()) ...

Since we flip based on whether the container box has flipped blocks writing
mode, not 'this'.

So we still need to look up the containing block if it's not passed in. It could
still be worthwhile, let me know if you think we should do it. It would still
speed up the non-inline case for mapToVisualRectInAncestorSpace?

[chrishtr, 2016/07/11 17:35:56]

Oh right. Yes seems worthwhile to do it just if container was passed in.

-    LayoutBlock* containerBlock = containingBlock();
-    if (!containerBlock || containerBlock == this)
+    const LayoutBox* containerBox = container ? container : containingBlock();
+    if (!containerBox || containerBox == this)
         return location();
-    return containerBlock->flipForWritingModeForChild(this, location());
+    return containerBox->flipForWritingModeForChild(this, location());
 }
 
 bool LayoutBox::hasRelativeLogicalWidth() const