[New Multicolumn] mapAbsoluteToLocalPoint() needs to convert to flow thread coordinates.

Source/core/layout/LayoutBoxModelObject.cpp

Index: Source/core/layout/LayoutBoxModelObject.cpp
diff --git a/Source/core/layout/LayoutBoxModelObject.cpp b/Source/core/layout/LayoutBoxModelObject.cpp
index ee6b932bd7a3fddbe5e5b35f97bf8f4f80d70ee9..0448c8790af1dad4cc8fda83f54747bf43d1b4e9 100644
--- a/Source/core/layout/LayoutBoxModelObject.cpp
+++ b/Source/core/layout/LayoutBoxModelObject.cpp
@@ -788,14 +788,21 @@ void LayoutBoxModelObject::mapAbsoluteToLocalPoint(MapCoordinatesFlags mode, Tra
     if (!o)
         return;
 
-    if (o->isLayoutFlowThread())
-        transformState.move(o->columnOffset(LayoutPoint(transformState.mappedPoint())));
-
     o->mapAbsoluteToLocalPoint(mode, transformState);
 
     LayoutSize containerOffset = offsetFromContainer(o, LayoutPoint());
 
-    if (!style()->hasOutOfFlowPosition() && o->hasColumns()) {
+    if (o->isLayoutFlowThread()) {
+        // Descending into a flow thread. Convert to the local coordinate space, i.e. flow thread coordinates.
+        const LayoutFlowThread* flowThread = toLayoutFlowThread(o);
+        LayoutPoint visualPoint = LayoutPoint(transformState.mappedPoint());
+        transformState.move(visualPoint - flowThread->visualPointToFlowThreadPoint(visualPoint));
+        // |containerOffset| is also in visual coordinates. Convert to flow thread coordinates.
+        // TODO(mstensho): Wouldn't it be better add a parameter to instruct offsetFromContainer()

[Julien - ping for review, 2015/03/25 21:41:32]

What's preventing this change now? I was wondering also if we couldn't override
offsetFromContainer() (e.g. in LayoutFlowThreadMultiColumn) and pipe the calls
to the overridden function instead of an extra parameter to the function.

[mstensho (USE GERRIT), 2015/03/25 22:35:05]

offsetFromContainer() currently needs to perform the conversion from flow thread
coordinates to visual coordinates when walking up the tree, like when called
from mapLocalToContainer(), while it's bad to do it when walking down the tree,
which is what we do here.

I would prefer to sit back and think about it, rather than fixing it right away,
as part of this CL. Also: any function that takes more than 3 parameters is
insane, in my book. :) So I'm reluctant to add another parameter if there are
better ways. I'm starting to think that it's wrong to perform this conversion at
all in offsetFromContainer(), since it makes ugly assumptions about what the
caller uses the result for (and those callers that don't want it need to convert
it back). Better have the callers do this on their own, maybe?

[Julien - ping for review, 2015/03/26 15:38:56]

It seems reasonable to let the callers do it. I see that it's definitely not
trivial and better to be pushed into a follow-up change.

+        // to return flowthread coordinates in the first place? We're effectively performing two
+        // conversions here, when in fact none is needed.
+        containerOffset = toLayoutSize(flowThread->visualPointToFlowThreadPoint(toLayoutPoint(containerOffset)));
+    } else if (!style()->hasOutOfFlowPosition() && o->hasColumns()) {
         LayoutBlock* block = toLayoutBlock(o);
         LayoutPoint point(roundedLayoutPoint(transformState.mappedPoint()));
         point -= containerOffset;