Bug 1502658 - Remove SVG use attribute change handling code from nsSVGUseFrame to SVGUseElement. r=longsonr, a=RyanVM
authorEmilio Cobos Álvarez <emilio@crisal.io>
Sun, 28 Oct 2018 23:20:43 +0000
changeset 498191 c28a4c172db8362c9c66fe788484b6083d46b0c8
parent 498190 442e20f46b5d670b0dafb158072dadde45d58e23
child 498192 cbd3baf67f7fe586c281032fe4b2e0ae83b3daa3
push id10059
push userryanvm@gmail.com
push dateMon, 29 Oct 2018 14:13:12 +0000
treeherdermozilla-beta@c28a4c172db8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerslongsonr, RyanVM
bugs1502658
milestone64.0
Bug 1502658 - Remove SVG use attribute change handling code from nsSVGUseFrame to SVGUseElement. r=longsonr, a=RyanVM Most of those shouldn't rely on a frame to run. Differential Revision: https://phabricator.services.mozilla.com/D9996
dom/svg/SVGUseElement.cpp
dom/svg/SVGUseElement.h
layout/svg/nsSVGUseFrame.cpp
layout/svg/nsSVGUseFrame.h
testing/web-platform/tests/svg/linking/reftests/use-hidden-attr-change.html
--- a/dom/svg/SVGUseElement.cpp
+++ b/dom/svg/SVGUseElement.cpp
@@ -87,16 +87,68 @@ SVGUseElement::~SVGUseElement()
     !OwnerDoc()->SVGUseElementNeedsShadowTreeUpdate(*this),
     "Dying without unbinding?"
   );
 }
 
 //----------------------------------------------------------------------
 // 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
 {
   *aResult = nullptr;
   SVGUseElement *it = new SVGUseElement(do_AddRef(aNodeInfo));
 
   nsCOMPtr<nsINode> kungFuDeathGrip(it);
   nsresult rv1 = it->Init();
@@ -174,17 +226,17 @@ SVGUseElement::CharacterDataChanged(nsIC
 {
   if (nsContentUtils::IsInSameAnonymousTree(this, aContent)) {
     TriggerReclone();
   }
 }
 
 void
 SVGUseElement::AttributeChanged(Element* aElement,
-                                int32_t aNameSpaceID,
+                                int32_t aNamespaceID,
                                 nsAtom* aAttribute,
                                 int32_t aModType,
                                 const nsAttrValue* aOldValue)
 {
   if (nsContentUtils::IsInSameAnonymousTree(this, aElement)) {
     TriggerReclone();
   }
 }
--- a/dom/svg/SVGUseElement.h
+++ b/dom/svg/SVGUseElement.h
@@ -82,16 +82,28 @@ public:
 
   nsIURI* GetSourceDocURI();
   URLExtraData* GetContentURLData() const { return mContentURLData; }
 
   // Updates the internal shadow tree to be an up-to-date clone of the
   // 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
    * referenced by the 'use' element's 'href' attribute, and that will update
    * the 'use' element if the element that that ID identifies changes to a
    * different element (or none).
    */
   class ElementTracker final : public IDTracker {
--- a/layout/svg/nsSVGUseFrame.cpp
+++ b/layout/svg/nsSVGUseFrame.cpp
@@ -1,16 +1,17 @@
 /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* vim: set ts=8 sts=2 et sw=2 tw=80: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "nsSVGUseFrame.h"
 
+#include "mozilla/dom/MutationEvent.h"
 #include "mozilla/dom/SVGUseElement.h"
 #include "SVGObserverUtils.h"
 
 using namespace mozilla;
 using namespace mozilla::dom;
 
 //----------------------------------------------------------------------
 // Implementation
@@ -36,69 +37,68 @@ nsSVGUseFrame::Init(nsIContent*       aC
 
   mHasValidDimensions =
     static_cast<SVGUseElement*>(aContent)->HasValidDimensions();
 
   nsSVGGFrame::Init(aContent, aParent, aPrevInFlow);
 }
 
 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);
 }
 
 //----------------------------------------------------------------------
 // nsSVGDisplayableFrame methods
 
 void
 nsSVGUseFrame::ReflowSVG()
 {
--- a/layout/svg/nsSVGUseFrame.h
+++ b/layout/svg/nsSVGUseFrame.h
@@ -26,19 +26,29 @@ protected:
 public:
   NS_DECL_FRAMEARENA_HELPERS(nsSVGUseFrame)
 
   // nsIFrame interface:
   void Init(nsIContent* aContent,
             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
   {
     return MakeFrameName(NS_LITERAL_STRING("SVGUse"), aResult);
   }
 #endif
 
new file mode 100644
--- /dev/null
+++ 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>