Rework mapToVisualRectInAncestorSpace to handle flipped blocks correctly.

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

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2005 Allan Sandfeld Jensen (kde@carewolf.com)
  *           (C) 2005, 2006 Samuel Weinig (sam.weinig@gmail.com)
  * Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010 Apple Inc. All rights reserved.
  * Copyright (C) 2013 Adobe Systems Incorporated. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ skipping 973 matching lines @@
         result.expand(-verticalScrollbarWidth(), 0);
     return result;
 }
 
 bool LayoutBox::mapScrollingContentsRectToBoxSpace(LayoutRect& rect, ApplyOverflowClipFlag applyOverflowClip, VisualRectFlags visualRectFlags) const
 {
     if (!hasOverflowClip())
         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;
     if (visualRectFlags & EdgeInclusive) {
         doesIntersect = rect.inclusiveIntersect(clipRect);
     } else {
         rect.intersect(clipRect);
         doesIntersect = !rect.isEmpty();
     }
-    if (doesIntersect)
-        flipForWritingMode(rect);
     return doesIntersect;
 }
 
 void LayoutBox::computeIntrinsicLogicalWidths(LayoutUnit& minLogicalWidth, LayoutUnit& maxLogicalWidth) const
 {
     minLogicalWidth = minPreferredLogicalWidth() - borderAndPaddingLogicalWidth();
     maxLogicalWidth = maxPreferredLogicalWidth() - borderAndPaddingLogicalWidth();
 }
 
 LayoutUnit LayoutBox::minPreferredLogicalWidth() const
@@ skipping 1012 matching lines @@
         if (parent == ancestorToStopAt)
             break;
     }
     rect.move(-offsetFromContainer);
 }
 
 bool LayoutBox::mapToVisualRectInAncestorSpace(const LayoutBoxModelObject* ancestor, LayoutRect& rect, VisualRectFlags visualRectFlags) const
 {
     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()) {
         // Use enclosingIntRect because we cannot properly compute pixel snapping for painted elements within
         // 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();
     if (position == AbsolutePosition && container->isInFlowPositioned() && container->isLayoutInline()) {
         topLeft += toLayoutInline(container)->offsetForInFlowPositionedInline(*this);
     } else if (styleToUse.hasInFlowPosition() && layer()) {
         // Apply the relative position offset when invalidating a rectangle.  The layer
         // is translated, but the layout box isn't, so we need to do this to get the
         // right dirty rect.  Since this is called from LayoutObject::setStyle, the relative position
         // flag on the LayoutObject has been cleared, so use the one on the style().
         topLeft += layer()->offsetForInFlowPosition();
     }
 
     // FIXME: We ignore the lightweight clipping rect that controls use, since if |o| is in mid-layout,
     // its controlClipRect will be wrong. For overflow clip we use the values cached by the layer.
     rect.setLocation(topLeft);
 
     if (container->isBox() && !toLayoutBox(container)->mapScrollingContentsRectToBoxSpace(rect, container == ancestor ? ApplyNonScrollOverflowClip : ApplyOverflowClip, visualRectFlags))
         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.
         if (ancestor->style()->position() != FixedPosition && container->isLayoutView() && position == FixedPosition)
             toLayoutView(container)->adjustOffsetForFixedPosition(rect);
         return true;
     }
 
     if (container->isLayoutView())
         return toLayoutView(container)->mapToVisualRectInAncestorSpace(ancestor, rect, position == FixedPosition ? IsFixed : 0, visualRectFlags);
@@ skipping 2352 matching lines @@
 LayoutPoint LayoutBox::flipForWritingModeForChild(const LayoutBox* child, const LayoutPoint& point) const
 {
     if (!style()->isFlippedBlocksWritingMode())
         return point;
 
     // The child is going to add in its x(), so we have to make sure it ends up in
     // the right place.
     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
 {
     return style()->logicalWidth().hasPercent()
         || style()->logicalMinWidth().hasPercent()
         || style()->logicalMaxWidth().hasPercent();
 }
 
 bool LayoutBox::hasRelativeLogicalHeight() const
@@ skipping 441 matching lines @@
         m_rareData->m_snapAreas->remove(&snapArea);
     }
 }
 
 SnapAreaSet* LayoutBox::snapAreas() const
 {
     return m_rareData ? m_rareData->m_snapAreas.get() : nullptr;
 }
 
 } // namespace blink