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

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

 /*
  * Copyright (C) 2004, 2005, 2007 Nikolas Zimmermann <zimmermann@kde.org>
  * Copyright (C) 2004, 2005, 2007, 2008, 2009 Rob Buis <buis@kde.org>
  * Copyright (C) 2007 Eric Seidel <eric@webkit.org>
  * Copyright (C) 2009 Google, Inc.
  * Copyright (C) Research In Motion Limited 2011. 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
  * License as published by the Free Software Foundation; either
@@ skipping 10 matching lines @@
  * Boston, MA 02110-1301, USA.
  */
 
 #include "core/layout/svg/LayoutSVGRoot.h"
 
 #include "core/frame/LocalFrame.h"
 #include "core/layout/HitTestResult.h"
 #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"
 #include "core/paint/SVGRootPainter.h"
 #include "core/svg/SVGElement.h"
 #include "core/svg/SVGSVGElement.h"
 #include "core/svg/graphics/SVGImage.h"
 #include "platform/LengthFunctions.h"
 
 namespace blink {
@@ skipping 233 matching lines @@
     LayoutReplaced::insertedIntoTree();
     SVGResourcesCache::clientWasAddedToTree(this, styleRef());
 }
 
 void LayoutSVGRoot::willBeRemovedFromTree()
 {
     SVGResourcesCache::clientWillBeRemovedFromTree(this);
     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()
 {
     SVGSVGElement* svg = toSVGSVGElement(node());
     ASSERT(svg);
     float scale = style()->effectiveZoom();
     FloatPoint translate = svg->currentTranslate();
     LayoutSize borderAndPadding(borderLeft() + paddingLeft(), borderTop() + paddingTop());
     m_localToBorderBoxTransform = svg->viewBoxToViewTransform(contentWidth() / scale, contentHeight() / scale);
@@ skipping 115 matching lines @@
             updateHitTestResult(result, pointInBorderBox);
             if (!result.addNodeToListBasedTestResult(node(), locationInContainer, boundsRect))
                 return true;
         }
     }
 
     return false;
 }
 
 }