Only include position once in foreign object client rects

SVG's foreign object coordinate spaces were recently updated [1] to
clarify SVG coordinates vs HTML coordinates. SVG coordinates are
equal to HTML coordinates plus location (x and y). LayoutSVGBlock's
MapLocalToAncestor takes HTML coordinates and offsets by location.

Element::ClientQuads passed ObjectBoundingBox, which is in SVG coordinates
and includes location, to MapLocalToAncestor which would apply the
location offset twice. This patch updates Element::ClientQuads to use
AbsoluteQuads which correctly calculates the foreign object bounds.

Two TODOs have been added:
1) Element::ClientQuads and Element::BoundsInViewport are very similar and
should be unified.
2) It's not clear whether stroke should be included in Element::ClientQuads.
If it should be, SVG special-cases can be removed from Element::ClientQuads.
A spec question has been filed: https://github.com/w3c/svgwg/issues/339.

[1] https://chromium.googlesource.com/chromium/src/+/09a6cd640fb5bf07aa06477362229c2e6969a718

Bug: 741615
Change-Id: Ibec65966e8661c4c2390284708ef53b5d0c1d11a
Reviewed-on: https://chromium-review.googlesource.com/597487
Commit-Queue: Philip Rogers <pdr@chromium.org>
Reviewed-by: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#491875}

third_party/WebKit/Source/core/dom/Element.cpp

