Support SMIL animation of the 'class' attribute

RESOLVED FIXED in mozilla5

Status

()

RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: jwatt, Assigned: longsonr)

Tracking

({dev-doc-complete})

unspecified
mozilla5
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(status2.0 wanted)

Details

(URL)

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

8 years ago
We should support SMIL animation of the 'class' attribute.
(Reporter)

Comment 1

8 years ago
Created attachment 497907 [details]
testcase
(Reporter)

Updated

8 years ago
status2.0: --- → wanted
(Assignee)

Comment 2

8 years ago
Created attachment 501678 [details] [diff] [review]
diff -w patch
Assignee: nobody → longsonr
Attachment #501678 - Flags: review?(jwatt)
(Assignee)

Updated

8 years ago
Attachment #501678 - Flags: review?(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.
Attachment #501678 - Flags: review?(dholbert) → review+
(Assignee)

Comment 5

8 years ago
Created attachment 502171 [details] [diff] [review]
dholbert's comments addressed
Attachment #501678 - Attachment is obsolete: true
Attachment #502171 - Flags: review?(jwatt)
Attachment #501678 - Flags: review?(jwatt)
(Assignee)

Comment 6

8 years ago
Created attachment 502173 [details] [diff] [review]
standard patch
Attachment #501679 - Attachment is obsolete: true
(Reporter)

Comment 7

8 years ago
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>
Attachment #502171 - Flags: review?(jwatt) → review+
(Assignee)

Comment 8

8 years ago
(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:
(Assignee)

Comment 9

8 years ago
(In reply to comment #7)

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

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

Comment 10

8 years ago
I've figured out what's wrong now. Will do a new patch.
(Assignee)

Comment 11

8 years ago
Created attachment 506239 [details] [diff] [review]
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.
Attachment #502171 - Attachment is obsolete: true
Attachment #502173 - Attachment is obsolete: true
Attachment #506239 - Flags: review?(jwatt)
(Reporter)

Updated

8 years ago
Attachment #506239 - Flags: review?(jwatt) → review+
(Reporter)

Comment 13

8 years ago
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.
Attachment #506239 - Flags: approval2.0?
Comment on attachment 506239 [details] [diff] [review]
hg changeset patch

Let's take this post-FF4
Attachment #506239 - Flags: approval2.0? → approval2.0-

Comment 15

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

I will try to merge this into mozilla-central tomorrow.
Whiteboard: fixed-in-cedar

Comment 16

8 years ago
http://hg.mozilla.org/mozilla-central/rev/3baa29a2109c
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Target Milestone: --- → mozilla2.2
(Assignee)

Comment 17

8 years ago
Thanks Ehsan.

Comment 18

8 years ago
Deb, this seems like something for your list of new platform features...
Keywords: dev-doc-needed
Documented:

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

Also mentioned on Firefox 5 for developers.
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Updated

7 years ago
Flags: in-testsuite+
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.)
Flags: needinfo?(longsonr)
(Assignee)

Comment 21

4 years ago
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");
Flags: needinfo?(longsonr)
You need to log in before you can comment on or make changes to this bug.