|Summary:||improve support for dynamically added <script> elements|
|Product:||WebKit||Reporter:||Sjoerd Mulder <sjoerdmulder>|
|OS:||OS X 10.3|
Description Sjoerd Mulder 2005-06-14 06:37:25 PDT
When creating a script-element with document.createElement and inserting this element into the DOM, the attached script is not executed or not loaded at all. This has been tested on IE6, Firefox 1.0.4 and Opera 8, these browsers all work. Tested Safari version: 1.3(v312). Testcase is provided at URL
Comment 1 Chris Petersen 2005-06-14 13:06:13 PDT
Interesting results: With Safari 2.0 (v412): All tests fail With TOT Webkit under 10.4.1 (8B15): Tests 1 - 3 pass but tests 4- 6 fail With Mac Firefox 1.0.4: All tests pass.
Comment 2 Anders Carlsson 2005-06-15 09:20:08 PDT
Created attachment 2365 [details] Fix Here's a patch that makes the test cases pass and also adds getters and setters for the text property.
Comment 3 Darin Adler 2005-06-16 09:51:21 PDT
Comment on attachment 2365 [details] Fix This patch looks pretty good. I have a few comments: 1) The !scriptString.isEmpty() check needs a comment. Anders explained to me why it's needed, and the code needs that explanation too. 2) The code needs formatting fixes; there are function declarations with extra spaces before and after ) and ( characters. There are also commas that don't have commas after them. 3) I'd like to understand why the insertBefore and appendChild methods look at the exceptioncode and newChild rather than looking at the result. Perhaps an "if (result)" is a better check? 4) Is there a good reason this can't be done inside the childrenChanged() function instead of individually overriding insertBefore and appendChild? Can we enhance childrenChanged() so it can be used? I'm not entirely comfortable with the precedent of overriding both of these functions. 5) I believe HTMLScriptElementImpl::setText could be implemented better by using removeChildren(). 6) The URL parameter to evaluateScript should be a const QString &, rather than QString. Looks like this is on the right track, although I still don't fully understand the rules for when the script should be run and re-run.
Comment 4 Niels Leenheer (HTML5test) 2005-06-16 11:08:35 PDT
5) I also implemented an alternative for this function for bug 3501, which does use removeChildren() and is also for HTML by simply reusing of a pre-existing text node. http://bugzilla.opendarwin.org/attachment.cgi?id=2368
Comment 5 Anders Carlsson 2005-06-16 11:40:54 PDT
Created attachment 2392 [details] Another test case Here's a test case with a couple of different test cases. WinIE does not allow adding children to the script node, only .text and .innerText works there, all tests pass on Mozilla though. I'll try to whip up a better patch that uses childrenChanged.
Comment 6 Anders Carlsson 2005-06-16 13:12:27 PDT
Created attachment 2397 [details] Better patch Here's a new patch that addresses the comments. I tried the previously attached test case in Opera and it always re-ran the script when the children changed. This patch still does what Mozilla and WinIE does, but it doesn't allow you to evaluate a script by appending a child node to a script node that's already in the document.
Comment 7 Darin Adler 2005-06-16 14:40:09 PDT
Comment on attachment 2397 [details] Better patch I think this is good. Two ideas to make it even better: 1) Put the NodeImpl *n declaration inside the for statement in HTMLScriptElementImpl::text. 2) HTMLScriptElementImpl::childrenChanged could check firstChild() instead of childNodeCount(), since that's faster (maybe we should have a hasChildren()). 3) HTMLScriptElementImpl::setText should do isTextNode instead of nodeType() == TEXT_NODE. Otherwise, super-great! (I also mentioned to Anders on IRC that we need this text/setText change for title elements.) r=me
Comment 8 Anders Carlsson 2005-06-16 14:58:13 PDT
Created attachment 2403 [details] Better patch Here's a new patch that addresses the issues in the previous comment.
Comment 9 Anders Carlsson 2005-06-16 15:05:21 PDT
Created attachment 2404 [details] Compilable patch And here's a version that actually compiles.
Comment 10 Darin Adler 2005-06-16 15:07:02 PDT
Comment on attachment 2403 [details] Better patch This won't compile. It's ->isTextNode(), not nodeType()->isTextNode(). But I'm saying r=me anyway. Just make sure whatever we check in does compile.
Comment 11 Anders Carlsson 2005-06-16 15:13:08 PDT
Created attachment 2405 [details] Support setting title.text Sorry to cause more bugzilla spam, but here's a patch that supports setting and getting title.text
Comment 12 Maciej Stachowiak 2005-06-16 17:50:58 PDT
Could you please make a separate patch with the title stuff (ideally with a test, if it's testable)? It makes life simpler to have one patch per issue. It also makes life easier if the test case is included in the patch in the right subdir of layout tests - not sure if you can make patches that add files against anoncvs, but if so, that would be handy.
Comment 13 Maciej Stachowiak 2005-06-16 17:52:02 PDT
Comment on attachment 2405 [details] Support setting title.text review- due to unrelated title change being included in the patch, and lack of test case for the title stuff.
Comment 14 Anders Carlsson 2005-06-17 00:30:22 PDT
I removed the obsolete flag from attachment 2404 [details] because it's the final patch but without the title attribute. I opened a separate bug, 3586, for the title change.
Comment 15 Darin Adler 2005-06-18 22:17:54 PDT
I changed the title to reflect the fact that we already had this dynamically-added <script> support in TOT. What we're doing now is updating it to work better (which basically means more like Gecko at this point).
Comment 16 Anders Carlsson 2005-06-19 23:10:22 PDT
*** Bug 3501 has been marked as a duplicate of this bug. ***
Comment 17 Joost de Valk (AlthA) 2005-07-03 08:03:41 PDT
Reporter, please mark this bug as verified if it has been fixed for you.
Comment 18 Sjoerd Mulder 2005-07-04 01:01:05 PDT
I wish i could verify the bug but, i cant compile it yet, still waiting on my new imac( now got a 500 g3 testmachine :( and no Developer Tools )
Comment 19 Sjoerd Mulder 2005-08-25 06:43:55 PDT
Comment 20 Darin Adler 2005-08-25 09:36:28 PDT
OK, then how about a new bug? Lets not keep using a single bug report for all sorts of different related fixes.
Comment 21 Darin Adler 2005-08-25 09:37:11 PDT
In other words, please write a bug about the fact that we don't support "onload' handlers on script tags. That's separate from what was fixed here.
Comment 22 Sjoerd Mulder 2005-08-25 10:46:21 PDT
(In reply to comment #15) Sorry for my new bug, but as the title was changed i thought that this was a general bug for 'improving dynamic script adding'. The original bug is solved!