Bug 15413 - SVGElementInstance does not implement EventTarget
Summary: SVGElementInstance does not implement EventTarget
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
: 15430 16934 20550 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-10-07 14:49 PDT by Eric Seidel (no email)
Modified: 2008-10-08 20:35 PDT (History)
2 users (show)

See Also:


Attachments
test case (no browser works, so not fully tested, might be wrong) (1.28 KB, image/svg+xml)
2007-10-07 15:34 PDT, Eric Seidel (no email)
no flags Details
New layout test (10.21 KB, patch)
2007-12-19 04:20 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Initial patch (76.74 KB, patch)
2007-12-19 04:22 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch (107.15 KB, patch)
2008-09-23 14:10 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch 1 - v1: Add (JS)EventTargetSVGElementInstance, don't use it. (233.17 KB, patch)
2008-09-23 15:25 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch 1 - v2 - Corrected patch, contained unrelated things. (52.53 KB, patch)
2008-09-23 15:34 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Several new layout tests, needs attached patch v2 (457.45 KB, patch)
2008-10-08 19:27 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Final patch (115.67 KB, patch)
2008-10-08 19:28 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Layout Tests - without large png diffs (54.27 KB, patch)
2008-10-08 19:58 PDT, Nikolas Zimmermann
sam: review+
Details | Formatted Diff | Diff
Final Patch - with correct ChangeLog (116.13 KB, patch)
2008-10-08 19:59 PDT, Nikolas Zimmermann
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2007-10-07 14:49:20 PDT
SVGElementInstance does not implement EventTarget

The implementation is stubbed out with empty methods.
Comment 1 Eric Seidel (no email) 2007-10-07 15:29:22 PDT
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.
Comment 2 Eric Seidel (no email) 2007-10-07 15:34:56 PDT
Created attachment 16579 [details]
test case (no browser works, so not fully tested, might be wrong)
Comment 3 Nikolas Zimmermann 2007-12-19 04:20:16 PST
Created attachment 17987 [details]
New layout test
Comment 4 Nikolas Zimmermann 2007-12-19 04:22:16 PST
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 5 Eric Seidel (no email) 2007-12-19 07:17:41 PST
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 6 Eric Seidel (no email) 2007-12-19 07:20:46 PST
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.
Comment 7 Eric Seidel (no email) 2007-12-19 07:21:17 PST
I'm flying today, but I can take a look next time I'm hacking.
Comment 8 Eric Seidel (no email) 2007-12-22 18:31:48 PST
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
Comment 9 Eric Seidel (no email) 2007-12-22 18:33:28 PST
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.
Comment 10 Eric Seidel (no email) 2007-12-22 19:15:16 PST
m_targetElementInstance = new EventTargetSVGElementInstance(this, target);
in void SVGUseElement::buildPendingResource()
is the callsite responsible for both copies.  Not yet sure why.
Comment 11 Eric Seidel (no email) 2007-12-22 20:03:25 PST
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

Comment 12 Eric Seidel (no email) 2007-12-27 01:34:17 PST
I would argue that you could land this w/o the currentTarget == target check and file a separate bug to track that issue.
Comment 13 Nikolas Zimmermann 2007-12-27 04:07:38 PST
(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
Comment 14 Nikolas Zimmermann 2008-01-19 02:30:51 PST
*** Bug 16934 has been marked as a duplicate of this bug. ***
Comment 15 Nikolas Zimmermann 2008-09-23 14:10:39 PDT
Created attachment 23722 [details]
Updated patch

Not marking for review yet, one layout test missing, ChangeLog to be written. Just for Eric ;-)
Comment 16 Nikolas Zimmermann 2008-09-23 15:25:03 PDT
Created attachment 23733 [details]
Patch 1 - v1: Add (JS)EventTargetSVGElementInstance, don't use it.
Comment 17 Nikolas Zimmermann 2008-09-23 15:34:14 PDT
Created attachment 23734 [details]
Patch 1 - v2 - Corrected patch, contained unrelated things.
Comment 18 Eric Seidel (no email) 2008-09-23 16:37:18 PDT
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.
Comment 19 Nikolas Zimmermann 2008-09-23 17:09:06 PDT
(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 20 Eric Seidel (no email) 2008-10-06 18:24:23 PDT
Comment on attachment 23734 [details]
Patch 1 - v2 - Corrected patch, contained unrelated things.

Clearing review flag since I think this landed.
Comment 21 Nikolas Zimmermann 2008-10-08 19:27:34 PDT
Created attachment 24217 [details]
Several new layout tests, needs attached patch v2
Comment 22 Nikolas Zimmermann 2008-10-08 19:28:55 PDT
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.
Comment 23 Nikolas Zimmermann 2008-10-08 19:58:59 PDT
Created attachment 24219 [details]
Layout Tests - without large png diffs
Comment 24 Nikolas Zimmermann 2008-10-08 19:59:36 PDT
Created attachment 24220 [details]
Final Patch - with correct ChangeLog
Comment 25 Nikolas Zimmermann 2008-10-08 19:59:40 PDT
*** Bug 20550 has been marked as a duplicate of this bug. ***
Comment 26 Nikolas Zimmermann 2008-10-08 20:00:26 PDT
*** Bug 15430 has been marked as a duplicate of this bug. ***
Comment 27 Nikolas Zimmermann 2008-10-08 20:35:27 PDT
Landed in r37435.