RemoveAttribute("class") on SVGStylableElement doesn't clear class name in nsSVGClassValue

RESOLVED FIXED

Status

()

defect
P2
normal
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: craig.topper, Assigned: craig.topper)

Tracking

({fixed1.9.1, regression})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 11 obsolete attachments)

1.46 KB, patch
longsonr
: review+
Details | Diff | Splinter Review
17.25 KB, patch
roc
: review+
Details | Diff | Splinter Review
Assignee

Description

11 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20081217 Shiretoko/3.1b3pre Firefox/3.1b3pre
Build Identifier: 

Calling RemoveAttribute("class") on any SVGStylableElement doesn't clear the class name in the nsSVGClassValue object stored within the nsSVGStylableElement class. Calls to GetAttribute("class") do show the class as being null, but element.className still shows the old class name.

I believe this was caused by the patch for bug 324461. nsSVGClassValue is an implementation of IDOMSVGAnimatedString and required the IDOMSVGAnimatedString code in ResetOldStyleBaseType of nsSVGGenericElement to clear it. This code was removed when bug 324461 was turned in.

I'm willing to write a patch for this. Should nsSVGClassValue be re-written similar to nsSVGString and the other "new style" attribute classes? This doesn't need to be as general purpose as those other classes so I'm thinking I can trap the calls to nsSVGStylableElement's ParseAttribute and UnsetAttr rather than implementing a DidChangeClass method in nsSVGGenericElement.

Reproducible: Always

Steps to Reproduce:
1. Create an SVGStylableElement with a class
2. Call RemoveAttribute("class") on that node
3. Read element.className
Actual Results:  
element.className will be the old class name

Expected Results:  
element.className should be an empty string
New style nsSVGString is the way to go. ParseAttribute/UnsetAttr is also preferred over DidChangeString. I'm hoping to remove the DidChange... methods one day.
Assignee

Comment 2

11 years ago
Posted patch First attempt at a fix (obsolete) — Splinter Review
I created a new nsSVGClassValue class following the style of nsSVGString.

I tried to avoid converting the nsAttrValue from nsSVGClassValue to a string to call SetAttr just to have it converted back to an nsAttrValue for SetAttrAndNotify. That required replicating some code from SetAttr which I don't really like.
Attachment #354493 - Flags: review?(longsonr)
Comment on attachment 354493 [details] [diff] [review]
First attempt at a fix

Looks to be broadly along the right lines except for one mystery piece I don't understand. 

You will need tests. I suggest adding to content/svg/content/test/test_dataTypes.html (and dataTypes-helper.svg) Add something that unsets the value and test it.

