Bug 17779 – SVG 1.1 Errata demands "SVG JavaScript Liveness" support

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Nikolas Zimmermann]

SVG Errata states: "All SVG DOM objects that directly correspond to an attribute, e.g. the SVGAnimatedLength 'ry' in an SVGRectElement, are live. This means that any changes made to the attribute are immediately reflected in the corresponding SVG DOM object." (see linked URL)

This makes sense, and way need to find a proper way to fix this bug.
HTML handles these cases in an easy & fast approach. HTML properties who equal to HTML attributes that get exposed to JS always operate (or return) simple POD types like String, long etc.

So HTML*Element classes don't actually store String/long etc values in the implementation, but forward calls ie. to setAlign(const String& align) to it's corresponding HTML DOM methods (setAttribute(HTMLNames::alignAttr. ...))

Accessing HTML*Element's properties works by forwarding to getAttribute().

In contrary SVG*classes containing animated SVG properties do store these types (mostly non-POD-Types, like SVGLength, SVGTransform, etc.) as member variables. This creates the problem that the string result of getAttribute(someSVGAttribute) is not equal to the current values exposed to SVG DOM (someElement.someSVGAttribute.baseVal....)

As you can see the fast HTML approach only works if you operate on POD types. For SVG a more sophisticated solution is needed. Proposing following concept:

Add a new method to the SVG ANIMATED_PROPERTY_DECLARATIONS macros: "virtual void synchronize##UpperProperty();" and maintain a boolean state wheter the attribute has been changed via SVG DOM.

Add a new function "virtual AtomicString svgAnimatedPropertyAsString(const QualifiedName&) const " to SVGElement, that has to be implemented by all SVG*Element classes which define animated properties.

The idea is that everytime JS getAttribute() calls are invoked for SVGElements, each SVG*Element will ask it's animated properties to synchronize their "Attribute*" string value with the internal representation as SVG DOM Object.

For example, SVGRectElement::synchronizeX(), will call SVGRectElement::svgAnimatedPropertyAsString(SVGNames::xAttr) to convert the internal "SVGAnimatedLength* m_x" object to a "String", and will update it in the element's NamedAttrMap. The call to getAttribute("x") will always be in sync with the result of someRect.x.baseVal.value.

Of course this updating procedure in the NamedAttrMap, only needs to be done in case the internal SVG DOM was modified, this is tracked by a boolean variable in every SVG*Animated*Element class. In case it's not modified, synchronize##UpperProperty() won't do anything.

Calls to setAttribute() already modify the corresponding SVG DOM in trunk, as setAttribute("x") (to stay with the example above) call SVGRectElement::parseMappedAttribute, which in turn calls setXBaseValue() which in fact updates the SVG DOM. It's just the SVG DOM -> XML DOM synchronization that hasn't been implemented yet.

[Nikolas Zimmermann]

Created attachment 19669 [details]
Prototyped idea

This patch implements the proposed solution. Open for discussion, as always.

[Nikolas Zimmermann]

(In reply to comment #1)
> Created an attachment (id=19669) [edit]
> Prototyped idea
> 
> This patch implements the proposed solution. Open for discussion, as always.
I'll put up another patch soon, which also implements a first complete version of the interface, which also includes the parts responsible to map SVG attributes to the synchronizeXXX() values, so that getAttribute() can just do: if (PointerToSVGUpdateFunction func = updateFunctionForAttribute(attributeName)) *func(); before executing the actual query in the NamedAttrMap. More on this later.

[Jonathan Haas]

Created attachment 19675 [details]
New layout test that demonstrates this problem

Creates a rectangle at (100, 100), sets the x coordinate to 200 using the SVG DOM, sets the y coordinate to 200 using setAttribute. The rectangle is moved to (200, 200), but using getAttribute on the x coordinate still returns 100.

[Jonathan Haas]

Created attachment 19745 [details]
Layout test for this bug

eseidel introduced me to the wonders of the JavaScript layout test framework, so I reworked this test to use it. Added expected results, too, and formatted as a patch.

[Nikolas Zimmermann]

Jonathan, thanks for the excellent testcase. I discovered the bug when creating svg/dynamic-updates/SVGLinearGradient-dom-gradientTransform-attr.html - though it's a wise idea to keep a seperated layout test just for the xml dom<->svg dom sync.

If you like working a bit on layout tests, there is much todo for LayoutTests/svg/dynamic-updates. In case you haven't looked at it yet, the idea is to test scriptability of every SVG DOM property of every SVG element exposed to JS. Hence the naming sheme: svg/dynamic-updates/SVGXXXElement-dom-xxx-attr.html and svg/dynamic-updates-SVGXXXElement-svgdom-xxx-prop.html.

The tests work like this:
- setup the needed svg elements dynamically, document.createElement / element.setAttribute
- test changing a single attribute value using a) XML DOM (getAttribute) b) SVG DOM (someElement.x.baseVal.value = ...)
- some shouldBe's wheter the value actually changed

