Bug 16447 – onload called too many times for <svg:image>

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

[Eric Seidel (no email)]

onload called too many times for <svg:image>

We seem to be calling onload multiple times for each <svg:image> object.

[Eric Seidel (no email)]

Created attachment 17911 [details]
test case (uses remote URL)

[Rob Buis]

Created attachment 20363 [details]
First attempt

There were two problems here. First, that load was dispatched both when image was loaded and tag was closed. Second, that image load was dispatched to parents too, while the spec says:

A SVGLoad event is dispatched only to the element to which the event applies; it is not dispatched to its ancestors.

This patch fixes that. The testcase works, just not sure if external image is allowed, can change it to use a local image if needed.
Cheers,

Rob.

[Eric Seidel (no email)]

Bah.  Unfortunately we need to better understand what proper SVGLoad event handling is before we can really make any changes to this code.  To do so, we need at least 3 (possibly 4) test cases, as described below:

1. normal loading, sign up for all listeners, make sure they are in order (a single capture listener on the top element should work)
2.  loading with externalResourcesRequired, make sure it delays all parents, but not siblings
3. using display: none, possibly (#4?) in combination with externalResoucesRequired

Your attached patch removes code which was originally written to handle the externalResoucesRequired delay case.  However, I'm not sure if <svg:image> sends its Load event right after tag close, or after image load?  The spec is somewhat vague:
http://www.w3.org/TR/SVG11/interact.html#LoadEvent
The event is triggered at the point at which the user agent has fully parsed the element and its descendants and is ready to act appropriately upon that element, such as being ready to render the element to the target device. Referenced external resources that are required must be loaded, parsed and ready to render before the event is triggered. Optional external resources are not required to be ready for the event to be triggered.

I think <image> has sorta an implicit "external resources required" on it, and thus delays its load event and all parent load events until its image is loaded, and thus it's ready for display.

http://www.w3.org/TR/SVG11/struct.html#ExternalResourcesRequired
externalResourcesRequired=true
Indicates that resources external to the current document are required. If an external resource is not available, progressive rendering is suspended, the document's SVGLoad event is not fired and the animation timeline does not begin until that resource and all other required resources become available, have been parsed and are ready to be rendered. If a timeout event occurs on a required resource, then the document goes into an error state (see Error processing). The document remains in an error state until all required resources become available.

[Eric Seidel (no email)]

Comment on attachment 20363 [details]
First attempt

I think we need more test cases (see comments above) before we can really evaluate this patch.

[Rob Buis]

Created attachment 20490 [details]
Better approach

This patch keeps the intend of the current code, delaying parent load dispatching until the image is loaded for externalResourcesRequired=true. Also it makes sure the event is dispatched to the parent node, the current code used "this"! With the new checks there are no multiple load dispatches.


Comparing with other implementations is a bit hard; Opera does not seem to support externalResourcesRequired, saying:

Failed attribute on image element: externalResourcesRequired="true".

Both and FireFox and Opera just seem to dispatch the load for the image right
after parsing the tag, but I don't think this is what the spec indicates.

The malloc avoiding code is just an idea I have, and could be dealt with separately, though while messing with the load code I think it is convenient
to attack that problem too.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 20490 [details]
Better approach

Nice!

I don't think static RefPtr is "safe".  the Event object would end up shared between frames, and would be unsafe.  Data could survive on the JSEvent wrapper too (since those are generally cached based on pointer).

I think that <image> should delay its onload until after its resources are loaded and ready to render anyway.  I'm not sure what that means for externalResourcesRequired on <image> then...

r- for the Event sharing issue.

[Rob Buis]

Created attachment 20491 [details]
Fix the "avoid malloc" part

This patch introduces hasListenerType, could also be called hasListenerForType
and/or moved into EventTargetNode.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 20491 [details]
Fix the "avoid malloc" part

Two comments:

1.  I think SVG Load events might support capture, in which case you need to dispatch the event anyway (unless you walk up the parent chain and check to see if any parent has a capturing load event listener (which is what the comment is about).

Also, 
hasListenerType(const AtomicString &eventType)
we put the & next to AtomicString, per our style guidelines.

Since you've pointed out yet another case that our testing missed, you might want to write a test case to verify that we correctly support capturing load event listeners (since this patch would have broken that).

r- for the broken event capture support.

[Rob Buis]

Created attachment 20492 [details]
Fix the "avoid malloc" part part II

This patch tries to do the ancestor capture listener check and adds a testcase for it, where it verifies that the capturing listener on the document element receives
all load events.
Cheers,

Rob.

[Nikolas Zimmermann]

Comment on attachment 20492 [details]
Fix the "avoid malloc" part part II

Looks nice. From a first glance, it seems all issues Eric menioned, are fixed, right? In  that case r=me.

[Rob Buis]

Hi Niko,

(In reply to comment #10)
> (From update of attachment 20492 [details] [edit])
> Looks nice. From a first glance, it seems all issues Eric menioned, are fixed,
> right? In  that case r=me.

Thnx for the r+!
Eric had some issues about a method name IIRC, I'll try to contact him about that.
Mfg,

Rob.

[Eric Seidel (no email)]

The change looks fine.

My comment over IRC was simply that:
hasLoadListener

doesn't actually do what it says.  It's more:
hasListenerForLoadEventTarget(element)
(although that's kinda klunky name)

The real fix of course is to fix the event system to use "EventPromise" stack-elements.  I think this fix is fine until Darin or someone follows through with adding an EventPromise.

Event promise is a stack-allocated object which knows how to malloc an Event with the right information, but only does so when required.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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

[Rob Buis]

Landed in r32738.