>+nsresult
>+nsSVGClassValue::SetBaseValueString(const nsAString& aValue,
>+                                    nsSVGStylableElement *aSVGElement)
>+{
>+  mBaseVal.ParseAtomArray(aValue);

This needs to set the separate animated value too.

>+
>+nsresult
>+nsSVGStylableElement::SetClassAttr(PRInt32 aNamespaceID, nsIAtom* aName,
>+                                   const nsAttrValue& aValue, PRBool aNotify)
>+{
>+  nsAutoString oldValue;
>+  nsAutoString newValue;
>+  PRBool modification = PR_FALSE;
>+  PRBool hasListeners = aNotify &&
>+    nsContentUtils::HasMutationListeners(this,
>+                                         NS_EVENT_BITS_MUTATION_ATTRMODIFIED,
>+                                         this);
>+
>+  aValue.ToString(newValue);
>+  
>+  // If we have no listeners and aNotify is false, we are almost certainly
>+  // coming from the content sink and will almost certainly have no previous
>+  // value.  Even if we do, setting the value is cheap when we have no
>+  // listeners and don't plan to notify.  The check for aNotify here is an
>+  // optimization, the check for haveListeners is a correctness issue.
>+  if (hasListeners || aNotify) {
>+    nsAttrInfo info(GetAttrInfo(aNamespaceID, aName));
>+    if (info.mValue) {
>+      // Check whether the old value is the same as the new one.  Note that we
>+      // only need to actually _get_ the old value if we have listeners.
>+      PRBool valueMatches;
>+      if (hasListeners) {
>+        // Need to store the old value
>+        info.mValue->ToString(oldValue);
>+        valueMatches = newValue.Equals(oldValue);
>+      } else { // if (aNotify) {  // guaranteed by initial if
>+        valueMatches = info.mValue->Equals(newValue, eCaseMatters);
>+      }
>+      if (valueMatches && info.mName->GetPrefix() == nsnull) {
>+        return NS_OK;
>+      }
>+      modification = PR_TRUE;
>+    }
>+  }
>+
>+  nsresult rv = BeforeSetAttr(aNamespaceID, aName, &newValue, aNotify);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsAttrValue tmpValue = aValue;
>+
>+  return SetAttrAndNotify(aNamespaceID, aName, nsnull, oldValue,
>+                          tmpValue, modification, hasListeners, aNotify,
>+                          &newValue);
>+}

Not sure what this is about? Care to explain where it has come from and what it is doing?

>+class nsSVGClassValue
>+{
>+public:
>+
>+  const nsAttrValue* GetAttrValue() const
>+    { return &mBaseVal; }

Maybe call this GetBaseAttrValue?

>+
>+  nsresult SetBaseValueString(const nsAString& aValue,
>+                              nsSVGStylableElement *aSVGElement);
>+  void GetBaseValueString(nsAString& aValue) const
>+    { mBaseVal.ToString(aValue); }

I'd just call these methods SetBaseValue and GetBaseValue

>+
>+  void Clear()
>+    { mBaseVal.Reset(); }

Can you call this Init rather than Clear please? And make it reset the animated value too.

>+
>+  nsresult ToDOMAnimatedString(nsIDOMSVGAnimatedString **aResult,
>+                               nsSVGStylableElement* aSVGElement);
>+
>+private:
>+  nsAttrValue mBaseVal;

You're going to need an mAnimVal too.

>+
>+  struct DOMAnimatedString : public nsIDOMSVGAnimatedString
>+  {

>+    NS_IMETHOD GetAnimVal(nsAString& aResult)
>+      { mVal->GetBaseValueString(aResult); return NS_OK; }

The above method needs to get the animated value not the base value.
> I tried to avoid converting the nsAttrValue from nsSVGClassValue to a string to
> call SetAttr just to have it converted back to an nsAttrValue for
> SetAttrAndNotify. That required replicating some code from SetAttr which I
> don't really like.

OK, should have read this; that's where the mystery code is from. Just do it the converting way rather than copying this code.
Attachment #354493 - Flags: review?(longsonr) → review-
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee

Comment 5

11 years ago
This patch addresses most of your comments. I left String in GetBaseValueString since the methods deal in strings and the base type in the class is nsAttrValue. This is similar to what is done for SVGLength2, SVGNumber2, SVGEnum, and SVGBoolean. Though this class is kinda odd since the DOM interface is in strings, but the internal represenation isn't.

Should DoGetClasses return the base or anim attribute?
Attachment #354493 - Attachment is obsolete: true
Attachment #354504 - Flags: review-
Assignee

Updated

11 years ago
Attachment #354504 - Flags: review- → review?(longsonr)
Assignee

Comment 6

11 years ago
Tests will be coming shortly
Assignee

Comment 7

11 years ago
Posted file Test case (obsolete) —
Assignee

Comment 8

11 years ago
Attachment #354507 - Attachment is obsolete: true
(In reply to comment #5)
> 
> Should DoGetClasses return the base or anim attribute?

The anim attribute.
Comment on attachment 354504 [details] [diff] [review]
Patch that addresses most of Robert's comments

>+class nsSVGClassValue
>+{
>+public:
>+
>+  void Init()
>+  {
>+    mBaseVal.Reset();
>+    mAnimVal.Reset();
>+  }

Should probably keep to the prevailing style of { and } placement here.

This and the anim change and I think you're there.
Attachment #354508 - Attachment is patch: true
Attachment #354508 - Flags: review+
Assignee

Comment 11

11 years ago
Attachment #354504 - Attachment is obsolete: true
Attachment #354517 - Flags: review?(longsonr)
Attachment #354504 - Flags: review?(longsonr)
Attachment #354517 - Flags: review?(longsonr) → review+
Flags: blocking1.9.1?
Keywords: regression
Assignee: nobody → craig.topper
Assignee

Updated

11 years ago
Attachment #354508 - Flags: superreview?(roc)
Assignee

Updated

11 years ago
Attachment #354517 - Flags: superreview?(roc)
Attachment #354508 - Flags: superreview?(roc) → superreview+
Why do we actually need an mBaseVal in nsSVGClassValue? Can't we just get the element's 'class' attribute when we need that?
Assignee

Comment 13

11 years ago
Followed roc's suggestion and removed mBaseVal in favor of GetAttr on the element. I ran the test case on it and it passed.

Could/should a similar thing be done for nsSVGString?
Attachment #354715 - Flags: review?(longsonr)
Yeah, I suppose it could and should be done for nsSVGString too.
Well, as long as all nsSVGStrings correspond to actual attributes. That isn't always the case. Although we could make the nsSVGString members nsAutoPtr<nsString>, and leave the pointers at null when the string is not needed in the object. (Actually mAnimVal should definitely be nsAutoPtr<nsSVGString> since it's almost always the same as mBaseVal.)
Assignee

Comment 16

11 years ago
GetBaseValue on nsSVGString doesn't actually have the information needed to call GetAttr because it doesn't have the namespace id or atom pointer.
Best do nsSVGString as a separate bug I think
Attachment #354715 - Flags: review?(longsonr) → review+
Assignee

Updated

11 years ago
Attachment #354517 - Flags: superreview?(roc)
Assignee

Updated

11 years ago
Attachment #354715 - Flags: superreview?(roc)
Assignee

Updated

11 years ago
Attachment #354517 - Attachment is obsolete: true
For lengths we just pass in the element pointer to GetBaseValue. I think we could do that for strings too. The namespace ID would always be "none".
For nsSVGClassValue, can we make mAnimVal be an nsAutoPtr<nsAttrValue>, which is null when it's the same as the base value? That would save a bit of memory for the common case where the class is not animated.
(In reply to comment #18)
> For lengths we just pass in the element pointer to GetBaseValue. I think we
> could do that for strings too. The namespace ID would always be "none".

No it wouldn't unfortunately as xlink:href are strings.
Oh, that's right, but if you pass in the element you can get the stringinfo object and that tells you the namespace as well as the attribute name atom.
Assignee

Comment 22

11 years ago
If we change mAnimVal nsSVGClassValue to not always be valid, then we need to call ParseAtoms on the value returned from GetAttr or dig further down and get the attr straight out of the attribute array of the element.
Doing the latter should be very simple.
Assignee

Comment 24

11 years ago
It just occurred to me that GetBaseValueString has no reason to be in the class right now. It doesn't use any member variables of the class. I'm thinking it should be moved up to the StylableElement itself.
Assignee

Comment 25

11 years ago
This patch turns mAnimval into an autoptr and falls back to the base value if it is null. This preserves the earlier change of getting the base value from the element's attribute array.
Attachment #354715 - Attachment is obsolete: true
Attachment #354715 - Flags: superreview?(roc)
Assignee

Updated

11 years ago
Attachment #354776 - Flags: review?(longsonr)
Assignee

Comment 26

11 years ago
Attachment #354776 - Attachment is obsolete: true
Attachment #354781 - Flags: review?(longsonr)
Attachment #354776 - Flags: review?(longsonr)
Assignee

Updated

11 years ago
Attachment #354781 - Flags: review?(longsonr)
Assignee

Comment 27

11 years ago
Got rid of the nsSVGClassValue class into nsSVGStylableElement. It had reached a point where every method in nsSVGClassValue needed a pointer to the element. Since the class only had one variable in it, it seemed easier just to move that one variable to the element and deal with everything there.
Attachment #354781 - Attachment is obsolete: true
+  if (aNamespaceID == kNameSpaceID_None && aName == nsGkAtoms::_class)
+    mAnimClass = nsnull;

{}

+nsSVGStylableElement::GetBaseClassString(nsAString& aResult) const

Call it GetClassBaseValString? GetBaseClass sounds like some kind of C++ "base class" thing which is totally wrong

+nsSVGStylableElement::SetBaseClassString(const nsAString& aValue)

Ditto

+  struct DOMAnimatedString : public nsIDOMSVGAnimatedString

Call this AnimatedClassString or something to make it clear that this is specifically an animated string for the class attribute
Craig, you don't need to ask me for any more reviews. Just get an sr from roc now.
Assignee

Comment 30

11 years ago
Posted patch Fixes roc's comments (obsolete) — Splinter Review
Attachment #354789 - Attachment is obsolete: true
Attachment #354792 - Flags: superreview?(roc)
Attachment #354792 - Flags: review?(roc)
Attachment #354792 - Attachment is patch: true
Attachment #354792 - Attachment mime type: application/octet-stream → text/plain
Attachment #354792 - Flags: superreview?(roc)
Attachment #354792 - Flags: superreview+
Attachment #354792 - Flags: review?(roc)
Attachment #354792 - Flags: review+
Keywords: checkin-needed
Whiteboard: [needs landing]
Assignee

Comment 31

11 years ago
Need to add cycle collection stuff for bug 467671. Will submit new patch soon.
Keywords: checkin-needed
Whiteboard: [needs landing]
Assignee

Comment 32

11 years ago
Attachment #354792 - Attachment is obsolete: true
Posted patch for check-in with tests (obsolete) — Splinter Review
Attachment #354508 - Attachment is obsolete: true
Attachment #354991 - Attachment is obsolete: true
I checked this in as http://hg.mozilla.org/mozilla-central/rev/245acb960a88 but then had to revert it as it caused reftest and mochitest failures.
Assignee

Comment 36

11 years ago
Ok the failures seem to be caused by the nsAttrValue in the attribute array having Type() eString instead of eAtom. This causes the CSS processing to believe there is no classes on the element.

We need to be able to get an nsAttrValue created with ParseAtomArray into the attribute array. My original patch did this with some gross replication of code. Is there an easier way?
Assignee

Comment 37

11 years ago
Nevermind I figured it out. Just need to call ParseAtomArray in the ParseAttribute override in nsSVGStylableElement.
Assignee

Comment 38

11 years ago
This patch removes some code from nsSVGStylableElement::ParseAttribute to allow nsStyledElement::ParseAttribute to handle some of the class attribute work like it does for non-SVG elements. Also removed the check from nsStyledElement that made sure SVG classes handled the class parsing themselves.
Attachment #355295 - Attachment is obsolete: true
Assignee

Updated

11 years ago
Keywords: checkin-needed
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Pushed e7e8b8ba6498
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs 191 landing]
Comment on attachment 354508 [details] [diff] [review]
Test case with the right content type


This part of the patch didn't got lost somehow and still needs landing
Attachment #354508 - Attachment is obsolete: false
Depends on: 467671
It wasn't in the patch in comment #38, that's why.
Keywords: checkin-needed
Whiteboard: [needs 191 landing] → [needs 191 landing][needs landing]
Version: unspecified → Trunk
(In reply to comment #39)
> Pushed e7e8b8ba6498
Pushed to 1.9.1 as http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5a3553ee0cbb

It looks like attachment 354508 [details] [diff] [review] (a tweak to "test_dataTypes.html") still needs landing on both mozilla-central and 1.9.1, though, per comment 40.
Whiteboard: [needs 191 landing][needs landing] → [attachment 354508 needs landing]
Whiteboard: [attachment 354508 needs landing] → [attachment 354508 needs landing on 1.9.1]
Attachment #355333 - Attachment description: For check-in. Fixes bug that broke tests → For check-in. Fixes bug that broke tests [Checkin: Comment 39 & 42]
attachment 354508 [details] [diff] [review] checked into 1.9.1 as
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d2b3f42b3562
Whiteboard: [attachment 354508 needs landing on 1.9.1]
You need to log in before you can comment on or make changes to this bug.