SVGElementInstance does not implement EventTarget The implementation is stubbed out with empty methods.
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.
Created attachment 16579 [details] test case (no browser works, so not fully tested, might be wrong)
Created attachment 17987 [details] New layout test
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
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.
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.
I'm flying today, but I can take a look next time I'm hacking.
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
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.
m_targetElementInstance = new EventTargetSVGElementInstance(this, target); in void SVGUseElement::buildPendingResource() is the callsite responsible for both copies. Not yet sure why.
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
I would argue that you could land this w/o the currentTarget == target check and file a separate bug to track that issue.
(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
*** Bug 16934 has been marked as a duplicate of this bug. ***
Created attachment 23722 [details] Updated patch Not marking for review yet, one layout test missing, ChangeLog to be written. Just for Eric ;-)
Created attachment 23733 [details] Patch 1 - v1: Add (JS)EventTargetSVGElementInstance, don't use it.
Created attachment 23734 [details] Patch 1 - v2 - Corrected patch, contained unrelated things.
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.
(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.
Comment on attachment 23734 [details] Patch 1 - v2 - Corrected patch, contained unrelated things. Clearing review flag since I think this landed.
Created attachment 24217 [details] Several new layout tests, needs attached patch v2
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.
Created attachment 24219 [details] Layout Tests - without large png diffs
Created attachment 24220 [details] Final Patch - with correct ChangeLog
*** Bug 20550 has been marked as a duplicate of this bug. ***
*** Bug 15430 has been marked as a duplicate of this bug. ***
Landed in r37435.