|Summary:||Dynamically created SVG script tags fail to execute|
|Severity:||Normal||CC:||eric, mjs, oliver|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.5|
Description jay 2008-06-04 06:48:07 PDT
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.