26528 - cloneNode() doesn't copy event handlers

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Chris Waterson]

Not sure if this is a bug or not, but I expected that cloneNode() would copy 
the event handlers. See attached test case. Assuming this is a bug, here's 
how to reproduce:

1. Click "Sheep". Note the message printed to the console.
2. Click "Clone It". A new "Sheep" button will be created via cloneNode().
3. Click the second "Sheep" button.

Expected behavior: a message is printed to the console.
Actual behavior: no message is printed.

Comment 1

[Chris Waterson]

Attachment: test case

Comment 2

[Jean-Francois Ducarroz]

In fact, they are copied but has the cloned node isn't yet in the DOM, the event 
handlers are not compiled and therefore wont work.

Comment 3

[vidur (gone)]

There are 2 issues here:
1) Should event handlers be cloned along with cloneNode()? The DOM WG says no, 
except HTML event handler attributes.
2) Should event handler attributes be compiled when an orphaned element is added 
to a document (currently we only compile them if the attribute is set while the 
element is attached to the document). The answer seems to be yes, but one can 
create cases where this might not do what you expect.

Comment 4

[vidur (gone)]

Marking M16 for now. Ducarroz implied that he is working around this issue, but 
would like it fixed eventually.

Comment 5

[Mike]

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

Comment 6

[Nisheeth Ranjan]

Marking nsbeta3...  Developers on the DOM newsgroup have complained about this 
functionality and the fix is reasonably straightforward.

Comment 7

[ekrock's old account (dead)]

correctness of DOM1 support. Fixed believed to be easy and low risk. Recomment
nsbeta3+.

Comment 8

[Johnny Stenback (:jst)]

Since this isn't known to break existing content we (I and Nisheeth) are
minusing this for now, please speak up if you think this really should be fixed
for beta3.

Comment 9

[Jan Carpenter]

Mass update of qa contact

Comment 10

[theBigTree]

Actually I think this is not a bug. a quote from
http://www.w3.org/TR/2000/PR-DOM-Level-2-Events-20000927/events.html

"When a Node is copied using the cloneNode method the EventListeners attached to
the source Node are not attached to the copied Node. If the user wishes the same
EventListeners to be added to the newly created copy the user must add them
manually."  

Comment 11

[Chris Waterson]

vidur, I'm guessing that the citation is a direct result of you clearing up this
issue with the WG? Please feel free to INVALID this bug...

Comment 12

[Richard.Hayton]

It may be 'correct' not to copy the event handlers. But it isn't what other
browsers do (IE 5) so is going to cause compatability problems. If we added 
an additional mechanism to copy event handlers, then we would probably be on
safe ground. 

I'm currently using a large and nasty js hack to manually copy them. It isn't 
very pretty and I suspect it will stop working when other 'features' of the
Gecho attribute/property duality a fixed up. Note that it isn't possible to
simple assign an event handler (using dest.onclick=src.onclick) as the function
context is wrong. My current workaround is as follows

function ffactory(body) // compile in context of current object
{
 return new Function(body)
}

//copy onclick event handler from src to dest
dest.newFn = ffactory;
dest.onclick = dest.newFn(src.getAttribute("onclick"));

This 'feature' is compounded by the fact there is no way to (simply) enumerate
all the event handlers on an element, so using the above in a loop is messy.

Hope the above helps anyone who has spent as long a me trying to get round this 
feature.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[Paul McGarry]

At the URL listed above, or more directly:
http://www.w3.org/TR/2000/PR-DOM-Level-2-Events-20000927/events.html#Events-registration-html40
it states:
"In HTML 4.0, event listeners were specified as attributes of an element. As
such, registration of a second event listener of the same type would replace the
first listener. The DOM Event Model allows registration of multiple event
listeners on a single EventTarget. To achieve this, event listeners are no
longer stored as attribute values.

In order to achieve compatibility with HTML 4.0, implementors may view the
setting of attributes which represent event handlers as the creation and
registration of an EventListener on the EventTarget."

While it says "implementors may" rather than something stronger, I'd suggest
that this would be a very useful thing for implementors to do.

Perhaps my attachment to bug 60700 gives a clearer demonstration of where this
may be useful.

Comment 15

[vidur (gone)]

Passing this one along to Johnny. At this point, I question whether we can
change our behavior without breaking some number of existing scripts (scripts
that manually copy the event handlers after cloning). Removing the Target
Milestone so Johnny can make a decision about validity and fix timeframe.

Comment 16

[Prashant Desale]

QA contact Update

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

Comment 18

[Prashant Desale]

Updating QA contact to Shivakiran Tummala.

Comment 19

[Sivakiran Tummala]

cloneNode's innerHTML also cannot be accessed. 

var foo = document.getElementById("foo");
var bar = foo.cloneNode(true);
alert (bar.innerHTML);

extended the existing testcase.

Comment 20

[Johnny Stenback (:jst)]

Accepting and targetting for mozilla0.9.8

Comment 21

[Fabian Guisset]

When fixing this we should make sure we don't break anything in the tree that
uses cloneNode(), and we should probably figure out whether this change is for
better or for worse, like Vidur said in post #15

Comment 22

[Sivakiran Tummala]

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

Comment 23

[Johnny Stenback (:jst)]

Pushing to mozilla1.0

Comment 24

[Fabian Guisset]

Attachment: Proposed fix.

This also fixes bug 71937 for free. The new CloneNode() is slower but does the
right thing, and CloneNode() is barely used.

Comment 25

[Philipp Metzler]

good news - i use CloneNode() very often! :-)

