Fix Ruby visual rects in flipped blocks writing mode.

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 78eca2d86f8337920eaf26036524d39d37768599..e177e4a614a95279f99087f66a8255456585e263 100644
--- a/third_party/WebKit/Source/core/layout/LayoutBox.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutBox.cpp
@@ -2096,7 +2096,9 @@ bool LayoutBox::mapToVisualRectInAncestorSpace(const LayoutBoxModelObject* ances
         rect = LayoutRect(layer()->transform()->mapRect(enclosingIntRect(rect)));
     }
     LayoutPoint topLeft = rect.location();
-    if (container->isBox()) {
+    // LayoutRubyRun is the (anonymous) block container for LayoutRuby{Base,Text}, all of which are boxes.
+    // We need to only flip once, so we leave that to LayoutRubyRun.
+    if (container->isBox() && !container->isRubyRun()) {

[chrishtr, 2016/07/21 17:24:54]

!container->isInline() also makes this test pass, and makes more sense than
special-casing isRubyRun, especially w.r.t the logic in
PaintLayer::updateLayerPosition.

Could you try that?

[wkorman, 2016/07/26 00:38:14]

I looked into how PaintLayer::updateLayerPosition ends up working with Ruby, and
I believe it's due to this bit:

                localPoint.moveBy(toLayoutBox(curr)->topLeftLocation());

here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pai...

Note no params are passed to topLeftLocation(), and so this is flipping within
containingBlock(), not container(), which I believe is necessary to get the
correct top-left location between LayoutRubyRun and its containing block, the
DIV.

I am essentially in the process of re-convincing myself that patchset 1 is not
unreasonable.

I spent more time fiddling with !container->isInline() today, I haven't yet
found solution that allows both the aforementioned
imported/csswg-test/css-writing-modes-3/* tests to pass along with the Ruby
tests, but perhaps we can find a way. I think we'll end up having to have some
conditional to identify the Ruby LayoutTree structure and do things differently,
at which point it's different but not IMO better than patchset 1.

We've rewritten mapToVisualRectInAncestorSpace() to use container, rather than
containingBlock(), in general. I think that's the delta between our logic and
PaintLayer logic, and it particularly affects Ruby due to the "LayoutRuby
(inline)" interspersed between the LayoutRubyRun and its containing DIV. We need
to reconcile that somehow, patchset 1 is one way.

[chrishtr, 2016/07/26 18:02:29]

Please add a TODO(wkorman) that it's not clear why specializing on isRubyRun
makes sense.

         topLeft.moveBy(topLeftLocation(toLayoutBox(container)));
         // If the row is the ancestor, however, add its offset back in. In effect, this passes from the joint <td> / <tr>
         // coordinate space to the parent space, then back to <tr> / <td>.