Closed Bug 320622 Opened 15 years ago Closed 13 years ago

removeAttribute doesn't effect some underlying DOM properties

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jwatt, Assigned: tor)

References

()

Details

Attachments

(2 files, 9 obsolete files)

 
An examination of the code suggests that they do default to 3. Do you have a testcase which proves otherwise?
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.
Attached 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.
Yes, I think this is what we'll need.
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 :-(
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
Attached 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)
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.
Attached 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)
(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)
Attached 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
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
Duplicate of this bug: 290967
More testcases in bug 290967.
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+
Attached 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.
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+
Attachment #279750 - Attachment is obsolete: true
Attachment #279750 - Flags: superreview?(roc)
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Backed out - nsSVGClassValue is also a nsIDOMSVGAnimatedString, but is not a nsSVGAnimatedString.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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+
Checked in.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Depends on: 458531
You need to log in before you can comment on or make changes to this bug.