Comment 26

[Johnny Stenback (:jst)]

More to do ==> slower execution, no surprise there. I doubt the slowdown will be
noticeable at all, except in extreme edge casaes.

Comment 27

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Giving to Fabian since he has been working on the fix. If you can't do the
testing and get reviews in time for 1.0 please let us know.

Comment 28

[Fabian Guisset]

Sure, this just needs reviews... testing is already done.

Comment 29

[Johnny Stenback (:jst)]

Comment on attachment 70948 [details] [diff] [review]
Proposed fix.

sr=jst

Comment 30

[Brendan Eich [:brendan]]

Comment on attachment 70948 [details] [diff] [review]
Proposed fix.

r=[e-mail]

Comment 31

[Asa Dotzler [:asa]]

Comment on attachment 70948 [details] [diff] [review]
Proposed fix.

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 32

[Dylan Schiemann]

So this fix clones attribute event listeners, or all event listeners for an
element?  It is a bit unclear to me what is being fixed since there was some
disagreement about the spec. ... and if it does clone all event listeners, is
there a way to not clone event listeners?

Comment 33

[Johnny Stenback (:jst)]

This makes cloneNode() clone event handlers that are specified as attributes in
HTML, not dynamically set element.on* event handlers.

Comment 34

[Dylan Schiemann]

And also not those added with addEventListener, right?  I ask because of the
comments in comment #14

Comment 35

[Johnny Stenback (:jst)]

Correct, event handlers (listeners) added with addEventListener() will not be
cloned either.

Comment 36

[Fabian Guisset]

Checking in nsGenericHTMLElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsGenericHTMLElement.cpp,v  <-- 
nsGenericHTMLElement.cpp
new revision: 1.338; previous revision: 1.337
done

Marking FIXED, thanks for the reviews!

Comment 37

[Sivakiran Tummala]

verified fixed. Build 2002-03-22-05

Comment 38

[Boris Zbarsky [:bzbarsky]]

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

Comment 39

[Fabian Guisset]

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

Comment 40

[Pulsebot]

Pushed by ffxbld-merge:
https://hg.mozilla.org/releases/mozilla-beta/rev/43a4563baf7e
[fenix] For https://github.com/mozilla-mobile/fenix/issues/26528 - Add a nimbus feature flag for MR Home Onboarding Dialog