619503 - Support SMIL animation of the 'class' attribute

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

We should support SMIL animation of the 'class' attribute.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: testcase

Comment 2

[Robert Longson [:longsonr]]

Attachment: diff -w patch

Comment 3

[Robert Longson [:longsonr]]

Attachment: patch

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 501678 [details] [diff] [review]
diff -w patch

Looks good to me.  Just a few nits & some test notes.


>+++ b/content/svg/content/src/nsSVGClass.cpp
>+nsSVGClass::ToDOMAnimatedString(nsIDOMSVGAnimatedString **aResult,
>+                                nsSVGElement *aSVGElement)
>+{
>+  *aResult = new DOMAnimatedString(this, aSVGElement);
>+  if (!*aResult)
>+    return NS_ERROR_OUT_OF_MEMORY;

No need for OOM check.

>+++ b/content/svg/content/src/nsSVGClass.h
>+  struct DOMAnimatedString : public nsIDOMSVGAnimatedString
>+    NS_IMETHOD GetBaseVal(nsAString & aResult)
>+      { mVal->GetBaseValue(aResult, mSVGElement); return NS_OK; }
>+    NS_IMETHOD SetBaseVal(const nsAString & aValue)
>+      { mVal->SetBaseValue(aValue, mSVGElement, PR_TRUE); return NS_OK; }
>+
>+    NS_IMETHOD GetAnimVal(nsAString & aResult)
>+    {

I've never seen this "nsType & varName" style in Gecko before -- can you make these &'s either left- or right-hugging, rather than neither? :)

>+++ b/layout/reftests/svg/smil/anim-class-01.svg
>+     class="reftest-wait" onload="setTimeAndSnapshot(1, true)">
[...]
>+  <rect width="100%" height="100%">
>+    <set attributeName="class" attributeType="XML"
>+         to="midway" begin="0s" dur="0.5s" fill="freeze"/>
>+    <animate attributeName="class" attributeType="XML"
>+         from="midway" to="final midway" begin="0.5s" dur="1s" fill="freeze"/>

Looks like that <set> element has no effect on the final test-rendering, since (after 0.5s) the <animate> element stomps on its effects.  Probably best to just remove the <set> for simplicity.

So this test ends up testing that "from-to" animation works, on an element with no base value.

We should test this a bit more thoroughly, particularly since it's so late in the Firefox 4.0 game.
A few ideas for things to test:
 - <rect class="redClass"><set to=""/></rect> (effectively clearing the 'class' attribute, if that works(?))
 - <rect class="redClass"><set to="limeClass"/></rect>
 - <rect class="limeClass"><set to="#'ThisIsAnInvalidClassName"/></rect>

r=dholbert with the nits & test fixed as noted, and with a few more tests (or additional animated rects in the existing test) along the lines suggested above.

Comment 5

[Robert Longson [:longsonr]]

Attachment: dholbert's comments addressed

Comment 6

[Robert Longson [:longsonr]]

Attachment: standard patch

Comment 7

[Jonathan Watt [:jwatt]]

Comment on attachment 502171 [details] [diff] [review]
dholbert's comments addressed

Great! r=jwatt

>diff --git a/content/svg/content/src/nsSVGElement.cpp b/content/svg/content/src/nsSVGElement.cpp
>-  // Strings
>-  {
>-    StringAttributesInfo info = GetStringInfo();
>-    for (PRUint32 i = 0; i < info.mStringCount; i++) {
>-      if (aNamespaceID == info.mStringInfo[i].mNamespaceID &&
>-          aName == *info.mStringInfo[i].mName) {
>-        return info.mStrings[i].ToSMILAttr(this);
>-      }
>-    }
>-  }

I don't mind this being moved down, but I'm wondering why? Just because it should be hit less now?

Also, while we have a mochitest for testing setting of .className.baseVal, we don't seem to have a reftest that checks it invalidates correctly. Could you add one, something like the following (untested!):

<!--
     Any copyright is dedicated to the Public Domain.
     http://creativecommons.org/licenses/publicdomain/
-->
<svg xmlns="http://www.w3.org/2000/svg"
     xmlns:xlink="http://www.w3.org/1999/xlink"
     class="reftest-wait" onload="set_lime_class();">
  <title>Test invalidation on setting .className.baseVal</title>
  <script xlink:href="smil-util.js" type="text/javascript"/>
  <style type="text/css">

    .lime { fill: lime; }

  </style>
  <script>

function set_lime_class()
{
  document.getElementById('rect').className.baseVal = 'lime';
  document.documentElement.removeAttribute('reftest-wait');
}

  </script>
  <rect id="rect" width="100%" height="100%" fill="red"/>
</svg>

Comment 8

[Robert Longson [:longsonr]]

(In reply to comment #7)
> Comment on attachment 502171 [details] [diff] [review]
> dholbert's comments addressed
> 
> Great! r=jwatt
> 
> >diff --git a/content/svg/content/src/nsSVGElement.cpp b/content/svg/content/src/nsSVGElement.cpp
> >-  // Strings
> >-  {
> >-    StringAttributesInfo info = GetStringInfo();
> >-    for (PRUint32 i = 0; i < info.mStringCount; i++) {
> >-      if (aNamespaceID == info.mStringInfo[i].mNamespaceID &&
> >-          aName == *info.mStringInfo[i].mName) {
> >-        return info.mStrings[i].ToSMILAttr(this);
> >-      }
> >-    }
> >-  }
> 
> I don't mind this being moved down, but I'm wondering why? Just because it
> should be hit less now?

Don't think of it as this code moving down as much as the other code moving up. Above the string code is a big if (namespace == none) test. The pointlist code needs to be in that block. Strings are the exception as they can be xlink:

Comment 9

[Robert Longson [:longsonr]]

(In reply to comment #7)

document.getElementById('rect').className.baseVal = 'lime';

doesn't work due to some assertion. Not sure why yet.

Comment 10

[Robert Longson [:longsonr]]

I've figured out what's wrong now. Will do a new patch.

Comment 11

[Robert Longson [:longsonr]]

Attachment: hg changeset patch

nsSVGClass.cpp needs to be registered as SVGAnimatedString as it implements nsIDOMSVGAnimatedString. This makes it a little simpler as the dom/base changes aren't required. Also included is a reftest for dynamically changing the class name using DOM.

Comment 12

[Jonathan Watt [:jwatt]]

Pushed this to Try:

http://tbpl.mozilla.org/?tree=MozillaTry#push-20176

Comment 13

[Jonathan Watt [:jwatt]]

Comment on attachment 506239 [details] [diff] [review]
hg changeset patch

Better link: http://tbpl.mozilla.org/?tree=MozillaTry&rev=063f7c689119

Passed try fine, modulo random orange, so requesting approval.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 506239 [details] [diff] [review]
hg changeset patch

Let's take this post-FF4

Comment 15

[(no longer active)]

http://hg.mozilla.org/projects/cedar/rev/3baa29a2109c

I will try to merge this into mozilla-central tomorrow.

Comment 16

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/3baa29a2109c

Comment 17

[Robert Longson [:longsonr]]

Thanks Ehsan.

Comment 18

[(no longer active)]

Deb, this seems like something for your list of new platform features...

Comment 19

[Eric Shepherd [:sheppy]]

Documented:

https://developer.mozilla.org/en/SVG/Attribute/class

Also mentioned on Firefox 5 for developers.

Comment 20

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I'm curious if you remember why you added:

  if (doc) {
    nsIPresShell* shell = doc->GetShell();
    if (shell) {
      shell->RestyleForAnimation(this, eRestyle_Self);
    }
  }

to nsSVGStylableElement::DidAnimateClass() (now in nsSVGElement::DidAnimateClass()).  In particular, it's not clear to me what styles will be different after the restyle has happened.  (I'm interested because I'm adding some optimizations for certain types of restyles, and I can't add such an optimization for this case because I don't understand what styles it's changing.)

Comment 21

[Robert Longson [:longsonr]]

SMIL animation could be from no class attribute or class="" to class="something" so you'll get whatever the definition of "something" is applied to the element and its descendants.

I expect that at the time I didn't think too much about it and probably copied it from  nsSMILMappedAttribute::FlushChangesToTargetAttr or nsSVGElement::WalkContentStyleRules but I'm not absolutely sure it's right. 

It should do whatever happens if you do setAttribute("class", "something");