Bug 4403 – Script element doesn't load if you set src when it is already in the document

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

The creation of SCRIPT elements at runtime is not working. It works well with 
other browsers.

Steps to Reproduce: 
- Open the URL.
- Click on the buttons displayed in the page.

Expected Results: 
Both buttons should have the same behavior. You should see two alert 
saying "Loaded!" and "External function call!".

Actual Results: 
On Safari, just the second button works. Works correctly on other browsers (IE 
Win and Firefox Mac/Win).

Notes: 
It should not be a security "feature", also because it is easy to load the 
scripts using XMLHttpRequest, as shown in the sample. But the the 
XMLHttpRequest method makes the development debugging a real pain, impossible 
to handle on huge scripts, like my online editor (http://fckeditor.net).

Created attachment 3352 [details]
Scripts used in the sample page.

[Joost de Valk (AlthA)]

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

[Maciej Stachowiak]

Vicki, I know you worked on this area recently, any thoughts? Do we expect this to work? It's inserting a 
dynamically created script tag into the document and then setting src:

	// Create the script element.
	var e = document.createElement( "script" ) ;
	e.type	= "text/javascript" ;

	// Add the new object to the HEAD.
	document.getElementsByTagName("head")[0].appendChild( e ) ; 
	
	// Load the script.
	// Gecko fires the "onload" event and IE fires "onreadystatechange"
	e.onload = e.onreadystatechange = LoadUsingCreateElement_OnLoad ;
	e.src = 'test.js' ;

[Dave Hyatt]

I think it should.  We know the script was not inserted by the parser, and is empty with no src at the time 
the src is set.  If all of those conditions are met, I think it would be ok to load and run the script.

[Maciej Stachowiak]

Here's a very simple workaround. Set the onload and src attributes of the script *before* inserting it 
into the document:

function LoadUsingCreateElement()
{
        // Create the script element.
        var e = document.createElement( "script" ) ;
        e.type  = "text/javascript" ;

        // Load the script.
        // Gecko fires the "onload" event and IE fires "onreadystatechange"
        e.onload = e.onreadystatechange = LoadUsingCreateElement_OnLoad ;
        e.src = 'test.js' ;

        // Add the new object to the HEAD.
        document.getElementsByTagName("head")[0].appendChild( e ) ;
}

Maciej, I've worked on your workaround and we have done a big step forward with 
it... but it actually works partially. It seams that the "onload" event is not 
being fired.

I've created another test page with your workaround applied:
http://fredck.com/bugs/safari/loadscript2/

As said in the page, you should see two alert messages when clicking on the 
buttons. The first is an alert() placed inline the .js file that is loaded and 
it fires correctly. The second one is an alert() placed in a function inside 
the .js file loaded that is called when the script has been completely loaded 
(onload). this one is not getting fired.

This are the contents of the .js file loaded by the SCRIPT element created on 
the fly:

---

var sMsg = 'External Function Call!' ;

function ShowMessage()
{
  alert( sMsg ) ;
}

alert( 'Loaded!' ) ;

---

You've said that your workaround worked totally for you... is it true? Am I 
missing something? 

Thanks again.

[Amos Hayes]

Just thought folks should know that this bug is holding up Safari support in FCKeditor and, because of 
that, the new Atlassian Confluence 2.0 will not support Safari properly for WYSIWYG editing. Mozilla based 
browsers and IE 6 are supported.

See the following for more info:

http://www.fckeditor.net/safari.html
http://jira.atlassian.com/browse/CONF-3864

[Ryan Kaldari]

Ditto for Sitemason (http://www.sitemason.com/). As soon as this is fixed for
FCKeditor we will be officially supporting Safari for our retail and enterprise CMS.

Created attachment 4718 [details]
patch

Created attachment 4724 [details]
Load script when src is set; generate load event when loaded

[Amos Hayes]

Appologies. Apparently the confluence issue was out of date. It was switched over to TinyMCE before the 
2.0 release. TinyMCE support of Safari is experimental at this point. I don't know if this bug is relevant to 
the TinyMCE code, but thanks for tackling it so quickly.

[Geoffrey Garen]

+    if (m_cachedScript || m_createdByParser || !oldSrc.isEmpty() || !inDocument())
+        return;

Just out of curiousity, why don't we load the script in these situations?

(In reply to comment #12)

> Just out of curiousity, why don't we load the script in these situations?

m_cachedScript because it means something is already loading; !inDocument() because the script will be 
loaded when insertedIntoDocument(); the rest is based on Dave Hyatt's comment #4, but now I see that I 
forgot to check that the script is empty...

[Darin Adler]

Comment on attachment 4724 [details]
Load script when src is set; generate load event when loaded

This code looks good, but most of it is in the wrong function.

Code to respond to an attribute change should be in the parseMappedAttribute
function. For example, see HTMLImageElementImpl::parseMappedAttribute and note
its handling of srcAttr.

To test this you can try using setAttribute('src', URL) and you'll see it won't
work. The setSrc function has to be just a simple cover that calls
setAttribute.

The loadEvent half of this fix is separate, and looks fine; a separate bug
report and separate test would be a slightly better way to handle that.

Created attachment 4785 [details]
Revised patch for loading script when src is set

I moved the code to parseMappedAttribute(). I also changed the rules for
deciding when to load from the new src. A load request will happen when setting
src to a non-empty string only if the script element never contained text and
never had non-empty src ("never" here actually means "not since it was inserted
into the document"), regardless of whether the script element was created by
the parser or dynamically.

This suggested behavior matches Firefox (as far as I checked) with the
exception of self-closing <script/> tags, for which Firefox never allows
dynamically loading.

WinIE's behavior is to load whenever src is set, regardless of how the script
was created, whether it contained text or had src, and whether the new src is
different from the old src.

I opened bug 5812 for the load event issue.

Comment on attachment 4785 [details]
Revised patch for loading script when src is set

Please see my previous comment about the logic of this patch.
A couple comments about the code itself: CloseRenderer() is obviously a
misnomer for this case; I am slightly abusing m_createdByParser.

[Darin Adler]

Comment on attachment 4785 [details]
Revised patch for loading script when src is set

This looks fine, except for the part where attr->value() is converted to a
QString and then to a DOMString. That's unnecessary, because an AtomicString is
a DOMString. The variable should just be a const AtomicString& instead. But I
see that this is just copied and pasted code from
HTMLScriptElementImpl::insertedIntoDocument -- ideally the two would share the
common code. It's strange that requestScript's charset parameter is a QString
and the URL parameter is a DOMString; that's worth fixing some day too.

The other two implementations of closeRenderer call the base class's
implementation, so ideally I'd like to see the call to
HTMLElementImpl::closeRenderer() at the end of the closeRenderer function even
though it's an inlined empty function.

These are both nits, so I'm going to say review+.

I'd like to see tests that test both setting src as a JavaScript property and
as an HTML attribute.

Created attachment 4820 [details]
a regression test to go with the code changes

Hello Guys,

I did some test with the patch and it worked for FCKeditor. This is really nice.

I just had some error on execution, but they doesn't happen all times. The 
errors says that a function, that is loaded using the <SCRIPT> creation is not 
defined. Some times it works well every time I load the editor, but a few times 
(no matter if I clear the cache or not) I had that error.

It is just a guess, but it would be nice if you could check if the "onload" in 
fired only after the script is download AND parsed, so it is available to be 
used. I fell that, if the browser process the file a little big slower because 
of other tasks, it could happen that the script is not available even if 
the "onload" gets fired.

Thanks again for your nice contribution.

(In reply to comment #19)
> I did some test with the patch and it worked for FCKeditor. This is really nice.

Which version of the patch? I'm asking because of this:

> It is just a guess, but it would be nice if you could check if the "onload" in 
> fired only after the script is download AND parsed, so it is available to be 
> used.

The latest (2005-11-23) version of the patch does not include onload events at all. There is now a 
separate bug, bug 5812, to deal with that. I think that when they are supported, onload events should 
be emitted only after the script is loaded evaluated.

Ok... now I've checked it. I had applied the 2005-11-18 patch because 
the "onload" thing is a key feature for my needs.

I noted that you have not proposed the "dispatchHTMLEvent(loadEvent, false, 
false);" as a patch for bug 5812. Does it mean that that is not the correct 
implementation solution? Ok... I'll add a comment there so we can discuss it in 
the right place.

[David Kilzer (:ddkilzer)]

NOTE: Bug 7552 has been retitled "FCKeditor master bug (...)".  Please file dependent bugs on Bug 7552 to make the FCKeditor work in Safari/WebKit.  Thanks!