Bug 19391

Summary: Dynamically created SVG script tags fail to execute
Product: WebKit Reporter: jay <jay>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: eric, mjs, oliver
Priority: P2 Keywords: HasReduction
Version: 528+ (Nightly build)   
Hardware: Macintosh   
OS: OS X 10.5   
URL: http://www.openicon.org/temp/safari-phpJS-broken.svg
Attachments:
Description Flags
First attempt
oliver: review-
Some more tests eric: review+

Description jay 2008-06-04 06:48:07 PDT
open the uri, after three seconds, alternative text should be loaded from javascript in external php file.

parity: ff & Opera
Comment 1 jay 2008-06-04 06:49:22 PDT
it maybe this issue is not solely related to SVG...
Comment 2 Oliver Hunt 2008-06-05 01:58:17 PDT
Okay this issue is caused by SVGScriptElement not implementing insertedIntoDocument and related-fu, which means that they never execute once they're inserted into the dom.  In fact the current implementation appears to do nothing but manage scripts that are referenced at parse time -- dynamically updating the href tag also does not appear to do the required update logic.
Comment 3 Rob Buis 2008-07-12 13:33:30 PDT
Created attachment 22258 [details]
First attempt

This patch is not finished since it doesnt handle for instance "childrenChanged".
Also I am not sure how to exactly dispatch the error and load events. Still there
are two nice testcases that already are getting passed, so I thought I'd try to get some early feedback :)
Cheers,

Rob.
Comment 4 jay 2008-07-12 14:26:24 PDT
in a fit of pique, well more absent-mindedness it seems I had broken the reduced testcase...

it's temporarily fixed, so grab it while you can...

tx again

~:"
Comment 5 Oliver Hunt 2008-07-12 14:40:38 PDT
Comment on attachment 22258 [details]
First attempt

One observation: SVGScriptElement::evaluateScript has commented out code.

However i don't believe this is correct in all cases.  the HTMLScriptElement has logic to prevent a script from being executed multiple times when attached and detached.

I'm wondering if it's worth making a ScriptElement class that has all the logic to handle attah/detach/execution, etc, although that will require multiple inheritance which will be yuck

and may screw html performance :-/
Comment 6 Rob Buis 2008-07-13 04:52:51 PDT
Created attachment 22259 [details]
Some more tests

This patch also tries setting the href attr dynamically and changing the text
children of the script. The behaviour is the same as for html scripts since
I noticed FF handling the svg scripts that way.
Cheers,

Rob.
Comment 7 Oliver Hunt 2008-07-14 05:14:38 PDT
Comment on attachment 22259 [details]
Some more tests

I'd like to see a patch with finishParsingChildren implemented as htmlscriptelement has, or a reason it shouldn't be implemented for svg.

Also i'd like to see tests for
* script element created, added, removed, and added again
* script element in source that is removed the added again
* script element created, added, removed, children added, re-added to the document
* ditto for a script in the source
* script created, given a source, added, removed, soure attr removed
* script created, given a source, added, removed, soure attr removed, source placed in the element, then added to the document again.
* script element (both created, and in the source) getting its source set, and changed

I think that covers it
Comment 8 Oliver Hunt 2008-07-20 14:18:24 PDT
Rob, did you ever get round to writing those tests?
Comment 9 Rob Buis 2008-07-20 22:58:35 PDT
Hi Oliver,

(In reply to comment #8)
> Rob, did you ever get round to writing those tests?

Still working on it once in a while. It's quite boring work actually :(
Nevertheless I'll have something to review this week, until then you can r-
if you want to clean the review queue a bit more.
Cheers,

Rob.
Comment 10 Oliver Hunt 2008-07-20 23:16:31 PDT
Well the patch itself is fine, it just needs a few more tests.  Hmm, you can commit, right?
Comment 11 Eric Seidel (no email) 2008-07-20 23:48:05 PDT
Yes, Rob can commit & review! :)
Comment 12 Eric Seidel (no email) 2008-07-24 11:49:36 PDT
Comment on attachment 22259 [details]
Some more tests

I think it's better to be explicit about what scriptTag it is in XMLTokenizer, I would prefer you leave the HTMLNames:: when the next line is SVGNames::scriptTag. :)

Looks fine.  Wow, I wish that SVGScriptElement and HTMLScriptElement could share more code.  I think our current split solution is going to lead to lots of copy/paste bugs.
Comment 13 Nikolas Zimmermann 2008-08-13 12:03:04 PDT

*** This bug has been marked as a duplicate of 20372 ***