Closed Bug 26790 Opened 21 years ago Closed 19 years ago

setting 'src' property on SCRIPT element has no effect

Categories

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

x86
Windows 98
defect

Tracking

()

VERIFIED FIXED
mozilla1.1beta

People

(Reporter: ian.paterson, Assigned: sicking)

References

Details

(Keywords: dom1, testcase, Whiteboard: [nsbeta2-][nsbeta3-] NEED TESTCASE-ckritzer;)

Attachments

(2 files, 2 obsolete files)

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.
QA Contact: gerardok → ckritzer
Related to bug 18843. Moving to the same milestone.
Status: NEW → ASSIGNED
Target Milestone: M16
nominating for nsbeta2 based on:
 - visibility
 - feature (DOM Level 1 Standards Compliance) broken
Keywords: nsbeta2
Whiteboard: investigating - PDT please skip for now - ckritzer
Putting on [nsbeta2-] radar.  
Whiteboard: investigating - PDT please skip for now - ckritzer → [nsbeta2-]investigating - PDT please skip for now - ckritzer
Putting on nsbeta3 radar.  Is this a dup?
Keywords: nsbeta3
M16 has been out for a while now, these bugs target milestones need to be 
updated.
Changing Status Whiteboard from: 
	[nsbeta2-]investigating - PDT please skip for now - ckritzer
to:
	[nsbeta2-] NEED TESTCASE-ckritzer;


                                                                                                 
ian.paterson@clientside.co.uk, could you provide a testcase for this?  Thanks! -
ckritzer
oooooohhhhkay...it didn't take my cahnges last time, so I'll try again...
Whiteboard: [nsbeta2-]investigating - PDT please skip for now - ckritzer → [nsbeta2-] NEED TESTCASE-ckritzer;
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.
Whiteboard: [nsbeta2-] NEED TESTCASE-ckritzer; → [nsbeta2-][nsbeta3-] NEED TESTCASE-ckritzer;
Target Milestone: M16 → Future
Mass update of qa contact
QA Contact: ckritzer → janc
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? :-)
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
I think so about, too.
Especially, for corss browser and huge script js system
Keywords: dom1
Component: DOM Level 1 → DOM HTML
QA contact Update
QA Contact: janc → desale
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.
Updating QA contact to Shivakiran Tummala.
QA Contact: desale → stummala
*** Bug 88318 has been marked as a duplicate of this bug. ***
exactly what part of this bug is still unfixed? I thought the fix for bug 18843 
fixed this too?
testcase from bug 88318 is in attachment 63049 [details]
nominating for nsbeta1, 
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=63049 is the testcase
Keywords: nsbeta1, testcase
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.
you can always set the src attribute before adding the element to the DOM. That 
should work just fine.
Attached patch patch to fix (obsolete) — Splinter Review
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
taking since i have the patch..

jkeiser, fabian, anybody feel up to reviewing?
Assignee: vidur → sicking
Status: ASSIGNED → NEW
Target Milestone: Future → mozilla1.0
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()?
> 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 :)
Status: NEW → ASSIGNED
> 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
Will this patch give us parity with IE's handling of script elements in the DOM?
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 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.
Attachment #75803 - Flags: review+
Attached patch address Axels comments (obsolete) — Splinter Review
Attachment #75803 - Attachment is obsolete: true
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.
Target Milestone: mozilla1.0 → mozilla1.1alpha
Pike, bug 133932 filed for the domref. Code looks good to me.
sicking, any progress with this?
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.
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.
   */
ooh, that's a very nice text, will attach a new patch tomorrow-ish
...
Target Milestone: mozilla1.1alpha → mozilla1.1beta
same code but with petervs comment
Attachment #76288 - Attachment is obsolete: true
Comment on attachment 88202 [details] [diff] [review]
using peterv comment

carry forward r=
Attachment #88202 - Flags: review+
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.
Attachment #88202 - Flags: superreview+
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
checked in
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Blocks: 153600
verified fixed. 11-03-08
Status: RESOLVED → VERIFIED
Component: DOM: HTML → DOM: Core & HTML
QA Contact: stummala → general
You need to log in before you can comment on or make changes to this bug.