[webcomponents] [Shadow]: The return type of Event.path should leverage WebIDL sequences (bugzilla: 25458) (#101) from Hayato Ito on 2015-05-25 (public-webapps-github@w3.org from May 2015)

From: Hayato Ito <notifications@github.com>
Date: Mon, 25 May 2015 01:52:01 -0700
To: w3c/webcomponents <webcomponents@noreply.github.com>
Message-ID: <w3c/webcomponents/issues/101@github.com>
Title: [Shadow]: The return type of Event.path should leverage WebIDL sequences (bugzilla: 25458)

Migrated from: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458

----
comment: 0
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c0
*Hayato Ito* wrote on 2014-04-25 08:14:01 +0000.

The subject explains well.

----

comment: 1
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c1
*Hayato Ito* wrote on 2014-04-25 08:18:18 +0000.

Committed at
https://github.com/w3c/webcomponents/commit/4e9b329f62cf2451901455d0cf1b7066fc86c1eb

----

comment: 2
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c2
*Anne* wrote on 2014-04-25 08:47:22 +0000.

Note that [] is an IDL array which are going away. I guess we can still fix that down the road?

----

comment: 3
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c3
*Hayato Ito* wrote on 2014-04-25 08:50:29 +0000.

Thanks.

Do you have a pointer about what we should use instead of T[]?

----

comment: 4
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c4
*Hayato Ito* wrote on 2014-04-25 08:55:00 +0000.

I guess we should use `Sequence<EventTarget>`. Is that correct?

----

comment: 5
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c5
*Anne* wrote on 2014-04-25 08:58:30 +0000.

I don't think it exists yet because nobody is maintaining IDL :-(

sequence<> does not work with attributes.

----

comment: 6
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c6
*Domenic Denicola* wrote on 2014-04-25 13:38:28 +0000.

Can they be real arrays with the exact semantics specified in prose?

This seems related to https://github.com/w3c/screen-orientation/issues/13

----

comment: 7
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c7
*Boris Zbarsky* wrote on 2014-04-25 16:17:40 +0000.

> Can they be real arrays with the exact semantics specified in prose?

Yes.  You can put "object" in the IDL and then specify in prose.

That specification could even leverage WebIDL sequences in the sense of creating one, converting to a JS value, and caching the result.

The only open question is whether the result should be frozen or whether we should just let the consumer munge it.  Does anything internal look at .path on events?  If so, we may want to either freeze it or specify an internal accessor that returns the actual path.

----

comment: 8
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c8
*Hayato Ito* wrote on 2014-04-28 07:11:57 +0000.

(In reply to Boris Zbarsky from comment #7)
> > Can they be real arrays with the exact semantics specified in prose?
> 
> Yes.  You can put "object" in the IDL and then specify in prose.
> 
> That specification could even leverage WebIDL sequences in the sense of
> creating one, converting to a JS value, and caching the result.
> 
> The only open question is whether the result should be frozen or whether we
> should just let the consumer munge it.  Does anything internal look at .path
> on events?  If so, we may want to either freeze it or specify an internal
> accessor that returns the actual path.

My expected behavior is 'frozen'.

If the following event listener is given,

button.addEventListener("click", function(event) {
  console.log(event.path.length);
  var p = event.path;
  p.pop();
  console.log(event.path.length);
  console.log(p.length);
  event.path = [];
  console.log(event.path);
});


this event listener should print the following as example:
  4
  4
  3
  4

As far as I tested, the current WebIDL implementation of blink for `readonly attribute T[]` behaves like that.

----

comment: 9
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c9
*Boris Zbarsky* wrote on 2014-04-28 07:25:05 +0000.

> this event listener should print the following as example:

If event.path is a frozen array, this event listener will throw an exception on the "p.pop()" line.

> the current WebIDL implementation of blink for `readonly attribute T[]` behaves
> like that.

I'm not aware of Blink having any support for "attribute T[]" in IDL, readonly or not.  What it does instead is fake it with interfaces with indexed getters/setters.  For example, event.path in Blink is a NodeList.

That said Array.prototype.pop.call(someNonemptyNodeList), as currently specced, would land us in http://heycam.github.io/webidl/#delete which would return false, since there is no deleter defined.  And then http://people.mozilla.org/~jorendorff/es6-draft.html#sec-deletepropertyorthrow would throw.  This is in fact what happens in Firefox, though Blink seems to get this wrong.

And if Blink _did_ implement IDL arrays as currently specced for some reason, then you'd land in http://heycam.github.io/webidl/#platform-array-object-delete and once again throw, if the IDL array is not empty.

I have no idea where the "3" in your suggested log output came from, by the way, unless you think event.path should return a new object on every get (so that event.path == event.path always tests false)....

----

comment: 10
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c10
*Hayato Ito* wrote on 2014-04-28 07:50:36 +0000.

Thank you.

WebIDL looks too complicated to me :)

I don't have any intention to say the current WebIDL implementation of blink is correct. I just assumed that WebIDL is *mature* and there is no significant difference between every user agents.

I am now feeling that we should make event.path into a method rather than an attribute unless we have a well-defined way to represent a *frozen* array.
I always get uncomfortable when an attribute returns something array-like.

We might want to fix the blink's implementation at first.
Let me investigate further.

----

comment: 11
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c11
*Boris Zbarsky* wrote on 2014-04-28 14:55:04 +0000.

> I just assumed that WebIDL is *mature* and there is no significant difference
> between every user agents.

Things keep being added as needed, so it's not stable in the sense of frozen feature set.  But IE and Firefox are pretty close in following WebIDL in the parts they implement.

Blink and WebKit are off in their own world in all sorts of ways, as far as I can tell, WebKit more so than Blink.

> unless we have a well-defined way to represent a *frozen* array

See comment 7.

----

comment: 12
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c12
*Domenic Denicola* wrote on 2014-04-28 15:07:11 +0000.

It is the TAG's opinion that in cases like these, the arrays should be mutable (non-frozen). It is un-JavaScript-ey to attempt to protect consumers from themselves.

----

comment: 13
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c13
*Boris Zbarsky* wrote on 2014-04-28 15:54:40 +0000.

This case is a bit weird because the "consumers" of an event, esp with web components (which I assume is a given for event.path), are all sorts of pieces of unrelated code.  So it's more like protecting one consumer from another one that the first doesn't even know exists.

----

comment: 14
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c14
*Domenic Denicola* wrote on 2014-04-28 15:57:26 +0000.

Sure, but the reasoning stands. Nobody does that in JS. The DOM sometimes tries to, but the TAG's opinion is that doing so is not good API design and should be discontinued if possible in new DOM APIs. (Historically, it seems like this was sometimes done because implementations did not distinguish between the integrity of the model layer and the integrity of the user-exposed data layer. This distinction is important and something the TAG highlighted recently.)

----

comment: 15
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c15
*Boris Zbarsky* wrote on 2014-04-28 15:59:37 +0000.

> Nobody does that in JS.

In all fairness, until recently there weren't any tools in the language for people to do that, no?

I don't actually have a strong opinion on this stuff, as long as we make it clear that the internal implementation is not using the modified data.  I just want to make sure we're considering all the issues here.

----

comment: 16
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c16
*Hayato Ito* wrote on 2014-04-30 06:42:04 +0000.

(In reply to Boris Zbarsky from comment #15)
> > Nobody does that in JS.
> 
> In all fairness, until recently there weren't any tools in the language for
> people to do that, no?
> 
> I don't actually have a strong opinion on this stuff, as long as we make it
> clear that the internal implementation is not using the modified data.  I
> just want to make sure we're considering all the issues here.

In the current blink's implementation, the internal implementation is not using the modified data.
An even.path always creates a new array instance and returns it. Modifying the returned array doesn't have any affect to the internal implementation.

Anyway, I'd like to know what is a recommended way for event.path.
Is there any suggestion?

1). Use 'readonly attribute EventTarget[] event.path' and fix the blink's implementation of T[].

2). Because T[] is going away in IDL, find the other reasonable way, though I'm not sure what is an available option here.

