removeAttribute doesn't effect some underlying DOM properties

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
14 years ago
11 years ago

People

(Reporter: jwatt, Assigned: tor)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(2 attachments, 9 obsolete attachments)

Reporter

Description

14 years ago
 
An examination of the code suggests that they do default to 3. Do you have a testcase which proves otherwise?
Reporter

Comment 2

14 years ago
The important part here is "if their attributes are removed". (Using removeAttributeNS.) I don't have a testcase (I can make one), but I know the code well enough to know it is the case. This bug was more a note to myself since I don't think many people will stumble over the problem.
Posted image testcase
OK, I understand now.

There seem to be lots of SVG attributes that should be reset when they are removed but as far as I can tell removeAttributeNS does not reset any values to the default in the mozilla SVG (or HTML if the concept is even relevant for HTML). This does seem to be implemented by the Adobe SVG plug in.

For example <rect> x and y attributes should be reset to 0. markerWidth and markerHeight are only special in that they do not get reset to 0 as most attributes do.

Is this how UnsetAttr should be implemented for marker elements?
Presumably this needs implementing for all other SVG elements.
Assignee

Comment 6

14 years ago
Yes, I think this is what we'll need.
Reporter

Comment 7

14 years ago
Comment on attachment 207616 [details] [diff] [review]
patch for marker only (for comment)

>-      align = nsIDOMSVGPreserveAspectRatio::SVG_MEETORSLICE_MEET;
>+      meetOrSlice = nsIDOMSVGPreserveAspectRatio::SVG_MEETORSLICE_MEET;

Crap! We should never hit that code path so I guess it doesn't matter too much, but it's annoying to think this has propogated from my original code and gone unnoticed for so long. Can you fix that in nsSVGSVGElement.cpp, nsSVGImageFrame.cpp and nsSVGPatternFrame.cpp to keep them in sync please?
wow, that is a massive amount of bloaty code. There must be a better way...
I mean, it would be nice if we had a better way to do attribute mapping :-(
Reporter

Comment 10

14 years ago
Comment on attachment 207616 [details] [diff] [review]
patch for marker only (for comment)

>+    if (aAttr == nsSVGAtoms::markerWidth) {
>+      nsIDOMSVGLength *length;
>+      mMarkerWidth->GetBaseVal(&length);
>+      length->SetValue(3.0f);

Need to call SetCoordCtxRect here.

>+    }
>+    if (aAttr == nsSVGAtoms::markerHeight) {
>+      nsIDOMSVGLength *length;
>+      mMarkerHeight->GetBaseVal(&length);
>+      length->SetValue(3.0f);

And here.

>+    }

And for that matter we should be checking for changes to markerWidth/Height in DidModifySVGObservable and calling SetCoordCtxRect there too. (And while on the subject of DidModifySVGObservable it should only be doing the sync mOrientAngle stuff when 'observable' is mOrientType. Note we aren't currently observing mMarkerWidth/Height, so you'd need to NS_ADD_SVGVALUE_OBSERVER() them in Init().)

>+    if (aAttr == nsSVGAtoms::orient) {
>+      mOrientType->SetBaseVal(SVG_MARKER_ORIENT_ANGLE);

You also need:

      nsIDOMSVGAngle *angle;
      mOrientAngle->GetBaseVal(&angle);
      angle->NewValueSpecifiedUnits(nsIDOMSVGAngle::SVG_ANGLETYPE_UNSPECIFIED, 0.0f);

>+    }
>+    if (aAttr == nsSVGAtoms::viewBox) {
>+      nsIDOMSVGRect *viewbox;
>+      mViewBox->GetBaseVal(&viewbox);
>+      SetCoordCtxRect(viewbox);
>+    }
>+    if (aAttr == nsSVGAtoms::preserveAspectRatio || aAttr == nsSVGAtoms::viewBox) {

This seems wrong. Why are you resetting mPreserveAspectRatio if viewBox is removed?

>+      nsIDOMSVGPreserveAspectRatio *par;
>+      mPreserveAspectRatio->GetBaseVal(&par);
>+      par->SetAlign(nsIDOMSVGPreserveAspectRatio::SVG_PRESERVEASPECTRATIO_XMIDYMID);
>+      par->SetMeetOrSlice(nsIDOMSVGPreserveAspectRatio::SVG_MEETORSLICE_MEET);
>+    }
>+  }
>+  return nsSVGMarkerElementBase::UnsetAttr(aNameSpaceID, aAttr, aNotify);
>+}
Assignee: general → longsonr
Posted patch patch for marker only (obsolete) — Splinter Review
New and improved with even more bloaty code than before! 

If someone else wrote a function to determine which atom matched (if any) from an array of atoms that would help a little as the multiple if statements in the UnsetAttr could be recoded as a switch.

It seems bug 290967 is tracking removeAttribute being broken in general so I am just doing marker under this one (Especially as I think it is the first one to get a removeAttribute implementation). Perhaps this should block bug 290967.

