Closed
Bug 619503
Opened 14 years ago
Closed 13 years ago
Support SMIL animation of the 'class' attribute
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla5
Tracking | Status | |
---|---|---|
status2.0 | --- | wanted |
People
(Reporter: jwatt, Assigned: longsonr)
References
()
Details
(Keywords: dev-doc-complete)
Attachments
(2 files, 4 obsolete files)
550 bytes,
image/svg+xml
|
Details | |
39.30 KB,
patch
|
jwatt
:
review+
roc
:
approval2.0-
|
Details | Diff | Splinter Review |
We should support SMIL animation of the 'class' attribute.
Reporter | ||
Comment 1•14 years ago
|
||
Reporter | ||
Updated•14 years ago
|
Assignee | ||
Comment 2•14 years ago
|
||
Assignee: nobody → longsonr
Attachment #501678 -
Flags: review?(jwatt)
Assignee | ||
Comment 3•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #501678 -
Flags: review?(dholbert)
Comment 4•14 years ago
|
||
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•14 years ago
|
||
Attachment #501678 -
Attachment is obsolete: true
Attachment #502171 -
Flags: review?(jwatt)
Attachment #501678 -
Flags: review?(jwatt)
Assignee | ||
Comment 6•14 years ago
|
||
Attachment #501679 -
Attachment is obsolete: true
Reporter | ||
Comment 7•14 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•14 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•14 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•13 years ago
|
||
I've figured out what's wrong now. Will do a new patch.
Assignee | ||
Comment 11•13 years ago
|
||
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•13 years ago
|
Attachment #506239 -
Flags: review?(jwatt) → review+
Reporter | ||
Comment 12•13 years ago
|
||
Pushed this to Try: http://tbpl.mozilla.org/?tree=MozillaTry#push-20176
Reporter | ||
Comment 13•13 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•13 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•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/3baa29a2109c
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Target Milestone: --- → mozilla2.2
Assignee | ||
Comment 17•13 years ago
|
||
Thanks Ehsan.
Comment 18•13 years ago
|
||
Deb, this seems like something for your list of new platform features...
Keywords: dev-doc-needed
Comment 19•13 years ago
|
||
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•13 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•10 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.
Description
•