Bug 15413 – SVGElementInstance does not implement EventTarget

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

[Eric Seidel (no email)]

SVGElementInstance does not implement EventTarget

The implementation is stubbed out with empty methods.

[Eric Seidel (no email)]

Ha. So it turns out that SVGElementInstance.idl has a bogus LANGUAGE_OBJECTIVE_JAVASCRIPT line in it which is preventing it from even attempting to generate addEventListener, etc.  However, if you remove that line, the autogeneration system falls on its face trying.  It looks like JSEventTargetNode isn't actually autogenerated.

[Eric Seidel (no email)]

Created attachment 16579 [details]
test case (no browser works, so not fully tested, might be wrong)

[Nikolas Zimmermann]

Created attachment 17987 [details]
New layout test

[Nikolas Zimmermann]

Created attachment 17988 [details]
Initial patch

Uploading my EventTargetSVGElementInstance work. All tests pass, no regression. One problem remains which I'm unable to track down: Sometimes "evt.target == evt.currentTarget" does not work - it's a caching problem. If you rerun the test several times you'll see different results, but only for this evt.target/currentTarget check - everything else works as expected.

Eric: could you check the patch, and maybe help track it down? Already several days on this issues, without luck :(

Greetings,
NIko

[Eric Seidel (no email)]

Comment on attachment 17988 [details]
Initial patch

You might consider using two enums here:
return KJS::jsBoolean(eventTarget->dispatchEvent(toEvent(args[0]), exception, false /* tempEvent */, false /* useSVGTargetRules */));

then you won't need the comments. :)

I'm slightly surprised that:
JSEventTargetSVGElementInstance
can't be autogenerated. (I'm sure you have good reasons why, that I'm just not seeing on 3 hours of sleep @ 6 in the morning. :)  But if they're small reasons, you should file a bug asking for the necessary autogen functionality.

If you're going to move one, you might as well move both:

EventListener* EventTargetNode::getHTMLEventListener(const AtomicString &eventType)

I'll need to look again when I'm more awake.

[Eric Seidel (no email)]

Comment on attachment 17987 [details]
New layout test

The layout test is awesome.  However, I recently learned that this type of test could be entirely in a .js file and stored in resources/ then there is some sort of make-js-test-wrappers script which will actually generate the .html file for you.  Eventually we'll get rid of the .html file at all and run-webkit-tests will work entirely with the .js files.  Ask darin for more info. :)

In this case, you might not actually use that method due to the extra <svg> element in the .html file, but I thought you should know in case you didn't already.

[Eric Seidel (no email)]

I'm flying today, but I can take a look next time I'm hacking.

[Eric Seidel (no email)]

I added some logging to my local checkout.  Looks like we're creating too many SVGElementInstance (EventTargetSVGElementInstance) objects per use/element pair:

new SVGElementInstance: 0x18ce20f0 useElement: 0x18cb8700 originalElement: 0x18cb6330
new SVGElementInstance: 0x18ca9120 useElement: 0x18cb8700 originalElement: 0x18cb6330
new SVGElementInstance: 0x18cbae20 useElement: 0x18c09ba0 originalElement: 0x18caea30
new JSEventTargetSVGElementInstance: 0x1b397ca0 SVGElementInstance: 0x18cbae20
new SVGElementInstance: 0x18cc9770 useElement: 0x18c09ba0 originalElement: 0x18caea30
new JSEventTargetSVGElementInstance: 0x1b397500 SVGElementInstance: 0x18cc9770
Failed v1: 0x1b397500 v2: 0x1b397ca0

the "Failed" line is a failed === check for event.target === event.currentTarget

[Eric Seidel (no email)]

There are only 3 callsites to:
new EventTargetSVGElementInstance

So this should be relatively easy to debug.  I'll leave this to WildFox at this point, we can talk about it more when you're next around.

[Eric Seidel (no email)]

m_targetElementInstance = new EventTargetSVGElementInstance(this, target);
in void SVGUseElement::buildPendingResource()
is the callsite responsible for both copies.  Not yet sure why.

[Eric Seidel (no email)]

Backtraces:

#0	0x02137456 in WebCore::EventTargetSVGElementInstance::EventTargetSVGElementInstance at EventTargetSVGElementInstance.cpp:37
#1	0x0207f2e4 in WebCore::SVGUseElement::buildPendingResource at SVGUseElement.cpp:286
#2	0x01fdef81 in WebCore::SVGElementInstance::updateElementInstance at SVGElementInstance.cpp:213
#3	0x01c7e127 in WebCore::EventTarget::updateSVGElementInstancesAfterEventListenerChange at EventTarget.cpp:417
#4	0x01c7f44d in WebCore::EventTarget::removeEventListener at EventTarget.cpp:171
#5	0x021374dd in WebCore::EventTargetSVGElementInstance::removeEventListener at EventTargetSVGElementInstance.cpp:66
#6	0x01dc3fc7 in WebCore::JSEventTargetPrototypeFunction<1>::callAsFunction at JSEventTargetBase.h:142
#7	0x002a65dc in KJS::JSObject::call at object.cpp:96
#8	0x0030c510 in KJS::FunctionCallDotNode::inlineEvaluate at nodes.cpp:1209
#9	0x002c1546 in KJS::FunctionCallDotNode::evaluate at nodes.cpp:1214
#10	0x002b433f in KJS::ExprStatementNode::execute at nodes.cpp:3651

#0	0x02137449 in WebCore::EventTargetSVGElementInstance::EventTargetSVGElementInstance at EventTargetSVGElementInstance.cpp:37
#1	0x0213746f in WebCore::EventTargetSVGElementInstance::EventTargetSVGElementInstance at EventTargetSVGElementInstance.cpp:37
#2	0x0207f2e4 in WebCore::SVGUseElement::buildPendingResource at SVGUseElement.cpp:286
#3	0x01fdef81 in WebCore::SVGElementInstance::updateElementInstance at SVGElementInstance.cpp:213
#4	0x01c7e127 in WebCore::EventTarget::updateSVGElementInstancesAfterEventListenerChange at EventTarget.cpp:417
#5	0x01c7f44d in WebCore::EventTarget::removeEventListener at EventTarget.cpp:171
#6	0x021374dd in WebCore::EventTargetSVGElementInstance::removeEventListener at EventTargetSVGElementInstance.cpp:66
#7	0x01dc3fc7 in WebCore::JSEventTargetPrototypeFunction<1>::callAsFunction at JSEventTargetBase.h:142
#8	0x002a65dc in KJS::JSObject::call at object.cpp:96

#0	0x02137449 in WebCore::EventTargetSVGElementInstance::EventTargetSVGElementInstance at EventTargetSVGElementInstance.cpp:37
#1	0x0213746f in WebCore::EventTargetSVGElementInstance::EventTargetSVGElementInstance at EventTargetSVGElementInstance.cpp:37
#2	0x0207f2e4 in WebCore::SVGUseElement::buildPendingResource at SVGUseElement.cpp:286
#3	0x01fdef81 in WebCore::SVGElementInstance::updateElementInstance at SVGElementInstance.cpp:213
#4	0x01c7e127 in WebCore::EventTarget::updateSVGElementInstancesAfterEventListenerChange at EventTarget.cpp:417
#5	0x01c7f655 in WebCore::EventTarget::addEventListener at EventTarget.cpp:150
#6	0x01c7fc00 in WebCore::EventTargetNode::addEventListener at EventTargetNode.cpp:80
#7	0x01dc416b in WebCore::JSEventTargetPrototypeFunction<0>::callAsFunction at JSEventTargetBase.h:114
#8	0x002a65dc in KJS::JSObject::call at object.cpp:96

[Eric Seidel (no email)]

I would argue that you could land this w/o the currentTarget == target check and file a separate bug to track that issue.

[Nikolas Zimmermann]

(In reply to comment #12)
> I would argue that you could land this w/o the currentTarget == target check
> and file a separate bug to track that issue.

Ok fair enough - we need to track it down at some point, though your suggestion makes sense, and I'll prepare a new patch later.

Greetings,
Niko

[Nikolas Zimmermann]

*** Bug 16934 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

Created attachment 23722 [details]
Updated patch

Not marking for review yet, one layout test missing, ChangeLog to be written. Just for Eric ;-)

[Nikolas Zimmermann]

Created attachment 23733 [details]
Patch 1 - v1: Add (JS)EventTargetSVGElementInstance, don't use it.

[Nikolas Zimmermann]

Created attachment 23734 [details]
Patch 1 - v2 - Corrected patch, contained unrelated things.

[Eric Seidel (no email)]

Comment on attachment 23734 [details]
Patch 1 - v2 - Corrected patch, contained unrelated things.

static inline instead of just "inline"

This will of course crash:
JSEventListener* jsListener = static_cast<JSEventListener*>(listener);
due to bug 21044.  Ideally that call should just be fixed to return jsNull() in both instances instead of crashing. :(  But we can also do that as part of the other bug.

Sadly we'll (chrome) have to write all this for v8 too, due to use of custom bindings. :(

I don't think this is needed for Mail:
 594 - (void)addEventListener:(NSString*)type :(id <DOMEventListener>)listener :(BOOL)useCapture
 595 {
 596     // FIXME: this method can be removed once Mail changes to use the new method <rdar://problem/4746649>
 597     [self addEventListener:type listener:listener useCapture:useCapture];
 598 }

Mail isn't going to be getting at SVGElementInstances, is it?  I guess it could unintentioanlly register listeners on svg <use> content?

Looks sane enough.

[Nikolas Zimmermann]

(In reply to comment #18)
> (From update of attachment 23734 [details] [edit])
> static inline instead of just "inline"
Fixed, as discussed on IRC.

> 
> This will of course crash:
> JSEventListener* jsListener = static_cast<JSEventListener*>(listener);
> due to bug 21044.  Ideally that call should just be fixed to return jsNull() in
> both instances instead of crashing. :(  But we can also do that as part of the
> other bug.
> 
> Sadly we'll (chrome) have to write all this for v8 too, due to use of custom
> bindings. :(
Ouch...

> 
> I don't think this is needed for Mail:
>  594 - (void)addEventListener:(NSString*)type :(id <DOMEventListener>)listener
> :(BOOL)useCapture
>  595 {
>  596     // FIXME: this method can be removed once Mail changes to use the new
> method <rdar://problem/4746649>
>  597     [self addEventListener:type listener:listener useCapture:useCapture];
>  598 }
> 
> Mail isn't going to be getting at SVGElementInstances, is it?  I guess it could
> unintentioanlly register listeners on svg <use> content?
I would have preferred to leave out these functions, marked as needed for Mail.app, though the DOMEventTarget protocol requires those functions. It won't compile without them.
> 
> Looks sane enough.
Thanks, landed.

[Eric Seidel (no email)]

Comment on attachment 23734 [details]
Patch 1 - v2 - Corrected patch, contained unrelated things.

Clearing review flag since I think this landed.

[Nikolas Zimmermann]

Created attachment 24217 [details]
Several new layout tests, needs attached patch v2

[Nikolas Zimmermann]

Created attachment 24218 [details]
Final patch 

Only took over a year, to finalize <use> but here it is :-)
ChangeLog misses references to this bug, and another one that it fixes, going to post them as comment.

[Nikolas Zimmermann]

Created attachment 24219 [details]
Layout Tests - without large png diffs

[Nikolas Zimmermann]

Created attachment 24220 [details]
Final Patch - with correct ChangeLog

[Nikolas Zimmermann]

*** Bug 20550 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

*** Bug 15430 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

Landed in r37435.