commit 3df6c20a050ae8bba8810401cb33e6ec0a8a98e1
author Tyler Denniston <tdenniston@google.com> Thu Jan 14 15:37:38 2021 -0500
committer Skia Commit-Bot <skia-commit-bot@chromium.org> Thu Jan 14 21:35:58 2021 +0000
tree 440ebc298243a192f75852466de1597038ca25f8
parent a738fb5d585ab18ad93ee16cfc97e00dc7474f13

[svg] Fix <use> bounds calculation

Previously we were using fX and fY as the top left of the returned
bounds. We need instead to offset the referenced node's bounds by fX,
fY.

While I was here I updated the attribute parsing to the new form and
changed the type of fHref from SkSVGString to SkSVGIRI.

Change-Id: I4bfb91bca62e47f5dabfbb4aad48cbb301a7ea36
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/354118
Reviewed-by: Florin Malita <fmalita@chromium.org>
Commit-Queue: Tyler Denniston <tdenniston@google.com>

modules/svg/include/SkSVGUse.h

diff --git a/modules/svg/include/SkSVGUse.h b/modules/svg/include/SkSVGUse.h
index 6c524e3..93c2d20 100644
--- a/modules/svg/include/SkSVGUse.h
+++ b/modules/svg/include/SkSVGUse.h
@@ -21,13 +21,11 @@
 
     void appendChild(sk_sp<SkSVGNode>) override;
 
-    void setHref(const SkSVGStringType&);
-    void setX(const SkSVGLength&);
-    void setY(const SkSVGLength&);
+    SVG_ATTR(X   , SkSVGLength, SkSVGLength(0))
+    SVG_ATTR(Y   , SkSVGLength, SkSVGLength(0))
+    SVG_ATTR(Href, SkSVGIRI   , SkSVGIRI())
 
 protected:
-    void onSetAttribute(SkSVGAttribute, const SkSVGValue&) override;
-
     bool onPrepareToRender(SkSVGRenderContext*) const override;
     void onRender(const SkSVGRenderContext&) const override;
     SkPath onAsPath(const SkSVGRenderContext&) const override;
@@ -36,9 +34,7 @@
 private:
     SkSVGUse();
 
-    SkSVGStringType    fHref;
-    SkSVGLength        fX = SkSVGLength(0);
-    SkSVGLength        fY = SkSVGLength(0);
+    bool parseAndSetAttribute(const char*, const char*) override;
 
     using INHERITED = SkSVGTransformableNode;
 };

modules/svg/src/SkSVGUse.cpp

diff --git a/modules/svg/src/SkSVGUse.cpp b/modules/svg/src/SkSVGUse.cpp
index bf4457c..6d80f0a 100644
--- a/modules/svg/src/SkSVGUse.cpp
+++ b/modules/svg/src/SkSVGUse.cpp
@@ -17,42 +17,15 @@
     SkDebugf("cannot append child nodes to this element.\n");
 }
 
-void SkSVGUse::setHref(const SkSVGStringType& href) {
-    fHref = href;
-}
-
-void SkSVGUse::setX(const SkSVGLength& x) {
-    fX = x;
-}
-
-void SkSVGUse::setY(const SkSVGLength& y) {
-    fY = y;
-}
-
-void SkSVGUse::onSetAttribute(SkSVGAttribute attr, const SkSVGValue& v) {
-    switch (attr) {
-    case SkSVGAttribute::kHref:
-        if (const auto* href = v.as<SkSVGStringValue>()) {
-            this->setHref(*href);
-        }
-        break;
-    case SkSVGAttribute::kX:
-        if (const auto* x = v.as<SkSVGLengthValue>()) {
-            this->setX(*x);
-        }
-        break;
-    case SkSVGAttribute::kY:
-        if (const auto* y = v.as<SkSVGLengthValue>()) {
-            this->setY(*y);
-        }
-        break;
-    default:
-        this->INHERITED::onSetAttribute(attr, v);
-    }
+bool SkSVGUse::parseAndSetAttribute(const char* n, const char* v) {
+    return INHERITED::parseAndSetAttribute(n, v) ||
+           this->setX(SkSVGAttributeParser::parse<SkSVGLength>("x", n, v)) ||
+           this->setY(SkSVGAttributeParser::parse<SkSVGLength>("y", n, v)) ||
+           this->setHref(SkSVGAttributeParser::parse<SkSVGIRI>("xlink:href", n, v));
 }
 
 bool SkSVGUse::onPrepareToRender(SkSVGRenderContext* ctx) const {
-    if (fHref.isEmpty() || !INHERITED::onPrepareToRender(ctx)) {
+    if (fHref.fIRI.isEmpty() || !INHERITED::onPrepareToRender(ctx)) {
         return false;
     }
 
@@ -68,7 +41,7 @@
 }
 
 void SkSVGUse::onRender(const SkSVGRenderContext& ctx) const {
-    const auto ref = ctx.findNodeById(fHref);
+    const auto ref = ctx.findNodeById(fHref.fIRI);
     if (!ref) {
         return;
     }
@@ -77,7 +50,7 @@
 }
 
 SkPath SkSVGUse::onAsPath(const SkSVGRenderContext& ctx) const {
-    const auto ref = ctx.findNodeById(fHref);
+    const auto ref = ctx.findNodeById(fHref.fIRI);
     if (!ref) {
         return SkPath();
     }
@@ -86,13 +59,17 @@
 }
 
 SkRect SkSVGUse::onObjectBoundingBox(const SkSVGRenderContext& ctx) const {
-    const auto ref = ctx.findNodeById(fHref);
+    const auto ref = ctx.findNodeById(fHref.fIRI);
     if (!ref) {
         return SkRect::MakeEmpty();
     }
 
-    const SkRect bounds = ref->objectBoundingBox(ctx);
-    const SkScalar x = ctx.lengthContext().resolve(fX, SkSVGLengthContext::LengthType::kHorizontal);
-    const SkScalar y = ctx.lengthContext().resolve(fY, SkSVGLengthContext::LengthType::kVertical);
-    return SkRect::MakeXYWH(x, y, bounds.width(), bounds.height());
+    const SkSVGLengthContext& lctx = ctx.lengthContext();
+    const SkScalar x = lctx.resolve(fX, SkSVGLengthContext::LengthType::kHorizontal);
+    const SkScalar y = lctx.resolve(fY, SkSVGLengthContext::LengthType::kVertical);
+
+    SkRect bounds = ref->objectBoundingBox(ctx);
+    bounds.offset(x, y);
+
+    return bounds;
 }