event.composedPath() may return different results if DOM is modified between two calls

[smaug----]

If I read the spec right, composedPath() relies on the current tree structure to figure out what to return.
So, if a listener does then something like

var cp = event.composedPath();
/*move nodes around*/
var cp2 = event.composedPath();

cp and cp2 may contain different nodes, which is surprising, since the internal path for the event hasn't changed.
In particular, relying on "shadow-including inclusive ancestor" in https://dom.spec.whatwg.org/#concept-closed-shadow-hidden in https://dom.spec.whatwg.org/#dom-event-composedpath looks wrong

[smaug----]

Looks like use of 'shadow-including root' in window case is problematic too.

[smaug----]

@hayatoito what does blink do here? @rniwa or webkit?
I guess I need to look at their source code.

[hayatoito]

Ah, my first reaction is "This is a spec bug. composedPath() shouldn't rely on the current status".

However, it looks Blink's implementation relies on the current status. :(

[hayatoito]

I think we should fix the spec if we can agree on that.

[domenic]

One thing to keep in mind if possible is to keep things "lazy", or at least optimizable. Right now if there are no capturing listeners for a non-bubbling event you can avoid computing the path. (Per spec.) It's a very nice property to have and allows us to keep events cheap so that people don't start using the XObserver pattern to replace events because it's "faster".

So, just be careful, in fixing the spec here. If possible.

[hayatoito]

Yup, that can be a reason we won't change the spec.

As far as I can tell, calculating composedPath() is not so cheap.

[hayatoito]

One more thing:
Since calculating composedPath() is not so cheap, Blink caches the result; composedPath() is calculated lazily, but only once per each tree scope. In the 2nd call of composedPath(), it just returns the cached result without relying on the current status. In this sense, Blink is not conforming spec.

[smaug----]

Oh, Blink is even against the current spec, and the current spec is against, well, sanity :)

When fixing this issue one doesn't need to calculate much during event path creation. Just annotate each event path tuple in a subtree with that subtree's unique "id" and whether that subtree is open or closed.
That is basically flag + a counter. Then just use that information when creating the path lazily.
And by fixing this, composedPath() would always return the same result for the currentTarget, so caching the result would be ok per spec.

So I don't see a reason to not fix this.

[hayatoito]

Yup, I tend to agree that is doable without much cost. I'm fine to fix the spec. That should be considered as a spec bug, basically.

It is not intentional that Blink is not confirming the current spec. It looks I forgot a possibility of DOM mutation between calls. :(

[rniwa]

Initially I thought this would be fine but there's a challenge here. Just remembering to which tree each node belongs is not enough because a shadow host could move from one tree to another during an event dispatching. It would mean that we have to compute the relationships between each shadow tree & document tree before the events get dispatched. This is going to be quite expensive since you'd have to remember to which shadow/document tree each node belongs as well as what the shadow tree's parent tree was. That's not the kind of a thing we'd like to implement in WebKit. As such I'm against making this spec change.

[domenic]

@smaug---- your algorithm is still O(depth) of the tree, right? That's not great; making event firing always O(n) even for non-bubbling events with no capture listeners seems like a really bad slowdown to me (given how few pages use capture listeners and how many events are non-bubbling).

[smaug----]

How is that O(depth)?

[hayatoito]

O(depth) is okay. We have to spend O(depth) cost at any case in event dispatching.

For most kinds of events, we have to traverse a tree up anyway because we can't tell there is no event listener for capture/bubble phase at each node beforehand.

Blink has some optimization for some kinds of events so that we can skip event path calculation entirely. But this case is very limited, AFAIK.

I'll spend some time later to know how feasible this spec change is for Blink, from performance's perspective.

[domenic]

How is that O(depth)?

I'd assumed "Just annotate each event path tuple in a subtree" requires iterating over each node in the path, and thus computing the path, which is O(depth).

We have to spend O(depth) cost at any case in event dispatching.

I don't think we do. We can easily tell when there is no capture event listener for a given event name in the document beforehand, just by keeping track. Which means that for non-bubbling events of that type we can just totally avoid event path computation and only dispatch on the single node.

From what I understand talking to @ikilpatrick Blink does not currently implement this optimization because of Blink's non-standard event.path which is computed eagerly. But we could implement it, and thus have O(1) event dispatching in the somewhat-common case I am talking about. We should not prevent ourselves or other browsers from achieving this speed-up, if at all possible.

If it would be helpful I can implement this O(1) dispatching in jsdom so you can see an example of how the implementation would look.

Blink has some optimization for some kind of events so that we can skip event path calculation entirely. But this case is limited.

Maybe the case is currently limited in Blink, but I think it could be quite common. (All non-bubbling events when there are no composed listeners for that event type on the document.) Regardless of how limited or common it is, it would be sad to mandate in the spec that we can never skip the event path calculation.

[annevk]

@domenic event.composedPath() is not lazily the way you describe now either. I don't think anyone would want it to be fully lazy.