Bug 3526

Summary: improve support for dynamically added <script> elements
Product: WebKit Reporter: Sjoerd Mulder <sjoerdmulder>
Component: JavaScriptCoreAssignee: Darin Adler <darin>
Status: VERIFIED FIXED    
Severity: Major CC: andersca, mfa
Priority: P2    
Version: 312.x   
Hardware: Mac   
OS: OS X 10.3   
URL: http://oege.ie.hva.nl/~mulder1p/JS_Add_Bug/
Attachments:
Description Flags
Fix
darin: review-
Another test case
none
Better patch
darin: review+
Better patch
darin: review+
Compilable patch
darin: review+
Support setting title.text mjs: review-

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
The adding of the scripts works well now :), but in Mozilla you can do:

oScript.onload = functionCallBack;

The onload event is never triggered when the script load is completed(on TOT)
(by setting the src attribute)

Test:

function callAfterLoad(){
alert('Script Loaded');
}

var oHead = document.getElementsByTagName('head')[0];
var oScript = document.createElement('script');

oScript.setAttribute('type', 'text/javascript');
oHead.appendChild(oScript);         // append to head

oScript.onload = callAfterLoad;
oScript.setAttribute('src', 'myfiletoload.js');
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!
Comment 23 Sjoerd Mulder 2006-03-08 09:11:25 PST
This bug is now also In Radar <rdar://4470861>