1.03 KB, text/html
1011 bytes, text/html
1009 bytes, patch
|Details | Diff | Splinter Review|
When attempting to clone a table row whose TD contains a java script, the browser goes into a seemingly infinite reload state. The URL changes to wyciwyg://0... and the browser can only recover by stopping, going back, then reloading the page. I found this problem with the daily build for 2002-02-19. I tested this under IE 5.0/5.5 and it works correctly. Here's the clone code I used: var rowNode = document.getElementById('Row1'); var bodyNode = document.getElementById('EditTable'); var newRow = rowNode.cloneNode(true); bodyNode.appendChild(newRow); I will attach a full test case...
"The URL changes to wyciwyg://0... and the browser can only recover by stopping, going back, then reloading the page." This is correct behavior assuming that we execute the script.... I feel that we should not execute <script> nodes if they are appended to content from the DOM.
The table data is already in the DOM as children of the table cell node (as is the script). I'm working on a testcase that will actually test what Mozilla and IE do here.
Created attachment 70829 [details] Testcase I tried this in IE5.0/Solaris and in Mozilla. Mozilla executes the script, IE does not.
Timeless just tested this with IE6/Windows. That also does not run the script.
What about script elements that refere to external scripts?
This bug is invalid. Creating and inserting a new script node *should* run the script. If I clone an entire document into a new window, I expect scripts to work in the new window, which often requires that inline scripts defining functions have been executed. document.write may not play nicely with DOM 2, but we shouldn't reduce the power and completeness of the DOM 2 functions to accommodate the DOM 0 function document.write.
Created attachment 70837 [details] Testcase using external script This includes an external script. The script _is_ run in IE5 (at cloneNode() time too, not when it's reinserted into the document).
Jesse says IE6 acts just like IE5 on the second testcase. I have to say that we should just make a decision here.... Do we try to emulate the IE behavior for scripts with no <src>? There are some issues here which Jesse has raised, especially with scripts that define things instead of creating content, and especially if the document being inserted into is different than the document being taken from. Frank, for purposes of real-world application, I'd suggest simply removing the script node from the DOM in the script itself. That way you'll get identical behavior to what you have now in IE (which doesn't run the script anyway) and will get identical behavior between IE and Mozilla.
I vote for compatibility with IE here, since that does actually mean quite a lot nowadays...
After talking to bz, I realized that if I'm trying to clone an arbitrary page, it makes more sense to copy the variables and functions over than it does to have each of the scripts execute again. I no longer think this bug is invalid.
Boris, Removing the script node works fine. I didn't realize that table data and the script were both there - I should have inspected it first. Sorry about that, I'm still learning this. I guess the real issue here is IE compatibility?
Frank, nothing to be sorry for. :) Yes, IE compatibility is the only issue. Fabian, is there any way to suppress execution of scripts without src on SetDocument, except when they are being appended from the content sink? I should also note that IE's behavior and ours differs for <script src=".."> and that IE's behavior seems to me to be insane....
bz, sorry, I don't know enough about the js engine and our script loading mechanism to answer your question
Mass-reassigning bugs to firstname.lastname@example.org
I have a hard time deciding what I think we should do here. We don't really follow IEs lead anyway when it comes to <script>s so IMHO IE compatibility is not that important. On one hand cloning a node should create a node that behaves just like the original copy, and in this case the original copy won't be reexecuted when being inserted again. Also, you can get the functionality by creating a new <script> element and copying children and the src attribute. On the other hand, cloning a script means that you're actually creating a new scriptelement, and we do execute newly created scriptelements. As i'm writing this i realize that i lean more towards not rerunning cloned scripts. In the case of cloning an entire document we probably don't want to rerun scripts (think script that do document.write). And in the case of copying part of a DOM and inserting in the same document then we defenetly don't want to rerun scripts.
Created attachment 141349 [details] [diff] [review] Well, this should be sufficient, right? Although, this doesn't handle the case when evaluation of a script clones itself; in that case the script is _not_ yet evaluated when it's cloned, right?
Did some more thinking. If we don't allow cloned scripts to rerun the we make it a lot harder to let someone rerun a piece of script. They would manually have to copy all children and attributes from an old scriptelement to the new one. However, I can't think of any usecase where you want to rerun the same script over and over. If you do you should put that script in a function and call the function instead. So the ability to clone a document without breaking seems more important then being able to rerun a script-element.
Comment on attachment 141349 [details] [diff] [review] Well, this should be sufficient, right? >+ it->mIsEvaluated = mIsEvaluated; Make that it->mIsEvaluated = mIsEvaluated | mIsEvaluating; Since the only way we could end up here is if the element actually contained evaluatable script and we'll soon be setting |mIsEvaluated| to true. And add a comment stating this. with that, r=me
Comment on attachment 141349 [details] [diff] [review] Well, this should be sufficient, right? Will make that change. jst, is this ok with you?
Comment on attachment 141349 [details] [diff] [review] Well, this should be sufficient, right? Apparently I failed to type in jst's email....
> However, I can't think of any usecase where you want to rerun the same script > over and over. If you do you should put that script in a function and call the > function instead. One theoretical use case would be cloning the element then slightly modifying the script then reinserting it. But I don't feel strongly about this.
You can always do that by creating a new <script> element and clone the textnode inside it. You can even replace the old <script> with the new one.
Comment on attachment 141349 [details] [diff] [review] Well, this should be sufficient, right? Yeah, I like this with sicking's suggestion, and we should probably copy mLineNumber in ::CloneNode() too. sr=jst with that.
Created attachment 145065 [details] [diff] [review] Patch updated to comments
Comment on attachment 145065 [details] [diff] [review] Patch updated to comments Could this please be approved for 1.7? This should be a fairly safe IE-compat change...
Comment on attachment 145065 [details] [diff] [review] Patch updated to comments Though actually, this is not quite IE compat. See comment 18.
Comment on attachment 145065 [details] [diff] [review] Patch updated to comments a=asa (on behalf of drivers) for checkin to 1.7
Checked in for 1.7