274886 - attributes corresponding to SVGLength properties are always set

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

I'm opening this bug since it no longer looks like the issue described in bug
270257, and the issue described below, will be fixed by making the same changes
to the source.

Bug: when an element is inserted into a document, all its markup attributes
corresponding to properties of type SVGLength are set, whether or not they were
really set in the markup.

This bug is occuring because of deficiencies in our current observer mechanism.
Some observers of nsSVGLength objects - nsSVGViewBox for one, there may be
others - need to know if the "context" of the nsSVGLength changes. As Alex
noted, for percentage lengths, the 'real' user-unit value is determined by
looking at the context, and most observers of percentage lengths will expect to
be informed of a change in the 'real' value of a length. However, the current
observer mechanism notifies *all* observers simply that an object has changed,
not *how* it has changed. Unfortunately, the notification sent out as a result
of calling nsSVGLength::SetContext results in a call to
nsSVGElement::SetAttrAndNotify. For example, the stack that results in
SetAttrAndNotify being called when inserting an nsSVGRect element into a
document is as follows.

Object               Method
------               ------
nsSVGRectElement     nsSVGElement::SetAttrAndNotify()
nsSVGRectElement     nsSVGElement::DidModifySVGObservable()
nsSVGAnimatedLength  nsSVGValue::NotifyObservers()
nsSVGAnimatedLength  nsSVGValue::DidModify()
nsSVGAnimatedLength  nsSVGAnimatedLength::DidModifySVGObservable()
nsSVGLength          nsSVGValue::NotifyObservers()
nsSVGLength          nsSVGValue::DidModify()
nsSVGLength          nsSVGLength::SetContext()
nsSVGRectElement     nsSVGRectElement::ParentChainChanged()
nsSVGRectElement     nsSVGElement::SetParent()

I propose we fix this by adding a "the context is being set" flag to nsSVGLength
which we will set/unset in SetContext. If we then add an "is the context being
set" method to the nsISVGLength interface, nsSVGElement::SetAttrAndNotify can
try QIing the attribute passed to it to nsISVGLength, and if that works, test
the "is the context being set" method to determine if it should be setting the
attribute.

Of course, this is a hack, but it will solve this problem so we can move on
other bugs until we replace the notification system.

Comment 1

[Alex Fritze]

It is quite likely that we want do discriminate between different modifications
elsewhere. Hence I think a more scaleable approach is to add a modification_type
parameter to WillModify/DidModify. 

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I agree with alex. The somthing's-being-changed notification should still go
out, we just should make it possible for observers to see what is being changed,
so that elements can ignore context changes.

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: patch

like this?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 171155 [details] [diff] [review]
patch

given the size of this patch you might as well make those extra arguments
required (i.e. don't supply a default value).

What's the purpose of mod_count?

A final nit: I would call mod_unspecified mod_other or something. Unspecified
sounds like it could include context.

Comment 5

[Jonathan Watt [:jwatt]]

Thanks for the feedback Jonas. I think you're right about not providing a
default value for most of the functions, but I think we should still supply a
default for WillModify and DidModify here:
http://lxr.mozilla.org/mozilla/source/content/svg/content/src/nsSVGValue.h#55
and thus also for nsSVGValueAutoNotifier here:
http://lxr.mozilla.org/mozilla/source/content/svg/content/src/nsSVGValue.h#91

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: patch v2

Comment 7

[tor]

Comment on attachment 171209 [details] [diff] [review]
patch v2

Looks good - the one minor nit I have is that the comment in
nsSVGElement.cpp should give a quick summary of why the
early return is there in addition to pointing at the bug.
No need to attach another full patch - if you put the verbage
in here it can be included in the checkin.  r=tor

Comment 8

[Jonathan Watt [:jwatt]]

Sure, how about:

 NS_IMETHODIMP
-nsSVGElement::DidModifySVGObservable(nsISVGValue* aObservable)
+nsSVGElement::DidModifySVGObservable(nsISVGValue* aObservable,
+                                     nsISVGValue::modificationType aModType)
 {
+  // return without setting attribute observers in the markup if their
+  // element is being inserted into an SVG fragment: bug 274886
+  if (aModType == nsISVGValue::mod_context)
+    return NS_OK;
+
   PRUint32 i, count = mMappedAttributes.AttrCount();


One minor nit I have myself is that in nsSVGAngle.cpp I put in one space too
many at:

nsSVGAngle::DidModifySVGObservable(nsISVGValue* observable,
                                    modificationType aModType)

Can you remove the extra space to bring the parameters into alingment before
checking this in please?

Comment 9

[Jonathan Watt [:jwatt]]

Checked in by tor:
http://bonsai.mozilla.org/cvsquery.cgi?who=tor%25cs.brown.edu&date=explicit&mindate=2005-01-15+17%3A44&maxdate=2005-01-15+17%3A44