Bug 12511 – <use> has event dispatching issues

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

[Nikolas Zimmermann]

1. A referenced element shouldn't be the event target, but it's corresponding SVGElementInstance object.
2. Event handlers on the referenced element don't take any effect.

And in general the event dispatching is flaky (problem with shadowAncestorNode() for SVG elements).
Patch coming soon.

[Nikolas Zimmermann]

Created attachment 12833 [details]
Initial patch

No new regressions/leaks.

[Eric Seidel (no email)]

Comment on attachment 12833 [details]
Initial patch

use-elementInstance-event-target.svg
Should probably also test event.target to make sure it's the proper type (by checking the toString?)  Maybe there is already a test for that?

Also, what about testing each of the ElementInstance methods?  We should make sure those return sane values.

This needs a comment to explain why this different behavior is necessary:
 Node* Node::shadowAncestorNode()
 {
+#ifdef SVG_SUPPORT
+    if (isSVGElement())
+        return this;
+#endif

Maybe it should be split into a different method for forms vs. svg shadow trees?  Not sure.

I think toSVGElementInstance is a neat change. :)

As you mentioned in your changelog, This SVG_SUPPORT block is probably not necessary:
+#ifdef SVG_SUPPORT
+    for (n = this; n; n = n->isShadowNode() ? n->shadowParentNode() : n->parentNode()) {
+#else
     for (n = this; n; n = n->parentNode()) {
+#endif

It at least needs a comment (for the next person who comes along wanting to make a different type of shadow node)

Shouldn't useElementForShadowTreeElement check the type?  ASSERT at least that it's an SVGUseElement?  Ah, I see you do that outside... I think it would be better to do that inside the method, since you named it "useElementFor" "topmostShadowParentFor" would be an alternative naming if you felt it would be needed by non-SVG code.

It's sad that every event dispatch has to do 4? tree walks at least.  We're adding 2 tree walks here, one to get up to the <use> and one to get back down to the element instance.

This is a great change.  I think it could be improved still a little, but my above comments were all on superficial/little stuff.

Adele and I agreed that she would also take a look.  If she felt it was sane she'd r+ it.

[Darin Adler]

Comment on attachment 12833 [details]
Initial patch

This has too much SVG_SUPPORT code sprinkled in the core. We need to aim to avoid doing that. I don't buy the "trying not to affect the non-SVG version" argument; most mainstream versions of WebKit are including SVG, so lets not try to optimize too much for a less-common case.

+    SVGElementInstance* instance = target->toSVGElementInstance();
+    if (instance)
+        return toJS(exec, instance);

We usually declare inside the if statements for lines like this one.

+#ifdef SVG_SUPPORT
+    class SVGElementInstance; 
+#endif

No need to put the declaration inside an #ifdef.

-    RegisteredEventListenerList::Iterator end = listenersCopy.end();    
-    
+    RegisteredEventListenerList::Iterator end = listenersCopy.end();
+

???

+#ifdef SVG_SUPPORT
+    for (n = this; n; n = n->isShadowNode() ? n->shadowParentNode() : n->parentNode()) {
+#else
     for (n = this; n; n = n->parentNode()) {
+#endif

I think it's a bad idea to put this in an ifdef. We have no need to slightly optimize the no-SVG case like this.

I'm not cormfortable with all this special-case code in EventTargetNode::dispatchEvent -- please make this non-SVG-specific.

 Node* Node::shadowAncestorNode()
 {
+#ifdef SVG_SUPPORT
+    if (isSVGElement())
+        return this;
+#endif

Is there some way we can avoid this.

[Nikolas Zimmermann]

Hi Eric & Darin,

> Should probably also test event.target to make sure it's the proper type (by
> checking the toString?)  Maybe there is already a test for that?
Hm,  the test will fail anyway if event.target is not of SVGElementInstance type...
Do you still think we should add a seperate toString test?
 
> Also, what about testing each of the ElementInstance methods?  We should make
> sure those return sane values.
Right, that deserves a new test!
 
> This needs a comment to explain why this different behavior is necessary:
>  Node* Node::shadowAncestorNode()
>  {
> +#ifdef SVG_SUPPORT
> +    if (isSVGElement())
> +        return this;
> +#endif
> 
> Maybe it should be split into a different method for forms vs. svg shadow
> trees?  Not sure.
Oh yes, that definately needs explaination. The SVG shadow tree should work like a normal tree (CSS inheritance across shadow-tree boundaries, event dispatching etc.) Current Event Dispatching code assumes that if it finds a shadow node, then dispatch the event to the shadowParentNode() - this is obviously correct for the HTML forms shadow tree part, but not for us.

Just grep for "shadowAncestorNode" in WebCore/dom - that shows quite some trickery for the HTML forms shadow tree part which breaks us.
 
> I think toSVGElementInstance is a neat change. :)
;-)
 
> As you mentioned in your changelog, This SVG_SUPPORT block is probably not
> necessary:
> +#ifdef SVG_SUPPORT
> +    for (n = this; n; n = n->isShadowNode() ? n->shadowParentNode() :
> n->parentNode()) {
> +#else
>      for (n = this; n; n = n->parentNode()) {
> +#endif
> 
> It at least needs a comment (for the next person who comes along wanting to
> make a different type of shadow node)

Okay, totally fine.
 
> Shouldn't useElementForShadowTreeElement check the type?  ASSERT at least that
> it's an SVGUseElement?  Ah, I see you do that outside... I think it would be
> better to do that inside the method, since you named it "useElementFor"
> "topmostShadowParentFor" would be an alternative naming if you felt it would be
> needed by non-SVG code.
okay right, maybe it should directly return a SVGUseElement object - I'll go for that.

> It's sad that every event dispatch has to do 4? tree walks at least.  We're
> adding 2 tree walks here, one to get up to the <use> and one to get back down
> to the element instance.
Yeah, I had some " // SVG <use> sucks" comments in before ;-)
We could optimize all of these by extensively using HashMap's for this job - could also simplify "associateInstancesWithShadowTreeElements()" in SVGUseElement a lot. Though I wanted to avoid this in the beginning, to get it working first - we can still optimize a lot!
 
> This is a great change.  I think it could be improved still a little, but my
> above comments were all on superficial/little stuff.
Nah, totally fine :-)
 
> Adele and I agreed that she would also take a look.  If she felt it was sane
> she'd r+ it.

Commenting on Darin's issues next...

[Nikolas Zimmermann]

Hi Darin,

> This has too much SVG_SUPPORT code sprinkled in the core. We need to aim to
> avoid doing that. I don't buy the "trying not to affect the non-SVG version"
> argument; most mainstream versions of WebKit are including SVG, so lets not try
> to optimize too much for a less-common case.
Hehe :-) Well I'm more than fine with this, if you see WebKit+SVG as mainstream now, it makes me more than happy, to remove these unneeded SVG_SUPPORT blocks.

> 
> +    SVGElementInstance* instance = target->toSVGElementInstance();
> +    if (instance)
> +        return toJS(exec, instance);
> 
> We usually declare inside the if statements for lines like this one.
Okay.
 
> +#ifdef SVG_SUPPORT
> +    class SVGElementInstance; 
> +#endif
> 
> No need to put the declaration inside an #ifdef.
Okay.

> 
> -    RegisteredEventListenerList::Iterator end = listenersCopy.end();    
> -    
> +    RegisteredEventListenerList::Iterator end = listenersCopy.end();
> +
> 
> ???
There was a trailing whitespace, I removed.

> 
> +#ifdef SVG_SUPPORT
> +    for (n = this; n; n = n->isShadowNode() ? n->shadowParentNode() :
> n->parentNode()) {
> +#else
>      for (n = this; n; n = n->parentNode()) {
> +#endif
> 
> I think it's a bad idea to put this in an ifdef. We have no need to slightly
> optimize the no-SVG case like this.
Okay, will remove that block.

> 
> I'm not cormfortable with all this special-case code in
> EventTargetNode::dispatchEvent -- please make this non-SVG-specific.

I'd love to - any idea how to eventually move that stuff out of EventTargetNode?
I could only think of reimplementing dispatchEvent() in SVGElement?
 
>  Node* Node::shadowAncestorNode()
>  {
> +#ifdef SVG_SUPPORT
> +    if (isSVGElement())
> +        return this;
> +#endif
> 
> Is there some way we can avoid this.

I commented above on this (in reply to Eric).

Thanks a lot for checking the patch!
Niko

[Nikolas Zimmermann]

Created attachment 12854 [details]
Updated patch

Updated patch adressing most (all?) issues Eric/Darin mentioned. Should be nicer now, as EventTargetNode is almost left unchanged - and SVGElement now implements dispatchEvent.

I like this solution much better!

[Nikolas Zimmermann]

Created attachment 12883 [details]
Final patch

Updated against ToT (test results changed because of the RenderHiddenSVGContainer addition) plus reference bugs which are fixed by this in the ChangeLog.

[Eric Seidel (no email)]

Comment on attachment 12883 [details]
Final patch

This could really be r+'d but since you're around and there are some minor issues, we might as well go one more round.

1.  you owe a test, given your previous comments about ElementInstance :)

2. dispatchEvent won't compile as-is, without changes to EventTargetNode.  Also, you might consider making dispatchEvent just take an additional target parameter, and defautl it to "this" instead of making a separate method.  You'd have to change all the places that override it though...  I could go either way on that.  It's a little ocnfusing as to which one shoudl be virtual and which one should be called (unless one is marked private) though w/o that change.

if (isSVGElement())
return this
is still ugly.  But at least you gave a nice comment in the code.  Maybe that method shoudl be virtual?

shadowTreeParentElementForShadowTreeElement could probably be written a little more succinctly as a do while loop, but it's fine...

You should note in your changelog that you fixed the <use> crash.  Ideally we'd also have a test for that crash (I know, no one made a reduction of it yet, but it's probably a pretty simple <use> with display: none)

[Nikolas Zimmermann]

Created attachment 12889 [details]
Final patch v2

All issues fixed - last missing test added.

[Maciej Stachowiak]

Comment on attachment 12889 [details]
Final patch v2

r=me on Eric's behalf, now that test case is included.

[Nikolas Zimmermann]

Landed in r19378.