Do I need to change the GUID of nsIDOMSVGRect since I added a method at the end? I am currently assuming its OK not to.
Attachment #207616 - Attachment is obsolete: true
Attachment #210154 - Flags: review?(jwatt)
Although comment 10 said

> Need to call SetCoordCtxRect here.

after resetting markerWidth

It also said

And for that matter we should be checking for changes to markerWidth/Height in
> DidModifySVGObservable and calling SetCoordCtxRect there too. (And while on the
> subject of DidModifySVGObservable it should only be doing the sync ? mOrientAngle
> stuff when 'observable' is mOrientType. Note we aren't currently observing
> mMarkerWidth/Height, so you'd need to NS_ADD_SVGVALUE_OBSERVER() them in
> Init().)

If this change is made then the act of changing the value of markerWidth results in a call to WillModifySVGObservable and so directly calling SetCoordCtxRect in UnsetAttr seems unnecessary. Accordingly I have removed that code in UnsetAttr
Attachment #210154 - Attachment is obsolete: true
Attachment #210335 - Flags: review?(jwatt)
Attachment #210154 - Flags: review?(jwatt)
Reporter

Comment 13

14 years ago
I'm about to leave on vacation for three weeks. I've not had time to look at this unfortunately, but to be honest I think this is too low priority for us to want to further complicate the code with these changes before we do our major rewrite. Maybe tor can take a look if you disagree Robert.
Posted patch corrections to nsSVGOrient (obsolete) — Splinter Review
I'm not sure what major rewrite you envisage Jonathan.

This patch corrects the following issues with marker elements:
a) removeAttribute does nothing 
  (further similar patches would be required to implement removeAttribute
   correctly on other elements)
b) DOM changes to refX, refY and viewBox are not tracked properly.
c) Setting orient="20deg" i.e. anything except "auto" does not work.
   In practice this is the less than 1% case. 95% of the time in the SVG 
   I have seen orient="auto" specified, the other 5% orient 
   given an invalid value rather than specified as an angle.

I am happy to split out b) and c) into a separate fix which would contain only
the nsSVGOrient class and changes to nsSVGMarkerElement (without the UnsetAttr).
Attachment #210335 - Attachment is obsolete: true
Attachment #210473 - Flags: review?(tor)
Attachment #210335 - Flags: review?(jwatt)
Assignee

Comment 15

14 years ago
(In reply to comment #14)
> I am happy to split out b) and c) into a separate fix which would contain only
> the nsSVGOrient class and changes to nsSVGMarkerElement (without the
> UnsetAttr).

If you could do that, it would be great.

For the unset portion, instead of needing to write this sort of code for each element, a table per element type and a base class unset method seems like it might be a cleaner direction to go.

If you do modify an idl as you do in this patch, the uuid should be updated.  Do we need to resort to a [noscript] method for this, though?
Comment on attachment 210473 [details] [diff] [review]
corrections to nsSVGOrient

I have created bug 325728 to address comment 10 markerWidth/markerHeight trakcing and the orient problem.
Attachment #210473 - Flags: review?(tor)
Posted patch work in progress (obsolete) — Splinter Review
Hopefully this addresses the previous review comments although it is still incomplete.

It has base class unset code which should be OK for 95% of attributes.
Unusual values require code in each element.

Only the marker element implementation has been completed so far.

I made the unset method of nsIDOMSVGRect [noscript] because:
a) it is not part of the SVG spec.
b) I don't think we should allow calling Unset from javascript.
Attachment #210473 - Attachment is obsolete: true
Attachment #214333 - Flags: review?(tor)
Hopefully this covers everything now.
Attachment #214333 - Attachment is obsolete: true
Attachment #215027 - Flags: review?(tor)
Attachment #214333 - Flags: review?(tor)
Comment on attachment 215027 [details] [diff] [review]
implement unset for all attributes

Looks like this will be implemented as part of bug 332162
Attachment #215027 - Flags: review?(tor)
Assignee: longsonr → general
Looks to me like this was fixed by the check in for bug 332162.
Blocks: 290967
Assignee

Updated

12 years ago
Severity: minor → normal
Summary: Properties markerWidth and markerHeight should default to 3 if their attributes are removed → removeAttribute doesn't effect some underlying DOM properties
Assignee

Updated

12 years ago
Duplicate of this bug: 290967
Assignee

Comment 23

12 years ago
More testcases in bug 290967.
Assignee

Updated

12 years ago
Attachment #278789 - Flags: review?(longsonr)
Comment on attachment 278789 [details] [diff] [review]
update to tip (new style types already handle reset, misc changes)

