Closed
Bug 26790
Opened 25 years ago
Closed 23 years ago
setting 'src' property on SCRIPT element has no effect
Categories
(Core :: DOM: Core & HTML, defect, P3)
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)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
sicking
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
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•25 years ago
|
||
Related to bug 18843. Moving to the same milestone.
Status: NEW → ASSIGNED
Target Milestone: M16
Comment 2•25 years ago
|
||
nominating for nsbeta2 based on:
- visibility
- feature (DOM Level 1 Standards Compliance) broken
Keywords: nsbeta2
Updated•25 years ago
|
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
M16 has been out for a while now, these bugs target milestones need to be
updated.
Comment 6•25 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•25 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•25 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 10•24 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•24 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•24 years ago
|
||
I think so about, too.
Especially, for corss browser and huge script js system
Updated•24 years ago
|
Component: DOM Level 1 → DOM HTML
Comment 14•24 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 16•23 years ago
|
||
*** Bug 88318 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 17•23 years ago
|
||
exactly what part of this bug is still unfixed? I thought the fix for bug 18843
fixed this too?
Comment 18•23 years ago
|
||
testcase from bug 88318 is in attachment 63049 [details]
Comment 19•23 years ago
|
||
nominating for nsbeta1,
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=63049 is the testcase
Comment 20•23 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.
Assignee | ||
Comment 21•23 years ago
|
||
you can always set the src attribute before adding the element to the DOM. That
should work just fine.
Assignee | ||
Comment 22•23 years ago
|
||
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
Assignee | ||
Comment 23•23 years ago
|
||
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•23 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()?
Assignee | ||
Comment 25•23 years ago
|
||
> 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
Assignee | ||
Comment 26•23 years ago
|
||
> 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•23 years ago
|
||
Will this patch give us parity with IE's handling of script elements in the DOM?
Assignee | ||
Comment 28•23 years ago
|
||
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
Assignee | ||
Comment 29•23 years ago
|
||
Comment 30•23 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+
Assignee | ||
Comment 31•23 years ago
|
||
Attachment #75803 -
Attachment is obsolete: true
Assignee | ||
Comment 32•23 years ago
|
||
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.
Assignee | ||
Updated•23 years ago
|
Attachment #76288 -
Flags: review+
Assignee | ||
Updated•23 years ago
|
Target Milestone: mozilla1.0 → mozilla1.1alpha
Comment 33•23 years ago
|
||
Pike, bug 133932 filed for the domref. Code looks good to me.
Comment 34•23 years ago
|
||
sicking, any progress with this?
Comment 35•23 years ago
|
||
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•23 years ago
|
||
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.
*/
Assignee | ||
Comment 37•23 years ago
|
||
ooh, that's a very nice text, will attach a new patch tomorrow-ish
Assignee | ||
Comment 39•23 years ago
|
||
same code but with petervs comment
Attachment #76288 -
Attachment is obsolete: true
Assignee | ||
Comment 40•23 years ago
|
||
Comment on attachment 88202 [details] [diff] [review]
using peterv comment
carry forward r=
Attachment #88202 -
Flags: review+
Comment 41•23 years ago
|
||
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 42•23 years ago
|
||
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
Assignee | ||
Comment 43•23 years ago
|
||
checked in
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
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.
Description
•