setting 'src' property on SCRIPT element has no effect

VERIFIED FIXED in mozilla1.1beta

Status

()

Core
DOM: Core & HTML
P3
normal
VERIFIED FIXED
18 years ago
4 years ago

People

(Reporter: ian.paterson, Assigned: sicking)

Tracking

({dom1, testcase})

Trunk
mozilla1.1beta
x86
Windows 98
dom1, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nsbeta2-][nsbeta3-] NEED TESTCASE-ckritzer;)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

18 years ago
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.

Updated

18 years ago
QA Contact: gerardok → ckritzer

Comment 1

18 years ago
Related to bug 18843. Moving to the same milestone.
Status: NEW → ASSIGNED
Target Milestone: M16

Comment 2

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

Updated

18 years ago
Whiteboard: investigating - PDT please skip for now - ckritzer

Comment 3

18 years ago
Putting on [nsbeta2-] radar.  
Whiteboard: investigating - PDT please skip for now - ckritzer → [nsbeta2-]investigating - PDT please skip for now - ckritzer

Comment 4

18 years ago
Putting on nsbeta3 radar.  Is this a dup?
Keywords: nsbeta3

Comment 5

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

Comment 6

18 years ago
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

Comment 7

18 years ago
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;

Comment 8

17 years ago
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

Comment 9

17 years ago
Mass update of qa contact
QA Contact: ckritzer → janc

Comment 10

17 years ago
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

17 years ago
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

17 years ago
I think so about, too.
Especially, for corss browser and huge script js system
Keywords: dom1
Component: DOM Level 1 → DOM HTML

Comment 13

17 years ago
QA contact Update
QA Contact: janc → desale

Comment 14

17 years ago
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

17 years ago
Updating QA contact to Shivakiran Tummala.
QA Contact: desale → stummala

Comment 16

16 years ago
*** 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?

Comment 18

16 years ago
testcase from bug 88318 is in attachment 63049 [details]

Comment 19

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

Comment 20

16 years ago
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.
Created attachment 75803 [details] [diff] [review]
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
taking since i have the patch..

jkeiser, fabian, anybody feel up to reviewing?
Assignee: vidur → sicking
Status: ASSIGNED → NEW
Target Milestone: Future → mozilla1.0

Comment 24

16 years ago
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
Created attachment 75991 [details]
testcase

Comment 30

16 years ago
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+
Created attachment 76288 [details] [diff] [review]
address Axels comments
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.
Attachment #76288 - Flags: review+
Target Milestone: mozilla1.0 → mozilla1.1alpha

Comment 33

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

Comment 34

16 years ago
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
Created attachment 88202 [details] [diff] [review]
using peterv comment

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
Last Resolved: 16 years ago
Resolution: --- → FIXED

Updated

16 years ago
Blocks: 153600

Comment 44

15 years ago
verified fixed. 11-03-08
Status: RESOLVED → VERIFIED

Updated

9 years ago
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.