Bug 4403

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

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

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' ;

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.

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.

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

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

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.

+ 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...

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.

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.

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!