Current limitations:
- repaint bugs currently not detected, as the waitForClickEvent logic is flawed (fixed on my hdd)
- For simplicity we're only testing the base class properties for now (aka. for SVGRectElement tests we only check the modification of x/y/width/height). Once _all_ base class properties are tested we can add tests for all super-class properties (most important: changing 'transform' of ie. SVGRectElement). The only for reason is that it's already exhausting to create ~ 10 tests per element :-)

The result is a text-only dump - like the normal JS test framework (aka. the testcase results you've uploaded here) plus a SVG pixel test dump (-expected.png)

For the test in LayoutTests/svg/dynamic-updates both results matter, the text results verify the actual DOM/SVG DOM attribute/property is accessable & changable - and the pixel test result verifies if repainting worked correctly (most important!)

As you can see I already finished tests for SVGAElement until SVGForeignObjectElement (alphabetically). On my harddisk if already added tests for SVGGradientElement / SVGLinearGradientElement / SVGRadialGradientElement (24 tests in total, checking 12 properties using two ways, XML DOM / SVG DOM).

I'm looking for help urgently, as it's a huge job to do.
Coming back to the SVG JavaScript liveness - I think your testcase is fine & commitable, though we need a new test suite LayoutTests/svg/property-liveness, using the same test structure as in svg/dynamic-updates, to assure that our implementation is 100% valid for all cases.

I hope you got interessted, if you are please drop me a line here :-)

Greetings,
Niko

[Nikolas Zimmermann]

Created attachment 19974 [details]
Initial patch

[Oliver Hunt]

Comment on attachment 19974 [details]
Initial patch

Still just doing a first run sanity check
* formatting issues with WebCore/svg/SVGScriptElement.h
* A number of macros use InheritFromClass, i feel SuperClass would be better
 * the changes to WebCore/rendering/SVGRootInlineBox.cpp seem unrelated afaict

A don't like StoredTypeWithDirtyFlag (the name) but haven't come up with anything better yet

Ocerall this looks sane, but it's going to take me a while to do a real review of the damned animation macros :p
:D

[Oliver Hunt]

Comment on attachment 19974 [details]
Initial patch

SVGLength::operator== needs slight reformatting to line up the m_'s

The only remaining issue i see is the name of the StoredDooferWithDirty (or whatever ;)

[Nikolas Zimmermann]

