This is an archived snapshot of W3C's public bugzilla bug tracker, decommissioned in April 2019. Please see the home page for more details.

Bug 18732 - [Custom]: element upgrade algorithm does not preserve attributes and children
Summary: [Custom]: element upgrade algorithm does not preserve attributes and children
Status: RESOLVED FIXED
Alias: None
Product: WebAppsWG
Classification: Unclassified
Component: HISTORICAL - Component Model (show other bugs)
Version: unspecified
Hardware: PC All
: P2 normal
Target Milestone: ---
Assignee: Dimitri Glazkov
QA Contact: public-webapps-bugzilla
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 18720
  Show dependency treegraph
 
Reported: 2012-08-29 00:56 UTC by Steve Orvell
Modified: 2012-10-29 22:03 UTC (History)
5 users (show)

See Also:


Attachments

Description Steve Orvell 2012-08-29 00:56:57 UTC
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.
Comment 1 Olli Pettay 2012-08-29 06:37:36 UTC
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.
Comment 2 Olli Pettay 2012-08-29 06:39:08 UTC
(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.
Comment 3 Scott Miles 2012-08-29 17:14:03 UTC
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?
Comment 4 Dimitri Glazkov 2012-08-29 18:27:03 UTC
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?
Comment 5 William Chen 2012-08-29 18:35:34 UTC
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.
Comment 6 Dimitri Glazkov 2012-08-29 19:38:18 UTC
(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)?
Comment 7 Scott Miles 2012-08-29 19:52:19 UTC
(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.
Comment 8 Olli Pettay 2012-08-29 21:01:12 UTC
(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
Comment 9 Olli Pettay 2012-08-29 21:05:41 UTC
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.)
Comment 10 Dimitri Glazkov 2012-08-29 21:09:12 UTC
(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 :)
Comment 11 Dimitri Glazkov 2012-08-29 21:10:04 UTC
(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 :)
Comment 12 Olli Pettay 2012-08-29 21:12:00 UTC
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.
Comment 13 Olli Pettay 2012-08-29 21:13:26 UTC
(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)
Comment 14 Scott Miles 2012-08-29 21:14:18 UTC
(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.
Comment 15 Olli Pettay 2012-08-29 21:18:47 UTC
(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.
Comment 16 Olli Pettay 2012-08-29 21:21:00 UTC
In other words I think replace with .register should
mean the same as clone() + replace() and then
adding possibly some default event listener stuff.
Comment 17 Scott Miles 2012-08-29 21:22:48 UTC
(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);
Comment 18 Olli Pettay 2012-08-29 21:29:31 UTC
(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).
Comment 19 Scott Miles 2012-08-29 21:34:34 UTC
(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.
Comment 20 Dimitri Glazkov 2012-08-29 22:54:03 UTC
(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?
Comment 21 Olli Pettay 2012-08-30 03:50:31 UTC
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.
Comment 22 Daniel Buchner 2012-08-30 14:54:22 UTC
(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 :)
Comment 23 Dimitri Glazkov 2012-08-30 18:06:55 UTC
(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?
Comment 24 Dominic Cooney 2012-09-04 20:46:55 UTC
(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 #?
Comment 25 Dimitri Glazkov 2012-10-29 21:29:23 UTC
(In reply to comment #24)

> Good. Bug #?

Filed bug 19763 to debate copying event listeners.
Comment 26 Dimitri Glazkov 2012-10-29 22:03:51 UTC
http://dvcs.w3.org/hg/webcomponents/rev/9972306cd9eb