18732 2012-08-29 00:56:57 +0000 [Custom]: element upgrade algorithm does not preserve attributes and children 2012-10-29 22:03:51 +0000 1 1 1 Unclassified WebAppsWG HISTORICAL - Component Model unspecified PC All RESOLVED FIXED P2 normal --- 18720 1 sorvell dglazkov bugs danieljb2 dominicc sjmiles wchen public-webapps-bugzilla oldest_to_newest 72958 0 sorvell 2012-08-29 00:56:57 +0000 The element upgrade algorithm does not transfer element attributes or existing event listeners to the created element before the element is replaced in the dom tree (section 5.2.4). Also because the node is replaced, any existing reference to the un-upgraded element become stale. For these reasons, it may be better to morph the node in place, rather than replacing it. 72964 1 bugs 2012-08-29 06:37:36 +0000 Hmm, this can be tricky. We don't want to move event listeners to the new element, because otherwise the upgrade event wouldn't be fired. 72965 2 bugs 2012-08-29 06:39:08 +0000 (In reply to comment #0) > Also because the node is replaced, any existing > reference to the un-upgraded element become stale. I don't understand this comment. What becomes stale? The old elements are still there is someone keeps reference to them. 72973 3 sjmiles 2012-08-29 17:14:03 +0000 My understanding is that, If my document looks like this: <x-element important="important"><span>important goodies</span></x-element> and my code does: x = document.querySelector("x-element"); x.addEventListener("onclick", clickAction); if the upgrade algorithm runs after the above code, the following effects occur: - the original x-element is replaced in DOM - the new x-element has no onclick - the new x-element has no 'important' attribute - x-element's original children are lost (not in DOM) - the x reference refers to a node that's no longer in DOM The 'onclick' is lost (the old element is not in DOM, the new element doesn't have that event). Any attempt to use the x reference to work with DOM will simply not work (that node is not in DOM). Any query for 'important' x-elements will not include the new element. It's true that 'x' still references a node, but I don't see how it's useful. That node is dangling and really should be GC'd asap. I believe the alternative Steve mentions is to upgrade the element 'in place' my modifying it's prototype and performing the rest of the upgrade steps on this original element. In this case, the attributes, listeners, and references are intact. This solution is also not ideal because we lose whatever the native constructor would have done to the replacement element. In any case, if the upgrade event is migrated to the new object, can't the event simply be fired on the new object? 72983 4 dglazkov 2012-08-29 18:27:03 +0000 From https://bugzilla.mozilla.org/show_bug.cgi?id=783129: (In reply to Dimitri Glazkov from comment #13) > Yes, will fix this shortly: > https://www.w3.org/Bugs/Public/show_bug.cgi?id=18732 > > My thinking is that we transplant attributes and children only, not event > listeners. What's your thinking on how folks can go about baking default event listeners into custom elements? Is the upgrade event able to be defined on the prototype so that listeners can be added to all upgraded nodes of a certain type without a separate code block from the custom element definition? 72985 5 wchen 2012-08-29 18:35:34 +0000 Why don't we want to transplant event listeners? It seems like there is some concern about the upgrade event, but as Scott mentioned, I don't see why we can't just fire it on the new element. 72991 6 dglazkov 2012-08-29 19:38:18 +0000 (In reply to comment #5) > Why don't we want to transplant event listeners? It seems like there is some > concern about the upgrade event, but as Scott mentioned, I don't see why we > can't just fire it on the new element. We could, but they may not do what the user intended. For example: this.addEventListener('click', method.bind(this)); when transplanted, will still be acting on the old element. Is this a Real Concern (tm)? 72994 7 sjmiles 2012-08-29 19:52:19 +0000 (In reply to comment #6) > (In reply to comment #5) > > Why don't we want to transplant event listeners? It seems like there is some > > concern about the upgrade event, but as Scott mentioned, I don't see why we > > can't just fire it on the new element. > > We could, but they may not do what the user intended. For example: > > this.addEventListener('click', method.bind(this)); > > when transplanted, will still be acting on the old element. Is this a Real > Concern (tm)? If user didn't bind it, the method would be called on 'this' anyway, so presumably the user is doing this specifically to force the callback to fire on the current 'this' in the face of event-listener transplantation. I figure he gets what he wants in this case. I suppose if a user does that binding without thinking about it, it could be a foot-gun. 73000 8 bugs 2012-08-29 21:01:12 +0000 (In reply to comment #6) > We could, but they may not do what the user intended. For example: > > this.addEventListener('click', method.bind(this)); > > when transplanted, will still be acting on the old element. Is this a Real > Concern (tm)? It is 73001 9 bugs 2012-08-29 21:05:41 +0000 I'd rather have some dictionary passed to register and it would contain the listeners which would be automatically registered. Something like document.register("foobarelement", { prototype: HTMLSpanElement, listeners: { click: function(evt) { /*do something*/ }, mouseover: function(evt) { /* do something else*/ }; } }) (And please don't say webkit's memory management can't handle that.) 73002 10 dglazkov 2012-08-29 21:09:12 +0000 (In reply to comment #9) > I'd rather have some dictionary passed to register and it would contain > the listeners which would be automatically registered. > > > Something like > > document.register("foobarelement", { > prototype: HTMLSpanElement, > listeners: { > click: function(evt) { /*do something*/ }, > mouseover: function(evt) { /* do something else*/ }; > } > }) > Sweet. That should really cut down on the amount of things that we have to do in "created" callback. > (And please don't say webkit's memory management can't handle that.) If it can't, we'll treat is a bug :) 73003 11 dglazkov 2012-08-29 21:10:04 +0000 (In reply to comment #10) > (In reply to comment #9) > > I'd rather have some dictionary passed to register and it would contain > > the listeners which would be automatically registered. > > > > > > Something like > > > > document.register("foobarelement", { > > prototype: HTMLSpanElement, > > listeners: { > > click: function(evt) { /*do something*/ }, > > mouseover: function(evt) { /* do something else*/ }; > > } > > }) > > > > Sweet. That should really cut down on the amount of things that we have to do > in "created" callback. Oh. One problem -- how do I bind a listener to specific instance? I guess I can't, but that's what the "created" callback is for :) 73004 12 bugs 2012-08-29 21:12:00 +0000 Though, hmm, what happens if .register is called again with the same element name, but different options. Should we let only one .register call per element name? That might make certain things easier. 73005 13 bugs 2012-08-29 21:13:26 +0000 (In reply to comment #11) > Oh. One problem -- how do I bind a listener to specific instance? I guess I > can't, but that's what the "created" callback is for :) Why you need to bind. I was thinking 'this' would be correct. Effectively when the element is created, element.addEventListener("click", options.listeners.click) would be called. (Might need some flag for capturing too) 73006 14 sjmiles 2012-08-29 21:14:18 +0000 (In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > I'd rather have some dictionary passed to register and it would contain > > > the listeners which would be automatically registered. > > > > > > > > > Something like > > > > > > document.register("foobarelement", { > > > prototype: HTMLSpanElement, > > > listeners: { > > > click: function(evt) { /*do something*/ }, > > > mouseover: function(evt) { /* do something else*/ }; > > > } > > > }) > > > > > > > Sweet. That should really cut down on the amount of things that we have to do > > in "created" callback. > > Oh. One problem -- how do I bind a listener to specific instance? I guess I > can't, but that's what the "created" callback is for :) (In reply to comment #9) > I'd rather have some dictionary passed to register and it would contain > the listeners which would be automatically registered. > > > Something like > > document.register("foobarelement", { > prototype: HTMLSpanElement, > listeners: { > click: function(evt) { /*do something*/ }, > mouseover: function(evt) { /* do something else*/ }; > } > }) > > > > (And please don't say webkit's memory management can't handle that.) (In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > I'd rather have some dictionary passed to register and it would contain > > > the listeners which would be automatically registered. > > > > > > > > > Something like > > > > > > document.register("foobarelement", { > > > prototype: HTMLSpanElement, > > > listeners: { > > > click: function(evt) { /*do something*/ }, > > > mouseover: function(evt) { /* do something else*/ }; > > > } > > > }) > > > > > > > Sweet. That should really cut down on the amount of things that we have to do > > in "created" callback. > > Oh. One problem -- how do I bind a listener to specific instance? I guess I > can't, but that's what the "created" callback is for :) Events are normally automatically called in instance scope, yeah? 'this' inside click should refer to the target element unless somebody done explicit bindings. But in any case, seems to me 'how to attach default listeners' is another topic. What I'm concerned about is this: Bob the developer writes: <x-awesome onclick="awesomeClick()"></x-awesome> I'm 99.9% sure Bob wants that method to fire when that node is clicked, and that he should not be concerned with how x-awesome is actually instantiated. 73008 15 bugs 2012-08-29 21:18:47 +0000 (In reply to comment #14) > What I'm concerned about is this: Bob the developer writes: > > <x-awesome onclick="awesomeClick()"></x-awesome> > > I'm 99.9% sure Bob wants that method to fire when that node is clicked, and > that he should not be concerned with how x-awesome is actually instantiated. That would work because onclick is just an attribute which is cloned. 73009 16 bugs 2012-08-29 21:21:00 +0000 In other words I think replace with .register should mean the same as clone() + replace() and then adding possibly some default event listener stuff. 73010 17 sjmiles 2012-08-29 21:22:48 +0000 (In reply to comment #15) > (In reply to comment #14) > > What I'm concerned about is this: Bob the developer writes: > > > > <x-awesome onclick="awesomeClick()"></x-awesome> > > > > I'm 99.9% sure Bob wants that method to fire when that node is clicked, and > > that he should not be concerned with how x-awesome is actually instantiated. > > > That would work because onclick is just an attribute which is cloned. Sorry, I was trying to by succinct, but it was just confusing. I meant to echo my original example where the user has done this: x = document.querySelector("x-awesome"); x.addEventListener("onclick", awesomeClick); 73015 18 bugs 2012-08-29 21:29:31 +0000 (In reply to comment #17) > I meant to echo my original example where the user has done this: > > x = document.querySelector("x-awesome"); > x.addEventListener("onclick", awesomeClick); Well, in that case the default listener would be fine. And btw, it is click, not onclick ;) There is the upgrade event, which dev could use to add necessary listeners. In fact, I'd say listener shouldn't added to the element before it is the right kind of element (document.register is called). 73016 19 sjmiles 2012-08-29 21:34:34 +0000 (In reply to comment #18) > (In reply to comment #17) > > I meant to echo my original example where the user has done this: > > > > x = document.querySelector("x-awesome"); > > x.addEventListener("onclick", awesomeClick); > > > Well, in that case the default listener would be fine. > And btw, it is click, not onclick ;) > > There is the upgrade event, which dev could use to add necessary listeners. > In fact, I'd say listener shouldn't added to the element before it is the > right kind of element (document.register is called). I understand, but again I'm concerned about average developer. He starts with <x-awesome onclick="awesomeClick()"></x-awesome> Then somebody tells him it's a security issue, so he changes it to do x = document.querySelector("x-awesome"); x.addEventListener("click", awesomeClick); Then his page doesn't work, and now he has to go look up about custom elements and do things differently because 'custom elements are weird'. In general, I want consumers of custom-elements to need to require as little arcana as possible. 73027 20 dglazkov 2012-08-29 22:54:03 +0000 (In reply to comment #19) > I understand, but again I'm concerned about average developer. > > He starts with > > <x-awesome onclick="awesomeClick()"></x-awesome> > > Then somebody tells him it's a security issue, so he changes it to do > > x = document.querySelector("x-awesome"); > x.addEventListener("click", awesomeClick); > > Then his page doesn't work, and now he has to go look up about custom elements > and do things differently because 'custom elements are weird'. > > In general, I want consumers of custom-elements to need to require as little > arcana as possible. I can see the value of this argument. Olli, WDYT? 73043 21 bugs 2012-08-30 03:50:31 +0000 If we copy event listeners based on that arguments, we should copy also all the expando properties. And I don't want either one. I prefer simple APIs which use as much existing infrastructure as possible. So, clone() + replace() + whatever defaults the options has is what I'd prefer. 73065 22 danieljb2 2012-08-30 14:54:22 +0000 (In reply to comment #21) > If we copy event listeners based on that arguments, we should copy also > all the expando properties. > And I don't want either one. > I prefer simple APIs which use as much existing infrastructure as possible. > So, clone() + replace() + whatever defaults the options has is what I'd > prefer. After consideration, I agree that we should *not* copy event listeners for a few reasons: - Node.clone(), even with the deep arg set to true, does not do this. The expectation of this clone/replace would be symmetric with that existing, similar interface. - Presumably (step in if this is not the case Dimitri) a developer would just add any default event listeners that must be strictly bound to the inflated custom element inside the upgrade event the recieve. - Due to the rise of event delegation techniques to near-ubiquitous status in the developer community (I can't recall a single popular JS framework that doesn't use it), many of these even situations have become non-issues. - Scott brings up a developer expectation mismatch that I at first agreed with, but after further thinking, do not believe is an issue. The fact is, this is a pretty advanced web API - it is unlikely folks with only basic-to-novice level understanding of HTML will ever get stumble across a situation where they're dealing with custom elements. Consider this: If you have gotten to a point where you even know to write <x-foo> you probably have a working understanding of custom elements. I'd rather defer to the educational power of web API socializers and evangelists here to get the word out that the clone()-ish node replacement done here behaves the same way the existing Node.clone() interface does. That's my 2 cents :) 73077 23 dglazkov 2012-08-30 18:06:55 +0000 (In reply to comment #22) To make progress: 1) will spec without copying event listeners 2) split off a bug to consider copying event listeners. We can continue debating there :) Good? 73197 24 dominicc 2012-09-04 20:46:55 +0000 (In reply to comment #23) > (In reply to comment #22) > > To make progress: > > 1) will spec without copying event listeners > 2) split off a bug to consider copying event listeners. We can continue > debating there :) > > Good? Good. Bug #? 77327 25 dglazkov 2012-10-29 21:29:23 +0000 (In reply to comment #24) > Good. Bug #? Filed bug 19763 to debate copying event listeners. 77333 26 dglazkov 2012-10-29 22:03:51 +0000 http://dvcs.w3.org/hg/webcomponents/rev/9972306cd9eb