26790 - setting 'src' property on SCRIPT element has no effect

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[ian.paterson]

createElement() can be used to create a SCRIPT element but subsequently setting 
the 'src' property doesn't cause the script to be downloaded/parsed/executed - 
as would have been expected from the DOM1 spec (the src attribute is not 
designated read only).

In addition to DOM1 compliance, this feature would allow web-developers to 
download scripts only on user demand. This is particularly important when 
JavaScript files contain data.

Comment 1

[vidur (gone)]

Related to bug 18843. Moving to the same milestone.

Comment 2

[ckritzer (gone)]

nominating for nsbeta2 based on:
 - visibility
 - feature (DOM Level 1 Standards Compliance) broken

Comment 3

[leger]

Putting on [nsbeta2-] radar.  

Comment 4

[leger]

Putting on nsbeta3 radar.  Is this a dup?

Comment 5

[Mike]

M16 has been out for a while now, these bugs target milestones need to be 
updated.

Comment 6

[ckritzer (gone)]

Changing Status Whiteboard from: 
	[nsbeta2-]investigating - PDT please skip for now - ckritzer
to:
	[nsbeta2-] NEED TESTCASE-ckritzer;


                                                                                                 
[e-mail], could you provide a testcase for this?  Thanks! -
ckritzer

Comment 7

[ckritzer (gone)]

oooooohhhhkay...it didn't take my cahnges last time, so I'll try again...

Comment 8

[Nisheeth Ranjan]

This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you or another
known resource will be working on this bug,or if it blocks your work in some way 
-- please attach your concern to the bug for reconsideration, but do not clear 
the nsbeta3- nomination.

Comment 9

[Jan Carpenter]

Mass update of qa contact

Comment 10

[Erik Arvidsson]

I just wanted to add some comments...

The way that a node is created should NEVER matter. This is one of the corner
stones of DOM in my opinion. If a DOM node has different effects depending on
how it is created IT IS A SERIOU BUG.

When (if?) you fix this bug I hope you also think about adding an event so that
the developer can be notified once the script file is loaded.

A better general solution would be to do something like MS did. MS added a
property to all nodes (elements?) that is called readyState and all elements
also fires an event called "readystatechange".

I really need this bug to be fixed. It is forcing our customers to stick with
NN4 until fixed and we dont want to do that, do we? :-)

Comment 11

[tato]

It is added because it felt like this 26790 looking like my hope.

I want to read URL which is necessary after onload of document to the SRC 
attribute of SCRIPT.
Oj.src=URL or setAttribute('src',URL)

After document is done with onload, scriptObject.src=URL doesn't work with 
Mozilla. However, I think that Mozilla should work it. exactly, like "require" 
of perl. For example, If we can set this src, js which isn't useful isn't read, 
and it branches off, and we can read only necessary js. Incidentally, work on 
WinNN4.x WinIE5 MacIE4.5/IE5 MacNN4.x . 

Sample : 
http://game.gr.jp/chkmoz/09/index.htm
http://game.ncs.gr.jp/~tato/skin/200009/testNNIE4.htm

Comment 12

[hagiwara]

I think so about, too.
Especially, for corss browser and huge script js system

Comment 13

[Prashant Desale]

QA contact Update

Comment 14

[vidur (gone)]

This isn't fixed yet, but 18843 has been. You can load scripts dynamically by
adding a new SCRIPT element with a SRC attribute to the tree.

Comment 15

[Prashant Desale]

Updating QA contact to Shivakiran Tummala.

Comment 16

[Fabian Guisset]

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

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

exactly what part of this bug is still unfixed? I thought the fix for bug 18843 
fixed this too?

Comment 18

[basic]

testcase from bug 88318 is in attachment 63049 [details]

Comment 19

[Sivakiran Tummala]

nominating for nsbeta1, 
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=63049 is the testcase

Comment 20

[/\/\arcio Galli]

Adding me to the cc. I am wondering if we have workarounds: for example
dynamically doing document write for one script element with the src attribute.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

you can always set the src attribute before adding the element to the DOM. That 
should work just fine.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: patch to fix

This patch makes us parse/load the script when
* the element is added to the document
* any attributes get set
* any children are added

However the element is only ever evaluated once

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

taking since i have the patch..

jkeiser, fabian, anybody feel up to reviewing?

Comment 24

[Fabian Guisset]

