Bug 16445 – Refactor EventTargetNode & JSEventTargetNode for an upcoming SVG patch

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

To fix bug 15413 (SVGElementInstance does not implement EventTarget) we need to introduce a new class called EventTargetSVGElementInstance inheriting from EventTarget & SVGElementInstance - just like EventTargetNode works.

Some information regarding SVGElementInstance & EventTarget:
SVG spec defines how event handling works for <use> elements and their corresponding SVGElementInstance objects - in particular it says that SVGElementInstance and the element it references share an event listener list. In our SVG implementation SVGElementInstance contains a pointer to the original referenced element as well as it's corresponding shadow tree element. The goal is that SVGUseElement now creates EventTargetSVGElementInstance objects instead of pure SVGElementInstance objects and installs any event listeners added through markup and/or JS on the shadow tree element! This way we can share code using EventTargetNode, as the only difference for EventTargetSVGElementInstance is the origin where it stores it's event listeners.

Example: <defs><rect id="rect"../> <use xlink:href="url(#rect)" id="test"/>
useElement = document.getElementById("test");
rectElement = document.getElementById("rect");

Case #1: someUseElement.instanceRoot.addEventListener(...)
calls 'addEventListener' on the 'EventTargetSVGElementInstance' object - which in turn install an event listener on the shadow tree element.

Case #2:  rectElement.addEventListener(...)
calls 'addEventListener ' on the 'EventTargetNode' object. Altering the event listeners will result in an update of the SVGElementInstances which belong to the 'rectElement' - this will reclone the shadow tree elements -> this way everything stays synchronized.

As it's possible to share a lot of code with the new (JS)EventTargetSVGElementInstance classes - I refactored the EventTargetNode & JSEventTargetNode class into base classes. JSEventTargetNode now forwards most calls to EventTargetNode - same for JSEventTargetSVGElementInstance.

This patch only contains the refactorization of the EventTargetNode classes - no SVG stuff contained here.

[Nikolas Zimmermann]

Created attachment 17900 [details]
Initial patch

[Oliver Hunt]

Okay, so far this looks sane, although i'd like maciej and possibly hyatt to look at it as well.

My only real concern atm is that a number of the new files use large quantities of code from other files, so the appropriate copyrights should be moved across as well.

[Nikolas Zimmermann]

(In reply to comment #2)
> Okay, so far this looks sane, although i'd like maciej and possibly hyatt to
> look at it as well.
> 
> My only real concern atm is that a number of the new files use large quantities
> of code from other files, so the appropriate copyrights should be moved across
> as well.
> 
I started of with fresh header templates and forgot about them - readded all existing copyrights in the approriate files (all files in bindings/js for instance).

Niko

[Nikolas Zimmermann]

Created attachment 17901 [details]
Updated patch

New patch after discussion with Maciej - remove the two callbacks and the JSEventTarget* owner pointer from JSEventTargetBase to save size. We can now store JSEventTargetBase by value in JSEventTargetNode (& JSEventTargetSVGElementInstance).

Fixed copyright as noticed by Oliver.

[Alexey Proskuryakov]

When reading the spec, I thought that a more natural approach would be to tweak event dispatching, not the elements themselves. It seems unfortunate that listener lists have to be kept in sync - as a general rule, everything that needs to be kept in sync will get out of sync sooner or later.

I.e. when dispatching an event to SVGElementInstance, dispatchGenericEvent() would take listener list from  the original, and so would addEventListener/removeEventListener.

Does this make any sense? Or did I just misunderstand the desired behavior?

[Nikolas Zimmermann]

(In reply to comment #5)
> When reading the spec, I thought that a more natural approach would be to tweak
> event dispatching, not the elements themselves. It seems unfortunate that
> listener lists have to be kept in sync - as a general rule, everything that
> needs to be kept in sync will get out of sync sooner or later.
> 
> I.e. when dispatching an event to SVGElementInstance, dispatchGenericEvent()
> would take listener list from  the original, and so would
> addEventListener/removeEventListener.
> 
> Does this make any sense? Or did I just misunderstand the desired behavior?

Hi Alexey, thanks for taking time to check this approach.

It will be easier to discuss once this patch is in, and I actually uploaded the follow-up SVG patch.
The idea is that EventTargetSVGElementInstance, addEventListener/removeEventListener/dispatchEvent etc.. call EventTarget::addEventListener(correspondingElement(), ...) which means that any events dispatched to an element, which gets directly or indirectly referenced by <use> will be intercepted in dispatchGenericEvent() and their 'target' & 'currentTarget' property will be modified, to point to the SVGElementInstance objects. So what you are describing above, seems perfectly fine.

Greetings,
Niko

[Nikolas Zimmermann]

Rereading my patch, I have some fears regarding s_prototypeName initialization.
Anyone has a suggestion for a nicer way  not involving a static const char*?

Greetings,
Niko

[Nikolas Zimmermann]

Created attachment 17915 [details]
Updated patch v2

Incorporated Erics suggestions:
- create helper method to convert from JSEventTargetProperties tokens to AtomicString to simplify getListener/setListener implementation
- Better way to handle s_prototypeName (better, as in actually working reliable!)

No regressions.

[Eric Seidel (no email)]

Comment on attachment 17915 [details]
Updated patch v2

I think this needs at least one more round for code clarity:


WildFox: your indenting is kinda strange in JSEventTargetPrototypeInformation

WildFox: IMO the big +    if (eventType == DOMSubtreeModifiedEvent) if-chain should be abstracted into a static inline funtion
type = listenerTypeForEventType(eventType)
WildFox: actually, better would probably be shouldEnableSpecialListenerTypeOnDocument(eventType, type)
then you can combine it with the next if
something like that

WildFox: you also seem to have done a search replace of /listen//
WildFox: +        // We do want capturing event ers to be invoked here, even though
WildFox: you should probably search for " ers"

WildFox: if you're moving the code anyway, you shoudl clean up the "why don't we recalc the node chain here" comment
which is about 15 lines long and only needs to be 2 :)
"We don't recalc the node chain here because she spec says so:"
[spec quote]

WildFox: all of your:
+#if ENABLE(SVG)
+        evt->setCurrentTarget(applySVGEventTargetRules(eventTargetNode, evt.get()));
blocks shoudl just change to a single static iline
setCurrentTargetRespectingSVGTargetRules(evt, eventTargetNode)
WildFox: and then you can have the special logic all in one plac,e instead of lots of copy/paste #if ENABLE code

[Nikolas Zimmermann]

Created attachment 17917 [details]
Updated patch v3

Updated, fixing Erics comments.

[Eric Seidel (no email)]

Comment on attachment 17917 [details]
Updated patch v3

Looks good.

+    // Remove existing identical er set with identical arguments.

Still need to do a search for " er" and fix those to " listener". :)

dispatchGenericEvent is horribly long, we'll eventually need to break it up into more static inlines.

Looks good.

[Nikolas Zimmermann]

Landed in r28756 with Erics last comment fixed.