(In reply to comment #7)
> (From update of attachment 19974 [details] [edit])
> Still just doing a first run sanity check
> * formatting issues with WebCore/svg/SVGScriptElement.h
> * A number of macros use InheritFromClass, i feel SuperClass would be better
>  * the changes to WebCore/rendering/SVGRootInlineBox.cpp seem unrelated afaict
The changes to SVGRootInlineBox are actually needed, because the changes to the ANIMATED_ properties highlight a serious problem with casting from Node* to SVGTextContentElement* where the Node points to an anchor element <a> (which is not a SVGTextContentElement!). So it's a serious bug already present in trunk - no idea why it doesn't crash though, vagrind would have detected these problems (I want valgrind on osx!!)

> 
> A don't like StoredTypeWithDirtyFlag (the name) but haven't come up with
> anything better yet
Maybe LazilyUpdatedType?

> SVGLength::operator== needs slight reformatting to line up the m_'s
Fixed.

Greetings,
Niko

[Oliver Hunt]

(In reply to comment #9)
> (I want valgrind on osx!!)
me too!!

> 
> > 
> > A don't like StoredTypeWithDirtyFlag (the name) but haven't come up with
> > anything better yet
> Maybe LazilyUpdatedType?
Sounds good.

r=me with that update

[Nikolas Zimmermann]

Took a long time, finally landed in r31566.
In case you wonder about the added .png files, those were missing layout test pixel dumps. Forgot to denote in the changelog :(

[Antti Koivisto]

This patch has:

--- WebCore/dom/Element.h	(revision 31232)
+++ WebCore/dom/Element.h	(working copy)
@@ -47,7 +47,7 @@
     virtual const ClassNames* getClassNames() const;
     const AtomicString& getIDAttribute() const;
     bool hasAttribute(const QualifiedName&) const;
-    const AtomicString& getAttribute(const QualifiedName&) const;
+    virtual const AtomicString& getAttribute(const QualifiedName&) const;

I don't think making Element::getAttribute() virtual is a good idea. It is bad for performance. I also don't think there are any acceptable architectural reasons for doing that.

[Nikolas Zimmermann]

(In reply to comment #12)
> This patch has:
> 
> --- WebCore/dom/Element.h       (revision 31232)
> +++ WebCore/dom/Element.h       (working copy)
> @@ -47,7 +47,7 @@
>      virtual const ClassNames* getClassNames() const;
>      const AtomicString& getIDAttribute() const;
>      bool hasAttribute(const QualifiedName&) const;
> -    const AtomicString& getAttribute(const QualifiedName&) const;
> +    virtual const AtomicString& getAttribute(const QualifiedName&) const;
> 
> I don't think making Element::getAttribute() virtual is a good idea. It is bad
> for performance. I also don't think there are any acceptable architectural
> reasons for doing that.

Hi Antti,

I agree with you, regarding the performance issue. Though when not making it virtual, we'd need to make NamedAttrMap aware of SVGElement..... The animated property synchronization has to happen before getAttribute / hasAttribute is called (hasAttribute is still broken).

We should really have a chat about this.

Greetings,
Niko

P.S. I heard the excellent animation news, looking forward to talk to you :-)

[Oliver Hunt]

Rolled out in r31600 per antti's request, also protects me from stephanie hunting me down over a perf regression :D

[Antti Koivisto]

I don't understand all the complexity in this patch. From what I gather 'liveness' simply means that SVGAnimatedFoo objects act as proxis to the underlying attribute value (instead of being copies).

According to my understanding

rectElement.x.baseVal.value = 55;

should just translate to rect->setAttribute("x", "55") internally. That will spin up rects parseAttribute() method which will update any cached length value.

Why do you need these property synchronizers etc? 

Genrally I find the macro/template code for animated properties virtually inpenetrable. Since the underlying concept is very simple (geting and setting values) I find it hard to understand why all this complexity is needed. I wonder if we could get rid some of it? For example, I don't see why we need to be able to generate the data member, all the accessors and the DOM binding object with single line macro. Could we just specify the data member and accessors explicitly in code (like we do everywhere else) and worry about the bindings separately?

[Antti Koivisto]

Comment on attachment 19974 [details]
Initial patch

marking r- for now for the virtual getAttribute

[Nikolas Zimmermann]

Hi Antti,
> I don't understand all the complexity in this patch. From what I gather
> 'liveness' simply means that SVGAnimatedFoo objects act as proxis to the
> underlying attribute value (instead of being copies).
> 
> According to my understanding
> 
> rectElement.x.baseVal.value = 55;
> 
> should just translate to rect->setAttribute("x", "55") internally. That will
> spin up rects parseAttribute() method which will update any cached length
> value.
Okay, for this simple example your assumption holds true, but not for any complex datastructure like SVGAnimatedTransformList. Think of modifying a single item in the SVGTransformList, you do not want to call setAttribute("transform", concat_of_all_list_entries_as_string) here, that's overkill and not performant in any way.

Another issue is that attributes are _string_ values, and we want to store SVG datatypes internally (SVGLength, SVGTransform, etc.) so there is no 1:1 map between propertys & attributes.

Hope that clarifies the concept a bit.

> Genrally I find the macro/template code for animated properties virtually
> inpenetrable. Since the underlying concept is very simple (geting and setting
> values) I find it hard to understand why all this complexity is needed. I
> wonder if we could get rid some of it? For example, I don't see why we need to
> be able to generate the data member, all the accessors and the DOM binding
> object with single line macro. Could we just specify the data member and
> accessors explicitly in code (like we do everywhere else) and worry about the
> bindings separately?
While I agree that the macro complexity is high, and it's quite hard to understand, we did this on purpose. Before SVGAnimated* property handling was actually specialiazed in the CodeGeneratorJS, though that adds much problems for any other language binding.

Need to leave, but I can elaborate on this later. Eric also knows the reasons behind.

Greetings,
Niko

[Antti Koivisto]

(In reply to comment #17)
> Okay, for this simple example your assumption holds true, but not for any
> complex datastructure like SVGAnimatedTransformList. Think of modifying a
> single item in the SVGTransformList, you do not want to call
> setAttribute("transform", concat_of_all_list_entries_as_string) here, that's
> overkill and not performant in any way.

You can optimize by updating the cached value directly and preventing parseMappedAttribute() from doing any parsing during setAttribute(). You could get rid of parseMappedAttibute() alltogether and instead invalidate your cache as a result of attributeChanged(). I think the problem is perfectly solvable without adding complex scheme to update underlying string value lazily. Generating the attribute string itself is not costy. 

I would just start by a simple working solution and optimizing later as needed. In common case any time spent on updating the attribute values as a result of a DOM call, no matter how ineffective, is likely to be minimal compared to the overall script, layout and paint complexity. 
 
> Another issue is that attributes are _string_ values, and we want to store SVG
> datatypes internally (SVGLength, SVGTransform, etc.) so there is no 1:1 map
> between propertys & attributes.

I am not sure we should be using SVG datatypes as internal storage for simply types like SVGLength. I think length values should be held in simple class like Length in HTML side (FloatLength?) and SVGLength would be just a proxy object accessing that externally, instantiated on demand.

There is no need for our internal data to be organized to match the (crazy) SVG DOM API.

> While I agree that the macro complexity is high, and it's quite hard to
> understand, we did this on purpose. Before SVGAnimated* property handling was
> actually specialiazed in the CodeGeneratorJS, though that adds much problems
> for any other language binding.

There is significant cost for this kind of complexity. It makes SVG code hard to understand, unhackable and drives potential contributors away. Hackability is one of the major WebKit goals.

> Need to leave, but I can elaborate on this later. Eric also knows the reasons
> behind.
> 
> Greetings,
> Niko
> 

[Antti Koivisto]

Yeah, I think you are right the the lazy synchronization does make sense for complex properties like transforms. The macro/template stuff needed is pretty unfortunate but that is the current approach and doing something about that is a separate topic.

Solution that does not involve making getAttribute() virtual should be found. There is existing updateStyleAttributeIfNeeded() mechanism for lazy updates which is called from all approriate places and could be expanded (or just renamed) to cover this case too.

With that kind of change the patch is fine.

[Nikolas Zimmermann]

(In reply to comment #19)
> Yeah, I think you are right the the lazy synchronization does make sense for
> complex properties like transforms. The macro/template stuff needed is pretty
> unfortunate but that is the current approach and doing something about that is
> a separate topic.
> 
> Solution that does not involve making getAttribute() virtual should be found.
> There is existing updateStyleAttributeIfNeeded() mechanism for lazy updates
> which is called from all approriate places and could be expanded (or just
> renamed) to cover this case too.
> 
> With that kind of change the patch is fine.

Excellent news, I'll see if I can come up with a better solution, soon.

Greetings,
Niko

[Antti Koivisto]

I actually just renamed updateStyleAttributeIfNeeded() to updateStyleAttribute() while optimizing it a bit. Just to let you know to avoid confusion.

Perhaps just call it updateLazyAttributes() or something, remove styleAttr check from

    if (name == styleAttr && !m_isStyleAttributeValid)
        updateStyleAttribute();

and do the attribute check inside the virtual function. That shouldn't add too many new virtual calls.

[Nikolas Zimmermann]

Created attachment 22007 [details]
Updated patch

Improved version of the previous patch. No more virtual getAttribute() hacks, instead use a similar solution compared to "updateStyleAttribute".

[Nikolas Zimmermann]

Created attachment 22009 [details]
Updated patch v2

Changed some variable names, after discussion with Antti.

[Antti Koivisto]

Comment on attachment 22009 [details]
Updated patch v2

looks good

[Nikolas Zimmermann]

Landed in r34913.

[Adam Roben (:aroben)]

I had to roll out attachment 22009 [details] in r34925. It broke the Windows build due to bad casting. As the ChangeLog in r34925 explains:

> r34913 introduced code that assigned pointers-to-member from a derived
> class into a base class pointer-to-member type (e.g., assigned a void
> (SVGUseElement::*)() into a void (SVGElement::*)()). This is bad
> because it could allow us to call SVGUseElement member functions on a
> different SVGElement-derived class. MSVC rightly flagged this as an
> error.

[Adam Roben (:aroben)]

Comment on attachment 22009 [details]
Updated patch v2

Clearing review flag

[Nikolas Zimmermann]

Created attachment 22099 [details]
Updated patch v3 (w/o layout tests)

Setting r? flag, though as discussed with Eric - I'll split this patch up in several chunks.
Just wanted to leave it here, so it can already be reviewed (most of the changes are macro changes anyway)

[Nikolas Zimmermann]

Created attachment 22128 [details]
Final patch (w/o layout tests, as nothing changed there)

[Nikolas Zimmermann]

Finally lined in r35035.

[Nikolas Zimmermann]

Oliver, where is the relation between filters and this bug? I've seen you added "Bug 17779 depends on ..."

[Oliver Hunt]

ummm
i was cloning the dependencies for the old filters bug, which had turned into a refactoring bug rather than the actual implementation bug.  So the questions is why did someone else say this was blocked by that.  And to that question i have no answer so i will remove those dependencies