Code looks good however I have a question..
What is the behavior in the following case:
The script has a src="" attribute, and one appends a child (I assume that's a
text child right? Shouldn't we throw an exception on non-text child nodes?) node
to the script element. What should we do then? Remove the src element thus
making it an inline script? 
By the way, is there a reason why you don't use the standard StringToAttribute()
method, instead of overriding SetAttr()?

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> What is the behavior in the following case:
> The script has a src="" attribute, and one appends a child (I assume that's a
> text child right? Shouldn't we throw an exception on non-text child nodes?)
> node to the script element. What should we do then? Remove the src element
> thus making it an inline script? 

IMHO we shouldn't do any modification that the client hasn't asked for. Adding 
the new childnode will do nothing for two reasons:
* Once the <script> has been evaluated we won't evaluate it ever again.
* If an element has both a src attribute and childNodes the childNodes will not
  be used.

> By the way, is there a reason why you don't use the standard
> StringToAttribute() method, instead of overriding SetAttr()?

StringToAttribute should be used when you need to change the type that the 
attribute is stored as. For example when you want the "width" attribute stored 
as ePixels instead of as a string. 
SetAttr should be used when you want to detect when an attribute is set. Look 
at the code in <input> for another example.

At least that's how I've understood things :)

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> Shouldn't we throw an exception on non-text child nodes?

Not sure, but i'd rather take that separatly if that's the case

Comment 27

[Johnny Stenback (:jst)]

Will this patch give us parity with IE's handling of script elements in the DOM?

Comment 28

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

we won't behave exactly like IE, here's how they behave:

* Childnodes of the <script> are always ignored. I can't find any way to add an
  inline script using standard DOM methods. Adding the childnodes before adding
  the element to the tree does not help.
* Setting the src of an element which is in the tree _always_ loads and
  evaluates the script. Even when setting the src to the same value repeatedly
  without clearing it between
* Adding a <script> with a src to the tree evaluates the script _only once_.
  Removing and readding the element does not reevaluate the script.

Personally I think our way makes more sence, i.e. an element can only be 
evaluated _once_, independently of what triggered that evaluation.

I'll attach the testcase i've used for testing

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: testcase

Comment 30

[Axel Hecht]

Comment on attachment 75803 [details] [diff] [review]
patch to fix

>+  NS_IMETHOD SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
>+                     const nsAReadableString& aValue, PRBool aNotify);
<...>
>+NS_IMETHODIMP 
>+nsHTMLScriptElement::SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
>+                             const nsAReadableString& aValue, PRBool aNotify)

nsAReadableString is dead, that's a |const nsAString&| now

>+  // if the element is in the document and we're creating the attribute

(I'm not saying this.) Remove the comment.
Put a comment before

>+void
>+nsHTMLScriptElement::MaybeProcessScript()

and explain the logic. Note that you don't change a evaluated script, even
if it was a inline script and you're setting @src now.

Fabian, could we get a bug to document this behaviour in the domref?

Jonas, please add your performance estimates.

Comment 31

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: address Axels comments

Comment 32

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

There should be no risk for performance regressions since we don't do any extra 
work during loading and parsing of pages. The only extra code executed is a few 
non-virtual functioncalls which bails immediatly.

Comment 33

[Fabian Guisset]

Pike, bug 133932 filed for the domref. Code looks good to me.

Comment 34

[Fabian Guisset]

sicking, any progress with this?

Comment 35

[Peter Van der Beken [:peterv]]

Comment on attachment 76288 [details] [diff] [review]
address Axels comments

>Index: nsHTMLScriptElement.cpp
>+  /**
>+   * Processes the script if it's in the document-tree and contains, or links
>+   * to, a script. It will never reevaluate an element, once it has been
>+   * evaluated there is no way to make it evaluate again, you'll have to
>+   * create a new element. This includes adding a src attribute to an element
>+   * that contains an inline script, the script will not be loaded.
>+   *
>+   * In order to be able to use multiple childNodes or to use the
>+   * fallback-mechanism of using both inline script and linked script you have
>+   * to add all attributes and childNodes before adding the element to the
>+   * document-tree.
>+   */

>@@ -124,6 +146,7 @@
> nsHTMLScriptElement::nsHTMLScriptElement()
> {
>   mLineNumber = 0;
>+  mIsEvaluated = PR_FALSE;
> }

Change this to:

nsHTMLScriptElement::nsHTMLScriptElement() : mLineNumber(0),
					     mIsEvaluated(PR_FALSE)
{
}

>+  if (NS_SUCCEEDED(rv) && aNotify && aName == nsHTMLAtoms::src &&
>+      aNameSpaceID == kNameSpaceID_None)
>+    MaybeProcessScript();

Please add braces (Like you already do in MaybeProcessScript).

>+  if (NS_SUCCEEDED(rv) && aDocument)
>+    MaybeProcessScript();

Same.

>+  if (NS_SUCCEEDED(rv) && aNotify)
>+    MaybeProcessScript();

Same.

>+  if (NS_SUCCEEDED(rv) && aNotify)
>+    MaybeProcessScript();

Same.

r=peterv. Let's get this in. Jst, please review.

Comment 36

[Peter Van der Beken [:peterv]]

I meant to add this suggestion for changing the comment:

  /**
   * Processes the script if it's in the document-tree and links to or
   * contains a script. Once it has been evaluated there is no way to make it
   * reevaluate the script, you'll have to create a new element. This also means
   * that when adding a src attribute to an element that already contains an
   * inline script, the script referenced by the src attribute will not be
   * loaded.
   *
   * In order to be able to use multiple childNodes, or to use the
   * fallback-mechanism of using both inline script and linked script you have
   * to add all attributes and childNodes before adding the element to the
   * document-tree.
   */

Comment 37

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

ooh, that's a very nice text, will attach a new patch tomorrow-ish

Comment 38

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

...

Comment 39

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: using peterv comment

same code but with petervs comment

Comment 40

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 88202 [details] [diff] [review]
using peterv comment

carry forward r=

Comment 41

[Johnny Stenback (:jst)]

Comment on attachment 88202 [details] [diff] [review]
using peterv comment

Add braces around those new one-line if's, for consistency with the surrounding
code if nothing else.

Comment 42

[Johnny Stenback (:jst)]

Comment on attachment 88202 [details] [diff] [review]
using peterv comment

Add braces around those new one-line if's, for consistency with the surrounding
code if nothing else.

sr=jst

Comment 43

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

checked in

Comment 44

[Sivakiran Tummala]

verified fixed. 11-03-08