Fix invalid selection produced when dragging mouse outside the SVG text element

third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp

Index: third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp
diff --git a/third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp b/third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp
index fbe5909f7a0917a37e843ac9a56c9a385ae59861..1f020a1611480db55b8681fd97a08d9b5134840d 100644
--- a/third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp
+++ b/third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp
@@ -28,6 +28,7 @@
 #include "core/layout/LayoutAnalyzer.h"
 #include "core/layout/LayoutPart.h"
 #include "core/layout/LayoutView.h"
+#include "core/layout/svg/LayoutSVGText.h"
 #include "core/layout/svg/SVGLayoutSupport.h"
 #include "core/layout/svg/SVGResourcesCache.h"
 #include "core/paint/PaintLayer.h"
@@ -281,6 +282,63 @@ void LayoutSVGRoot::willBeRemovedFromTree()
     LayoutReplaced::willBeRemovedFromTree();
 }
 
+PositionWithAffinity LayoutSVGRoot::positionForPoint(const LayoutPoint& point)
+{
+    LayoutObject* closestDescendant = nullptr;

[fs, 2016/01/12 17:40:17]

Since this is always LayoutSVGText it could be that type instead.

+    float closestDistance = std::numeric_limits<float>::max();
+    FloatPoint absolutePoint(point);
+    AffineTransform transformToParent;
+    transformToParent.makeIdentity();

[fs, 2016/01/12 17:40:17]

The AffineTransform constructor does this. 

[fs, 2016/01/12 17:40:17]

The AffineTransform constructor does this. 

+    for (LayoutObject* descendant = firstChild(); descendant; descendant = descendant->nextInPreOrder(this)) {

[fs, 2016/01/12 17:40:17]

This will walk the entire SVG fragment. I think we should at least attempt to do
some high-level pruning by doing breadth-first (rather than depth-first like
this). That would be possible to write in a data-recursive manner, but I suspect
that just adding a helper to SVGLayoutSupport (similar to how for instance
layoutChildren is done) and make it code-recursive will be easier to deal with.

I think LayoutBox::positionForPoint could serve as inspiration for approx how
that will look. (How/if overlapping <text>s should be handled can be dealt with
later - looks like the current "solution" would pick the first one in
pre-order.)

[pdr., 2016/01/12 23:52:47]

+1, LayoutBox::positionForPoint is a good starting point.

If we do this, can you add FloatRect::distanceTo(FloatPoint) instead of
implementing another copy of
http://wiki.unity3d.com/index.php/Distance_from_a_point_to_a_rectangle? Maybe
even as a patch before this patch with a simple unit test?

+        if (descendant->isSVGText()) {
+            FloatRect boundaries = toLayoutSVGText(descendant)->objectBoundingBox();
+            FloatPoint closestPoint;
+            AffineTransform transform;
+            transform.makeIdentity();

[fs, 2016/01/12 17:40:17]

Done by the constructor.

+
+            LayoutObject* layoutObject = descendant;
+            while (layoutObject) {
+                layoutObject = layoutObject->parent();
+                if (layoutObject->isSVGRoot())
+                    break;
+                transform = layoutObject->localToParentTransform() * transform;
+            }
+            transform = m_localToBorderBoxTransform * transform;
+
+            boundaries = transform.mapRect(boundaries);

[fs, 2016/01/12 17:40:17]

|transform| here does not include any transform from |descendant|, and hence
this maps from the user space of the ancestor of |descendant| rather than the
user space of |descendant| itself. This in turn mean that the comparisons below
are incorrect (compares points in different coordinate spaces.)

Also, this will compare against the transformed bounding box of the <text> which
means the distance can be smaller than it is in reality. Consider for example a
<text> that is rotated 45 degrees. It'd be preferable to either use a quad or
transform the query-point to the <text>s user space.

+            closestPoint.setX(std::max(std::min(absolutePoint.x(), boundaries.maxX()), boundaries.x()));

[fs, 2016/01/12 17:40:17]

Could also use clampTo(...) here instead of max(min(..., ...), ...).

+            closestPoint.setY(std::max(std::min(absolutePoint.y(), boundaries.maxY()), boundaries.y()));
+            float distance = (absolutePoint - closestPoint).diagonalLengthSquared();
+
+            if (distance < closestDistance) {
+                closestDistance = distance;
+                closestDescendant = descendant;
+                transformToParent = transform;
+            }
+        }
+    }
+
+    if (!closestDescendant)
+        return createPositionWithAffinity(0);

[fs, 2016/01/12 17:40:17]

There are most likely cases where we should be calling
LayoutReplaced::positionForPoint - when outside of/clipped by the bbox of the
root etc.

+
+    AffineTransform transform;
+    transform.makeIdentity();
+    if (toLayoutSVGText(closestDescendant)->location().x())
+        transform.setE(toLayoutSVGText(closestDescendant)->location().x());
+    if (toLayoutSVGText(closestDescendant)->location().y())
+        transform.setF(toLayoutSVGText(closestDescendant)->location().y());
+    transformToParent = transformToParent * closestDescendant->localToParentTransform() * transform;

[fs, 2016/01/12 17:40:17]

This reinforces the comment above. Also, location() shouldn't be needed here.

+
+    absolutePoint = transformToParent.inverse().mapPoint(absolutePoint);
+
+    if (absolutePoint.x() < 0)

[fs, 2016/01/12 17:40:17]

Why?

+        absolutePoint.setX(0);
+    if (absolutePoint.y() < 0)
+        absolutePoint.setY(0);
+
+    return closestDescendant->positionForPoint(LayoutPoint(absolutePoint.x(), absolutePoint.y()));
+}
+
 // LayoutBox methods will expect coordinates w/o any transforms in coordinates
 // relative to our borderBox origin.  This method gives us exactly that.
 void LayoutSVGRoot::buildLocalToBorderBoxTransform()