3). Make event.path an operation: 'sequence[EventTarget] getPath()'

----

comment: 17
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c17
*Anne* wrote on 2014-04-30 11:12:59 +0000.

We want 2. The way you could define this is by saying it returns an "object" in IDL and define in prose that a JavaScript Array Object is returned which represents a copy of the underlying path concept (which is a list of EventTarget objects).

You should also keep an XXX somewhere in the specification to update this once IDL is fixed with proper support for JavaScript Array Objects.

----

comment: 18
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c18
*Boris Zbarsky* wrote on 2014-04-30 16:55:02 +0000.

And define when that object is created and when a new one is created (presumably as part of event dispatch).

----

comment: 19
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c19
*Hayato Ito* wrote on 2014-05-01 04:03:08 +0000.

Thank you. 2 looks a reasonable option here.

I updated the spec at
https://github.com/w3c/webcomponents/commit/6f296e4bb73238a15baab805a0079c16f9dfac7f

I appreciate any feedbacks for that.

----

comment: 20
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c20
*Domenic Denicola* wrote on 2014-05-01 04:10:19 +0000.

It should not be a new array on each get; it should be the same array. Otherwise e.path !== e.path.

You should create the array object upon construction of the event, and return the same one each time.

I also think the issues you linked to are not really related to the problem at hand.