>+
>+      // Now check for one of the old style basetypes going away
>+      nsCOMPtr<nsISVGValue> svg_value = GetMappedAttribute(aNamespaceID, aName);
>+
>+      if (svg_value) {
>+        nsCOMPtr<nsIDOMSVGAnimatedAngle> a = do_QueryInterface(svg_value);
>+        if (a) {
>+          NS_WARNING("must provide element processing for unset angle");
>+        }
>+        nsCOMPtr<nsIDOMSVGAnimatedBoolean> b = do_QueryInterface(svg_value);
>+        if (b) {
>+          NS_WARNING("must provide element processing for unset boolean");
>+        }
>+        nsCOMPtr<nsIDOMSVGAnimatedInteger> i = do_QueryInterface(svg_value);
>+        if (i) {
>+          NS_WARNING("must provide element processing for unset integer");
>+        }

I put this my original patch when there were lots of things with the old base types and I was afraid I might have missed some out. That's no longer the case so perhaps this should just go. If you think it should stay then NS_ASSERTION is probably better than NS_WARNING.

r=longsonr either way.
Attachment #278789 - Flags: review?(longsonr) → review+
Assignee

Comment 25

12 years ago
Posted patch switch to NS_ASSERTION (obsolete) — Splinter Review
Assignee: general → tor
Status: NEW → ASSIGNED
Attachment #279750 - Flags: superreview?(roc)
+        nsCOMPtr<nsIDOMSVGAnimatedAngle> a = do_QueryInterface(svg_value);
+        NS_ASSERTION(!a, "must provide element processing for unset angle");
+
+        nsCOMPtr<nsIDOMSVGAnimatedBoolean> b = do_QueryInterface(svg_value);
+        NS_ASSERTION(!b, "must provide element processing for unset boolean");
+
+        nsCOMPtr<nsIDOMSVGAnimatedInteger> i = do_QueryInterface(svg_value);
+        NS_ASSERTION(!i, "must provide element processing for unset integer");

This should all be #ifdef DEBUG, we don't need the QIs on non-debug builds

+  if (aName == nsGkAtoms::filterRes && aNamespaceID == kNameSpaceID_None) {
+    mFilterResX->SetBaseVal(0);
+    mFilterResY->SetBaseVal(0);
+
+    return nsGenericElement::UnsetAttr(aNamespaceID, aName, aNotify);

why not just fall through here

+  if (aNamespaceID == kNameSpaceID_None && aName == nsGkAtoms::numOctaves) {
+    mNumOctaves->SetBaseVal(1);
+    return nsGenericElement::UnsetAttr(aNamespaceID, aName, aNotify);
+  }

ditto

+    if (processed) {
+      return nsGenericElement::UnsetAttr(aNamespaceID, aName, aNotify);
+    }

ditto, and elsewhere

Is there no way to share the code for initializing and unsetting attributes? It seems with this patch we have to write the default values in two places.
(In reply to comment #26)
> +  if (aName == nsGkAtoms::filterRes && aNamespaceID == kNameSpaceID_None) {
> +    mFilterResX->SetBaseVal(0);
> +    mFilterResY->SetBaseVal(0);
> +
> +    return nsGenericElement::UnsetAttr(aNamespaceID, aName, aNotify);
> 
> why not just fall through here

Because then you would end up in nsSVGElement::UnsetAttr and you would get the assertion that you had not overridden the unset i.e. the bit that you mentioned should be ifdef DEBUG. Calling nsGenericElement::UnsetAttr bypasses that.
Can we assert instead that the property has the default value?
(In reply to comment #28)
> Can we assert instead that the property has the default value?
> 

In nsSVGElement::UnsetAttr you mean? Only by encoding what the default value for all of those attributes actually is there too I think!
Will we eventually have animated angles, booleans and integers converted to the optimized length-style format, so these checks can go away, and also get rid of the duplication of the default value? If so, then I can live with this patch.
I hope so, although I'd like Tor to confirm.
Assignee

Comment 32

12 years ago
Yes, the plan is to switch everything over.  I went this route because it seemed like it might be more palatable to the powers-that-be given the current state of the tree freeze.
Comment on attachment 278789 [details] [diff] [review]
update to tip (new style types already handle reset, misc changes)

ok then. just wrap those QIs in #ifdef DEBUG.
Attachment #278789 - Flags: superreview+
Attachment #278789 - Flags: approval1.9+
Assignee

Comment 34

12 years ago
Attachment #279750 - Attachment is obsolete: true
Attachment #279750 - Flags: superreview?(roc)
Assignee

Comment 35

12 years ago
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Assignee

Comment 36

12 years ago
Backed out - nsSVGClassValue is also a nsIDOMSVGAnimatedString, but is not a nsSVGAnimatedString.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 37

12 years ago
Attachment #215027 - Attachment is obsolete: true
Attachment #278789 - Attachment is obsolete: true
Attachment #279780 - Attachment is obsolete: true
Attachment #279803 - Flags: superreview?(roc)
Attachment #279803 - Flags: superreview?(roc)
Attachment #279803 - Flags: superreview+
Attachment #279803 - Flags: review+
Attachment #279803 - Flags: approval1.9+
Assignee

Comment 38

12 years ago
Checked in.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
Depends on: 458531
You need to log in before you can comment on or make changes to this bug.