Bug 1502658 - Remove SVG use attribute change handling code from nsSVGUseFrame to SVGUseElement. r=longsonr

Most of those shouldn't rely on a frame to run. Differential Revision: https://phabricator.services.mozilla.com/D9996 --HG-- extra : moz-landing-system : lando
2018-10-28 23:20:43 +00:00 · 2018-10-28 23:20:43 +00:00 · d4ae853741
--- a/dom/svg/SVGUseElement.cpp
+++ b/dom/svg/SVGUseElement.cpp
@ -92,6 +92,58 @@ SVGUseElement::~SVGUseElement()
 //----------------------------------------------------------------------
 // nsINode methods

+void
+SVGUseElement::ProcessAttributeChange(int32_t aNamespaceID, nsAtom* aAttribute)
+{
+  if (aNamespaceID == kNameSpaceID_None) {
+    if (aAttribute == nsGkAtoms::x || aAttribute == nsGkAtoms::y) {
+      if (auto* frame = GetFrame()) {
+        frame->PositionAttributeChanged();
+      }
+    } else if (aAttribute == nsGkAtoms::width ||
+               aAttribute == nsGkAtoms::height) {
+      const bool hadValidDimensions = HasValidDimensions();
+      const bool isUsed = OurWidthAndHeightAreUsed();
+      if (isUsed) {
+        SyncWidthOrHeight(aAttribute);
+      }
+
+      if (auto* frame = GetFrame()) {
+        frame->DimensionAttributeChanged(hadValidDimensions, isUsed);
+      }
+    }
+  }
+
+  if ((aNamespaceID == kNameSpaceID_XLink ||
+       aNamespaceID == kNameSpaceID_None) &&
+      aAttribute == nsGkAtoms::href) {
+    // We're changing our nature, clear out the clone information.
+    if (auto* frame = GetFrame()) {
+      frame->HrefChanged();
+    }
+    mOriginal = nullptr;
+    UnlinkSource();
+    TriggerReclone();
+  }
+}
+
+nsresult
+SVGUseElement::AfterSetAttr(int32_t aNamespaceID,
+                            nsAtom* aAttribute,
+                            const nsAttrValue* aValue,
+                            const nsAttrValue* aOldValue,
+                            nsIPrincipal* aSubjectPrincipal,
+                            bool aNotify)
+{
+  ProcessAttributeChange(aNamespaceID, aAttribute);
+  return SVGUseElementBase::AfterSetAttr(aNamespaceID,
+                                         aAttribute,
+                                         aValue,
+                                         aOldValue,
+                                         aSubjectPrincipal,
+                                         aNotify);
+}
+
 nsresult
 SVGUseElement::Clone(dom::NodeInfo* aNodeInfo, nsINode** aResult) const
 {
@ -179,7 +231,7 @@ SVGUseElement::CharacterDataChanged(nsIContent* aContent,

 void
 SVGUseElement::AttributeChanged(Element* aElement,
-                                int32_t aNameSpaceID,
+                                int32_t aNamespaceID,
                                nsAtom* aAttribute,
                                int32_t aModType,
                                const nsAttrValue* aOldValue)
--- a/dom/svg/SVGUseElement.h
+++ b/dom/svg/SVGUseElement.h
@ -87,6 +87,18 @@ public:
  // referenced element.
  void UpdateShadowTree();

+  // Shared code between AfterSetAttr and nsSVGUseFrame::AttributeChanged.
+  //
+  // This is needed because SMIL doesn't go through AfterSetAttr unfortunately.
+  void ProcessAttributeChange(int32_t aNamespaceID, nsAtom* aAttribute);
+
+  nsresult AfterSetAttr(int32_t aNamespaceID,
+                        nsAtom* aAttribute,
+                        const nsAttrValue* aValue,
+                        const nsAttrValue* aOldValue,
+                        nsIPrincipal* aSubjectPrincipal,
+                        bool aNotify) final;
+
 protected:
  /**
   * Helper that provides a reference to the element with the ID that is
--- a/layout/svg/nsSVGUseFrame.cpp
+++ b/layout/svg/nsSVGUseFrame.cpp
@ -6,6 +6,7 @@

 #include "nsSVGUseFrame.h"

+#include "mozilla/dom/MutationEvent.h"
 #include "mozilla/dom/SVGUseElement.h"
 #include "SVGObserverUtils.h"

@ -41,59 +42,58 @@ nsSVGUseFrame::Init(nsIContent*       aContent,
 }

 nsresult
-nsSVGUseFrame::AttributeChanged(int32_t aNameSpaceID,
+nsSVGUseFrame::AttributeChanged(int32_t aNamespaceID,
                                nsAtom* aAttribute,
                                int32_t aModType)
 {
-  auto* useElement = static_cast<SVGUseElement*>(GetContent());
-
-  if (aNameSpaceID == kNameSpaceID_None) {
-    if (aAttribute == nsGkAtoms::x || aAttribute == nsGkAtoms::y) {
-      // make sure our cached transform matrix gets (lazily) updated
-      mCanvasTM = nullptr;
-      nsLayoutUtils::PostRestyleEvent(
-        useElement, nsRestyleHint(0),
-        nsChangeHint_InvalidateRenderingObservers);
-      nsSVGUtils::ScheduleReflowSVG(this);
-      nsSVGUtils::NotifyChildrenOfSVGChange(this, TRANSFORM_CHANGED);
-    } else if (aAttribute == nsGkAtoms::width ||
-               aAttribute == nsGkAtoms::height) {
-      bool invalidate = false;
-      if (mHasValidDimensions != useElement->HasValidDimensions()) {
-        mHasValidDimensions = !mHasValidDimensions;
-        invalidate = true;
-      }
-
-      // FIXME(emilio): This shouldn't really belong to nsSVGUseFrame, but
-      // SVGUseElement.
-      if (useElement->OurWidthAndHeightAreUsed()) {
-        invalidate = true;
-        useElement->SyncWidthOrHeight(aAttribute);
-      }
-
-      if (invalidate) {
-        nsLayoutUtils::PostRestyleEvent(
-          useElement, nsRestyleHint(0),
-          nsChangeHint_InvalidateRenderingObservers);
-        nsSVGUtils::ScheduleReflowSVG(this);
-      }
-    }
+  // Currently our SMIL implementation does not modify the DOM attributes. Once
+  // we implement the SVG 2 SMIL behaviour this can be removed
+  // SVGUseElement::AfterSetAttr's implementation will be sufficient.
+  if (aModType == MutationEvent_Binding::SMIL) {
+    auto* content = SVGUseElement::FromNode(GetContent());
+    content->ProcessAttributeChange(aNamespaceID, aAttribute);
  }

-  if ((aNameSpaceID == kNameSpaceID_XLink ||
-       aNameSpaceID == kNameSpaceID_None) &&
-      aAttribute == nsGkAtoms::href) {
-    // we're changing our nature, clear out the clone information
+  return nsSVGGFrame::AttributeChanged(aNamespaceID, aAttribute, aModType);
+}
+
+void
+nsSVGUseFrame::PositionAttributeChanged()
+{
+  // make sure our cached transform matrix gets (lazily) updated
+  mCanvasTM = nullptr;
+  nsLayoutUtils::PostRestyleEvent(
+    GetContent()->AsElement(), nsRestyleHint(0),
+    nsChangeHint_InvalidateRenderingObservers);
+  nsSVGUtils::ScheduleReflowSVG(this);
+  nsSVGUtils::NotifyChildrenOfSVGChange(this, TRANSFORM_CHANGED);
+}
+
+void
+nsSVGUseFrame::DimensionAttributeChanged(bool aHadValidDimensions,
+                                         bool aAttributeIsUsed)
+{
+  bool invalidate = aAttributeIsUsed;
+  if (mHasValidDimensions != aHadValidDimensions) {
+    mHasValidDimensions = !mHasValidDimensions;
+    invalidate = true;
+  }
+
+  if (invalidate) {
    nsLayoutUtils::PostRestyleEvent(
-      useElement, nsRestyleHint(0),
+      GetContent()->AsElement(), nsRestyleHint(0),
      nsChangeHint_InvalidateRenderingObservers);
    nsSVGUtils::ScheduleReflowSVG(this);
-    useElement->mOriginal = nullptr;
-    useElement->UnlinkSource();
-    useElement->TriggerReclone();
  }
+}

-  return nsSVGGFrame::AttributeChanged(aNameSpaceID, aAttribute, aModType);
+void
+nsSVGUseFrame::HrefChanged()
+{
+  nsLayoutUtils::PostRestyleEvent(
+    GetContent()->AsElement(), nsRestyleHint(0),
+    nsChangeHint_InvalidateRenderingObservers);
+  nsSVGUtils::ScheduleReflowSVG(this);
 }

 //----------------------------------------------------------------------
--- a/layout/svg/nsSVGUseFrame.h
+++ b/layout/svg/nsSVGUseFrame.h
@ -31,9 +31,19 @@ public:
            nsContainerFrame* aParent,
            nsIFrame* aPrevInFlow) override;

-  nsresult AttributeChanged(int32_t aNameSpaceID,
+  // Called when the x or y attributes changed.
+  void PositionAttributeChanged();
+
+  // Called when the href attributes changed.
+  void HrefChanged();
+
+  // Called when the width or height attributes changed.
+  void DimensionAttributeChanged(bool aHadValidDimensions,
+                                 bool aAttributeIsUsed);
+
+  nsresult AttributeChanged(int32_t aNamespaceID,
                            nsAtom* aAttribute,
-                            int32_t aModType) override;
+                            int32_t aModType) final;

 #ifdef DEBUG_FRAME_DUMP
  nsresult GetFrameName(nsAString& aResult) const override
--- a/testing/web-platform/tests/svg/linking/reftests/use-hidden-attr-change.html
+++ b/testing/web-platform/tests/svg/linking/reftests/use-hidden-attr-change.html
@ -0,0 +1,32 @@
+<!doctype html>
+<meta charset=utf-8>
+<title>use element reacts to attribute changes when it's not rendered</title>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<link rel="help" href="https://svgwg.org/svg2-draft/struct.html#UseElement">
+<link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1502658">
+<link rel="match" href="/svg/linking/reftests/use-descendant-combinator-ref.html">
+<style>
+</style>
+<p>
+  You should see a green square, and no red.
+</p>
+<svg
+  version="1.1"
+  xmlns="http://www.w3.org/2000/svg"
+  xmlns:xlink="http://www.w3.org/1999/xlink"
+  style="display: none">
+  <defs>
+    <g id="square">
+      <rect width="100" height="100" fill="green" />
+    </g>
+  </defs>
+  <g id="test">
+    <use />
+  </g>
+</svg>
+<script>
+  onload = () => {
+    document.querySelector("use").setAttributeNS("http://www.w3.org/1999/xlink", "href", "#square");
+    document.querySelector("svg").style.display = "";
+  }
+</script>