----

comment: 21
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c21
*Boris Zbarsky* wrote on 2014-05-01 04:42:35 +0000.

> You should create the array object upon construction of the event

Construction or dispatch?  Dispatch would make more sense, since that's when the path is determined, and events can be redispatched.

Apart from that, I totally agree with comment 20.

Also, one more issue: I suggest you create the array by creating a WebIDL sequence<EventTarget> in your prose and then explicitly invoking the "convert to an ECMAScript value" concept from WebIDL.  That will actually rigorously define how the array is set up and avoid different implementations disagreeing about whether to use [[Set]] or [[DefineOwnProperty]] for the array entries.

----

comment: 22
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c22
*Hayato Ito* wrote on 2014-05-01 05:53:42 +0000.

(In reply to Domenic Denicola from comment #20)
> It should not be a new array on each get; it should be the same array.
> Otherwise e.path !== e.path.
> 
> You should create the array object upon construction of the event, and
> return the same one each time.
> 
> I also think the issues you linked to are not really related to the problem
> at hand.

The situation might be complex here in event.path.
The return value of event.path can differ even in the lifecycle of the same event object.

For example,

- an event listener, A, is added to a node, NODE_A.
- an event listener, B, is added to a node, NODE_B.
- Suppose NODE_A and NODE_B are in the different node trees.
- Suppose NODE_A and NODE_B are in the same event path for an event. (Don't confuse "event path" with event.path API).

Suppose, an event listener A is invoked at first. Then an event listener B is invoked.

var pathInA;

function eventListenerA(event) {
  pathInA = event.path;
  // pathInA is something like [D, C, A] here.
}

function eventListenerB(event) {
  var pathInB = event.path;
  // pathInB is something like [D, C, B] here, which is different what we saw in eventListenerA.
  // pathInA and pathInB should be the same JavaScript Array object?
}

----

comment: 23
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c23
*Hayato Ito* wrote on 2014-05-01 06:01:20 +0000.

FYI.
This is a related on-going discussion: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25423

If we could give away an *encapsulation* from "event.path", event.path could return the same JavaScript Array object.

----

comment: 24
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c24
*Boris Zbarsky* wrote on 2014-05-01 06:04:59 +0000.

If this API is not exposing the full, immutable-during-the-dispatch event path from the target to the root, then it should be a method, not a getter, imo.  And it needs a better name.

----

comment: 25
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c25
*Hayato Ito* wrote on 2014-05-01 06:47:56 +0000.

+Dimitri to CC List.

(In reply to Boris Zbarsky from comment #24)
> If this API is not exposing the full, immutable-during-the-dispatch event
> path from the target to the root, then it should be a method, not a getter,
> imo.  And it needs a better name.

That reminds me of the old discussion here.
See the comment https://code.google.com/p/chromium/issues/detail?id=234030#c22

Dimitri, WDYT?


One more note:

As I recall, the same event object can be *re-used* between event dispatching if we dispatch an event explicitly by calling EventTarget.dispatchEvent(event), re-using the same event object.
So the result of event.path could be *different* anyway for the same event object.

----

comment: 26
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c26
*Boris Zbarsky* wrote on 2014-05-01 07:15:53 +0000.

> As I recall, the same event object can be *re-used* between event dispatching

Yes, see comment 21.  Creating a new array on explicit action like array dispatch is probably fine.  Creating one seemingly-randomly during event propagation seems a lot weirder.

----

comment: 27
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c27
*Anne* wrote on 2014-05-01 09:36:58 +0000.

The way you want to define this is that events have an associated path and an associated path object. Then at various points in the lifetime of the event, you change the associated concepts as appropriate. E.g. initially, when you create an event, its associated path is the empty list and its associated path object is a new Array representing its associated path.

Then during dispatch when the path is calculated you set a new path and a new Array representing it.

Event.prototype.path meanwhile returns the path object. The specification algorithms use the path.

----

comment: 28
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c28
*Dimitri Glazkov* wrote on 2014-05-01 16:40:15 +0000.

(In reply to Anne from comment #27)
> The way you want to define this is that events have an associated path and
> an associated path object. Then at various points in the lifetime of the
> event, you change the associated concepts as appropriate. E.g. initially,
> when you create an event, its associated path is the empty list and its
> associated path object is a new Array representing its associated path.
> 
> Then during dispatch when the path is calculated you set a new path and a
> new Array representing it.
> 
> Event.prototype.path meanwhile returns the path object. The specification
> algorithms use the path.

Love it.

----

comment: 29
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c29
*Hayato Ito* wrote on 2014-05-27 11:26:03 +0000.

Let me resume this work after I resolve this issue:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458

----

comment: 30
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c30
*Hayato Ito* wrote on 2014-05-27 11:27:01 +0000.

Correction:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=25423

----

comment: 31
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c31
*Erik Arvidsson* wrote on 2014-05-29 23:17:26 +0000.

This discussion has mostly been about the IDL. I assume this bug is also about including the Window object in the event path?

----

comment: 32
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c32
*Hayato Ito* wrote on 2014-05-30 03:57:14 +0000.

(In reply to Erik Arvidsson from comment #31)
> This discussion has mostly been about the IDL. I assume this bug is also
> about including the Window object in the event path?

Yep, I remember that we had a discussion about Window object, however, I am lost and couldn't find the discussion.

Could someone give me the link to the discussion?

I am feeling that we should file a bug for that as another bug.


I couldn't find any mention about Window object "in 4.7 Dispatching events" of DOM standard.

http://dom.spec.whatwg.org/#dispatching-events says:

> If event's target attribute value is participating in a tree, let event path be a static ordered list of all its ancestors in tree order, and let event path be the empty list otherwise.

I am not sure what is the best way to deal with Window object in event path. I am assuming that Window object is not the parent of Document. Please correct me if I'm wrong.

----

comment: 33
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c33
*Anne* wrote on 2014-05-30 06:46:15 +0000.

Window is bug 21066. And yes, HTML defines Window and defines it as a parent of Document for the purposes of event dispatch. Ideally we make that a bit more explicit down the road. I still haven't figured out a good way to integrate shadow trees into DOM/HTML.

----

comment: 34
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c34
*Olli Pettay* wrote on 2014-05-30 22:08:11 +0000.

Note, load event dispatched somewhere in the dom (or shadow dom) doesn't
propagate to Window.

----

comment: 35
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c35
*Anne* wrote on 2014-05-31 07:25:08 +0000.

Yeah, that is noted by HTML. We should probably formalize that a bit better somehow.

----

comment: 36
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25458#c36
*Hayato Ito* wrote on 2015-04-15 05:31:22 +0000.

|Window| was already addressed in the spec tentatively.
https://github.com/w3c/webcomponents/commit/73b64edd930a623616923c9cfc0f118fc8cc6abc

So I think we can't resolve this issue until we can leverage WebIDL sequences.

Let me move this into "15480: [Shadow]: Things to Consider in the Future" category.

---
Reply to this email directly or view it on GitHub:
https://github.com/w3c/webcomponents/issues/101
Received on Monday, 25 May 2015 08:53:11 UTC