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
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.
Created attachment 2365 [details]
Here's a patch that makes the test cases pass and also adds getters and setters
for the text property.
Comment on attachment 2365 [details]
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
6) The URL parameter to evaluateScript should be a const QString &, rather than
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.
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
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.
Created attachment 2397 [details]
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 on attachment 2397 [details]
I think this is good. Two ideas to make it even better:
1) Put the NodeImpl *n declaration inside the for statement in
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() ==
(I also mentioned to Anders on IRC that we need this text/setText change for
Created attachment 2403 [details]
Here's a new patch that addresses the issues in the previous comment.
Created attachment 2404 [details]
And here's a version that actually compiles.
Comment on attachment 2403 [details]
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.
Created attachment 2405 [details]
Support setting title.text
Sorry to cause more bugzilla spam, but here's a patch that supports setting and
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 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.
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.
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).
*** Bug 3501 has been marked as a duplicate of this bug. ***
Reporter, please mark this bug as verified if it has been fixed for you.
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 )
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)
var oHead = document.getElementsByTagName('head');
var oScript = document.createElement('script');
oHead.appendChild(oScript); // append to head
oScript.onload = callAfterLoad;
OK, then how about a new bug? Lets not keep using a single bug report for all sorts of different related
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.
(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!
This bug is now also In Radar <rdar://4470861>