Bug 3526 – improve support for dynamically added <script> elements

• Component: JavaScriptCore
• Status: VERIFIED FIXED

[Sjoerd Mulder]

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

[Chris Petersen]

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.

[Anders Carlsson]

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.

[Darin Adler]

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.

[Niels Leenheer (HTML5test)]

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

[Anders Carlsson]

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.

[Anders Carlsson]

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.

[Darin Adler]

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

[Anders Carlsson]

Created attachment 2403 [details]
Better patch

Here's a new patch that addresses the issues in the previous comment.

[Anders Carlsson]

Created attachment 2404 [details]
Compilable patch

And here's a version that actually compiles.

[Darin Adler]

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.

[Anders Carlsson]

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

[Maciej Stachowiak]

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.

[Maciej Stachowiak]

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.

[Anders Carlsson]

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.

[Darin Adler]

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).

[Anders Carlsson]

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

[Joost de Valk (AlthA)]

Reporter, please mark this bug as verified if it has been fixed for you.

[Sjoerd Mulder]

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 )

[Sjoerd Mulder]

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');

[Darin Adler]

OK, then how about a new bug? Lets not keep using a single bug report for all sorts of different related 
fixes.

[Darin Adler]

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.

[Sjoerd Mulder]

(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!

[Sjoerd Mulder]

This bug is now also In Radar <rdar://4470861>