@@ -1150,8 +1150,17 @@ IntRect Element::BoundsInViewport() const {
     return IntRect();
 
   Vector<FloatQuad> quads;
-  if (IsSVGElement() && GetLayoutObject()) {
+
+  // TODO(pdr): Unify the quad/bounds code with Element::ClientQuads.
+
+  // Foreign objects need to convert between SVG and HTML coordinate spaces and
+  // cannot use LocalToAbsoluteQuad directly with ObjectBoundingBox which is
+  // SVG coordinates and not HTML coordinates. Instead, use the AbsoluteQuads
+  // codepath below.
+  if (IsSVGElement() && GetLayoutObject() &&
+      !GetLayoutObject()->IsSVGForeignObject()) {
     // Get the bounding rectangle from the SVG model.
+    // TODO(pdr): This should include stroke.
     if (ToSVGElement(this)->IsSVGGraphicsElement())
       quads.push_back(GetLayoutObject()->LocalToAbsoluteQuad(
           GetLayoutObject()->ObjectBoundingBox()));
@@ -1192,8 +1201,16 @@ void Element::ClientQuads(Vector<FloatQuad>& quads) {
   if (!element_layout_object)
     return;
 
-  if (IsSVGElement() && !element_layout_object->IsSVGRoot()) {
+  // Foreign objects need to convert between SVG and HTML coordinate spaces and
+  // cannot use LocalToAbsoluteQuad directly with ObjectBoundingBox which is
+  // SVG coordinates and not HTML coordinates. Instead, use the AbsoluteQuads
+  // codepath below.
+  if (IsSVGElement() && !element_layout_object->IsSVGRoot() &&
+      !element_layout_object->IsSVGForeignObject()) {
     // Get the bounding rectangle from the SVG model.
+    // TODO(pdr): ObjectBoundingBox does not include stroke and the spec is not
+    // clear (see: https://github.com/w3c/svgwg/issues/339, crbug.com/529734).
+    // If stroke is desired, we can update this to use AbsoluteQuads, below.
     if (ToSVGElement(this)->IsSVGGraphicsElement())
       quads.push_back(element_layout_object->LocalToAbsoluteQuad(
           element_layout_object->ObjectBoundingBox()));

third_party/WebKit/Source/core/dom/ElementTest.cpp

@@ -248,4 +248,90 @@ TEST_F(ElementTest, GetElementsByClassNameCrash) {
   // The test passes if no crash happens.
 }
 
+TEST_F(ElementTest, GetBoundingClientRectForSVG) {
+  Document& document = GetDocument();
+  SetBodyContent(
+      "<style>body { margin: 0 }</style>"
+      "<svg width='500' height='500'>"
+      "  <rect id='rect' x='10' y='100' width='100' height='71'/>"
+      "  <rect id='stroke' x='10' y='100' width='100' height='71' "
+      "      stroke-width='7'/>"
+      "  <rect id='stroke_transformed' x='10' y='100' width='100' height='71' "
+      "      stroke-width='7' transform='translate(3, 5)'/>"
+      "  <foreignObject id='foreign' x='10' y='100' width='100' height='71'/>"
+      "  <foreignObject id='foreign_transformed' transform='translate(3, 5)' "
+      "      x='10' y='100' width='100' height='71'/>"
+      "  <svg id='svg' x='10' y='100'>"
+      "    <rect width='100' height='71'/>"
+      "  </svg>"
+      "  <svg id='svg_stroke' x='10' y='100'>"
+      "    <rect width='100' height='71' stroke-width='7'/>"
+      "  </svg>"
+      "</svg>");
+
+  Element* rect = document.getElementById("rect");
+  DOMRect* rect_bounding_client_rect = rect->getBoundingClientRect();
+  EXPECT_EQ(10, rect_bounding_client_rect->left());
+  EXPECT_EQ(100, rect_bounding_client_rect->top());
+  EXPECT_EQ(100, rect_bounding_client_rect->width());
+  EXPECT_EQ(71, rect_bounding_client_rect->height());
+  EXPECT_EQ(IntRect(10, 100, 100, 71), rect->BoundsInViewport());
+
+  // TODO(pdr): Should we should be excluding the stroke (here, and below)?
+  // See: https://github.com/w3c/svgwg/issues/339 and Element::ClientQuads.
+  Element* stroke = document.getElementById("stroke");
+  DOMRect* stroke_bounding_client_rect = stroke->getBoundingClientRect();
+  EXPECT_EQ(10, stroke_bounding_client_rect->left());
+  EXPECT_EQ(100, stroke_bounding_client_rect->top());
+  EXPECT_EQ(100, stroke_bounding_client_rect->width());
+  EXPECT_EQ(71, stroke_bounding_client_rect->height());
+  // TODO(pdr): BoundsInViewport is not web exposed and should include stroke.
+  EXPECT_EQ(IntRect(10, 100, 100, 71), stroke->BoundsInViewport());
+
+  Element* stroke_transformed = document.getElementById("stroke_transformed");
+  DOMRect* stroke_transformedbounding_client_rect =
+      stroke_transformed->getBoundingClientRect();
+  EXPECT_EQ(13, stroke_transformedbounding_client_rect->left());
+  EXPECT_EQ(105, stroke_transformedbounding_client_rect->top());
+  EXPECT_EQ(100, stroke_transformedbounding_client_rect->width());
+  EXPECT_EQ(71, stroke_transformedbounding_client_rect->height());
+  // TODO(pdr): BoundsInViewport is not web exposed and should include stroke.
+  EXPECT_EQ(IntRect(13, 105, 100, 71), stroke_transformed->BoundsInViewport());
+
+  Element* foreign = document.getElementById("foreign");
+  DOMRect* foreign_bounding_client_rect = foreign->getBoundingClientRect();
+  EXPECT_EQ(10, foreign_bounding_client_rect->left());
+  EXPECT_EQ(100, foreign_bounding_client_rect->top());
+  EXPECT_EQ(100, foreign_bounding_client_rect->width());
+  EXPECT_EQ(71, foreign_bounding_client_rect->height());
+  EXPECT_EQ(IntRect(10, 100, 100, 71), foreign->BoundsInViewport());
+
+  Element* foreign_transformed = document.getElementById("foreign_transformed");
+  DOMRect* foreign_transformed_bounding_client_rect =
+      foreign_transformed->getBoundingClientRect();
+  EXPECT_EQ(13, foreign_transformed_bounding_client_rect->left());
+  EXPECT_EQ(105, foreign_transformed_bounding_client_rect->top());
+  EXPECT_EQ(100, foreign_transformed_bounding_client_rect->width());
+  EXPECT_EQ(71, foreign_transformed_bounding_client_rect->height());
+  EXPECT_EQ(IntRect(13, 105, 100, 71), foreign_transformed->BoundsInViewport());
+
+  Element* svg = document.getElementById("svg");
+  DOMRect* svg_bounding_client_rect = svg->getBoundingClientRect();
+  EXPECT_EQ(10, svg_bounding_client_rect->left());
+  EXPECT_EQ(100, svg_bounding_client_rect->top());
+  EXPECT_EQ(100, svg_bounding_client_rect->width());
+  EXPECT_EQ(71, svg_bounding_client_rect->height());
+  EXPECT_EQ(IntRect(10, 100, 100, 71), svg->BoundsInViewport());
+
+  Element* svg_stroke = document.getElementById("svg_stroke");
+  DOMRect* svg_stroke_bounding_client_rect =
+      svg_stroke->getBoundingClientRect();
+  EXPECT_EQ(10, svg_stroke_bounding_client_rect->left());
+  EXPECT_EQ(100, svg_stroke_bounding_client_rect->top());
+  EXPECT_EQ(100, svg_stroke_bounding_client_rect->width());
+  EXPECT_EQ(71, svg_stroke_bounding_client_rect->height());
+  // TODO(pdr): BoundsInViewport is not web exposed and should include stroke.
+  EXPECT_EQ(IntRect(10, 100, 100, 71), svg_stroke->BoundsInViewport());
+}
+
 }  // namespace blink