Change deepPath() back to path (a FrozenArray<EventTarget> attribute)

[foolip]

Continuing from #361 (comment):

Event.deepPath() came up in a Shadow DOM v1: Status updates blink-dev thread, where I question why this is a method and why we can't just stick with Event.path.

The core of my concern is that there's no particular reason to think it will ever be possible to remove Event.path from Blink. The usage (~2.5%) is almost certainly inflated by enumeration (making copies of events) but that still means we have no idea about how risky such a change would be.

I think only a readonly attribute FrozenArray<EventTarget> path with deep semantics should be very seriously considered. There are two warts that I know of:

• All other things in DOM operate on the "local" tree, so using the generic name path for the deep event path is a bit inconsistent.
• A method makes it more obvious that a computation required.

Still, at least in my mind, this is not terrible enough to risk yet another duplicated API in the long run, like document.characterSet+document.charset and innumerable prefixed APIs.

@annevk also asks "whether Blink includes WindowProxy in the path", and it would be good to flesh out the definition a bit.

Here's a quick and basic test for that:
http://software.hixie.ch/utilities/js/live-dom-viewer/saved/3975

In Chrome Canary it prints ["HTMLButtonElement","ShadowRoot","HTMLDivElement","HTMLBodyElement","HTMLHtmlElement","HTMLDocument","Window"] so the Window(Proxy) is included in the path.

[annevk]

I think I would be okay with this change.

I initially pressed for "deep" since I thought we might have "path" meaning the path of the event without open shadow trees and "deep" could be a convention for APIs that include open shadow trees. However, as we consider more features and their integration with shadow trees, the more it seems like some of them might leak open shadow trees and not follow this "deep" naming.

It does not really help that we still haven't established solid principles around this. Is anyone keeping track of APIs that leak open shadow trees in some way?

[rniwa]

Even if we're renaming deepPath to path, we should still keep it a method because the things inside the path needs to change overtime as it cross closed-shadow boundary.

[hayatoito]

It's a good opportunity to see the history of Event.path.

1. Blink shipped Event.path as a property.
2. Then, at F2F meeting in the last year, we agreed on renaming Event.path to Event.deepPath.
3. Then, in the discussion, Use FrozenArray<T> in WebIDL #361, Event.deepPath becomes a method, Event.deepPath().
   I agree with @rniwa's comment, Change deepPath() back to path (a FrozenArray<EventTarget> attribute) #428 (comment), here. That's the reason I wanted to change this to a method.
4. Then, @rniwa suggested one behavior change for Event.deepPath. See deepPath should return an empty array if it's no longer being dispatched #373. I accepted this change because:

• We do not have to worry about a backward compatibility issue. Event.path and Event.deepPath() can have the different behavior.
• Event.deepPath() is now a method.

5. Then, reverting renaming, which was done at (2), is proposed (<-- Now)

Given this history, what is the best choice for us?

Option A) Event.path as a property, including the behavior change of 4.

• Cons: This is an incompatible change for Blink. Blink would break the Web.
• Cons: Change deepPath() back to path (a FrozenArray<EventTarget> attribute) #428 (comment)

Option B) Event.path as a property, also reverting behavior change of 4.

• Cons: We lost the behavior of 4. I prefer the behavior of 4.
• Cons: Change deepPath() back to path (a FrozenArray<EventTarget> attribute) #428 (comment)
• Cons: console.log(event) would upset developers, as we saw in https://bugs.chromium.org/p/chromium/issues/detail?id=401947

Option C) Event.path() as a method

• Cons: This is an incompatible change for Blink. Blink would break the Web.

Option D) No action. Event.deepPath() as a method.

• Cons: Blink will have Event.path and Event.deepPath()

[rniwa]

I'm not sure if option C would break the Web for Blink. In theory, you should be able to add a sequence like object that can be called. i.e. event.path would continue to behave like an array of Nodes which is also callable.

I'm gonna also add a new option:

Option E) Add Event.getPath() or Event.currentPath() as a method

[foolip]

#428 (comment) is very interesting, and I can see why that tipped the balance in favor of a method.

However, the main benefit of FrozenArray<T> over the old T[] construct, I think, is that event.path===event.path will always evaluate to true. But there shouldn't really be any problem in letting event.path change when dispatch begins and when dispatch ends. Reverting #373 wouldn't really help, as at the very least it has to change when dispatch begins. I can't see this spelled out in explicit terms in WebIDL, but if it is currently disallowed then presumably the spec could be changed.

If the alternatives @hayatoito outlines, Option A seems worth exploring. It seems pretty likely that this detail could be changed without breaking the web. To access event.path after dispatch would require saving a reference to event for later, and most event handlers probably don't do that. It should be easy to add a use counter for, but there is a risk that such a counter would be poisoned by code saving the event and later enumerating its properties without caring specifically about path.

[rniwa]

@foolip : well, the problem here is that event.path would change over the course of the dispatching of a single event because it needs to hide nodes within closed shadow trees. That is, for every node n in the event path p of an event e, e.deepPath() would return a subset of p which consisting of disclosed nodes of n when the current target is n.

[foolip]

Yes, but that shouldn't be an additional problem, as long as it has to change once (which it must) then the model is already one where the attribute's value can change. However, it would still be possible to make event.path===event.path always evaluate to true. Implementation-wise, the bindings would have to ask the internal Event class if path has changed since it was last returned, and if not just return the previous value. It would be very similar to a proposed change to HTMLMediaElement's buffered attribute.

[rniwa]

@foolip : no :( It can potentially change for every n in p. Please go talk with @domenic / @hayatoito.

I also don't see the value of event.path===event.path always being true since I doubt any author would need to check the equality of different kinds of event paths. What are use cases that demand such operations?

[hayatoito]

However, it would still be possible to make event.path===event.path always evaluate to true.

No. As long as event.path returns an immutable FrozenArray<EventTarget>, that is impossible in event object's lifecycle. :(

Also developers can re-use the same event object instance, in dispatching yet another (synthetic) event.

[foolip]

@rniwa, I acknowledged that it can change for every node n in the event path p, or at least every other n if ShadowRoot is included on the path like it currently is in Blink. But that event.path can change at specific points during dispatch is not the same thing as it changing for every access, and event.path===event.path could still be made to hold true if we wanted. If FrozenArray<T> currently doesn't allow for this then that's a hassle, but not insurmountable.

[rniwa]

What's the advantage of making event.path===event.path true? I can't think of use cases in which that property is needed.

[hayatoito]

Ops. I understand you meant that the expression of event.path===event.path is always true. I see.

I thought that you meant:

const n = event.path;
... (later) ...
n === event.path;  (is always true)

[foolip]

@bzbarsky called it a "very reasonable expectation" but of course it's not a matter of use cases.

Having a path property that returns a new array every time would work, and although ugly I think I'd still lean towards that to avoid the risk of having both path and deepPath forever.

[bzbarsky]

Having properties that return a new object every time is an antipattern for several reasons:

1. It's very inefficient. Consider what typical code that walks event.path would look like and when it would create new objects.
2. It breaks consumers who might want to use the property value as a key in a WeakMap (less of an issue for this particular case, I agree, since you're not likely to be dealing with multiple event paths at once).
3. It breaks consumers who add properties to the return value only to see them disappear. Note that this does not affect FrozenArray, because it's frozen so you can't add properties.
4. Less importantly in practical terms, it violates the a.b === a.b constraint. I say less importantly, because just having a.b = NaN also violates that constraint. The expression involving === is just a shorthand for the "no surprises" aspect of property getters: they should generally not be super-expensive (the web platform fails this in a bunch of cases, yes) and they should not allocate new things each time you call them (the web platform is a bit better here).

There are probably more reasons that I can't think of right now this time of night. Again, this has been discussed to death for years now...

It's unfortunate that after this has all been known for years the shadow DOM implementors in Blink decided to add and then ship in release the "create a new object each time" behavior for path.

Anyway, having a FrozenArray attribute that changes at well-defined points (like whenever the current target changes, or event dispatch starts or ends) is quite feasible if we do want to go that route.

I would be somewhat opposed to implementing the "path creates a new object every time` behavior unless people think it's actually needed for web compat.

[domenic]

See also https://w3ctag.github.io/design-principles/#attributes-like-data

IMO option D seems quite reasonable. Blink will have to pay a legacy tax for a long time having shadow DOM v0 features. That is sometimes the unfortunate cost of being a first-mover. event.path is not a very high cost as far as I can tell.

[rniwa]

I agree that an IDL attribute that creates a new object each time doesn't meet author expectation, and this is why we had a consensus to make it a method instead. I was asking use cases more in terms of semantics. Why do authors want to use the result of event.deepPath() as a key in some map or compare two paths returned by the same event?

[foolip]

Yes, a method would have been better, and I understand that nobody objected when this change was made. But the situation we find ourselves in is that nobody can say if or when Event.path can be deprecated and removed from Blink. Lacking some way of estimating the compat risk of such a change, there's just no way to proceed.

I think the options are:

1. A certainty of getting only a less-than-ideal API, the event.path attribute.
2. An x% chance of getting only the nicer API, event.deepPath() and a (100-x)% chance of getting both event.path and event.deepPath().

Because I think x is a relatively small number, option 2 looks risky to me.

[bzbarsky]

Are we talking about just Blink having to support event.path for some time into the future, or about everyone ending up having to implement event.path?

[foolip]

I mean that event.deepPath() would be replaced by event.path in the spec, and that this is the only thing anyone ships. That's the only path where long-term interop is guaranteed AFAICT.

[rniwa]

Why? Only Blink implements event.path and ships v0 shadow DOM API. I'm not particularly concerned about existing contents that use event.path since it's a relatively new feature even in Blink, and we haven't gotten any bug reports about websites not working because of it. So realistically speaking, only Blink would be stuck keeping that API surface.

[foolip]

Having to keep event.path in Blink alongside event.deepPath() indefinitely while other browsers only support event.deepPath() is not a good outcome for interoperability.

event.path has been shipping since Chrome 35, which went stable in May 2014. That's enough time to worry about breakage when removing it, even if the lack of support hasn't showed up as a compat problem for other engines. Critically, use counters are rendered useless by enumeration, so we have no reliable measure of risk, and inaction is the default.

It's not an ideal API, but is it so bad that it cannot be salvaged?

[rniwa]

I implemented the feature as deepPath() in http://trac.webkit.org/changeset/198056 in WebKit.

[foolip]

It's also implemented in Blink, behind a disabled-by-default flag as it appears to be in WebKit.

Since #428 (comment) I have learned that FrozenArray<T> really only means that the return value is a frozen array, and doesn't imply that the same frozen array is returned every time or similar. In other words, there would be no problem to simply define event.path as a readonly attribute FrozenArray<EventTarget> and to have an internal slot that is updated at the appropriate steps in during dispatch.

@rniwa, would this be acceptable to you and WebKit, or are there additional problems not addressed?

[annevk]

@foolip and the event object would hold a weak reference? It seems a little harder to optimize and it would no longer clearly identify the API as piercing through the shadow boundary. (Is there precedent for that elsewhere?)

[rniwa]

No, like I've repeatedly said, we want event.deepPath(), not event.path especially because we don't support FrozenArray in WebKit. Given no other browser engine (intends to) implements event.path, I don't think compatibility risk is high. In fact, all those websites that currently use event.path must have a fallback path for other browsers. If they don't, they're already broken on other browsers.

[foolip]

@rniwa, is there any problem in addition to the lack of FrozenArray support? Blink also doesn't support that yet, FWIW.

[foolip]

@annevk, I think the event object would hold a strong reference, why weak? At the end of dispatch it would be replaced by an empty array. But of course none of the arrays need to be created eagerly, if the attribute is never read then no actual array would be created. It's probably easier to spec it as a internal slot, though.

[hayatoito]

My preference is:

• Strongly preferred: Option D (or E)
• Blink can live in a world: Option A or Option B. I prefer Option A to Option B, though Option A would cause the Blink in trouble due to the backward incompatibility risk.
• I object to Option C strongly.

@foolip,
I am curious how you can evaluate each cons in Option A - Option D? I understand that you think the cons of Option D is not ignorable, but could you explain why cons of Option D is much bigger than the cons of other options?

Whether using FrozenArray or not does not sound important to me here. An engine can optimize the timing of creation of an internal array of C++, or re-using an internal array, without any spec support, as I actually did it in Blink.

[foolip]

Sure, here's how I view the options. Where there is uncertainty, I'll give a percentage of the odds I'd give if running a (very strange) lottery. If the real odds were much different and could be known, I might view things differently.

Option A) Event.path as a property, including the behavior change of 4.

• Pro: 90% chance of fast interop
• Con: 10% risk of compat issues forcing a move to Option B or E.

(I assume this also includes the changes during dispatch when crossing closed shadow tree boundaries, not just the final emptying of the array. This doesn't change the situation because v0 is open.)

Option B) Event.path as a property, also reverting behavior change of 4.

• Pro: 100% chance of fast interop
• Con: A silly API that will presumably still change during dispatch, just not at the end.

Option C) Event.path() as a method

• Con: Longer path to interop, even riskier than trying to remove Event.path outright. Not worth further consideration IMHO.

Option D) No action. Event.deepPath() as a method.

• Pro: 100% chance of eventual interop on deepPath()
• Con: 80% risk that Event.path will never be removed from Blink because we have no way of estimating the compat risk.
• Con: 20% risk that eventually Blink will have a strong hold in some market, causing enough compat problems to compel other engines to also implement Event.path, eventually spreading to 100% of engines. (This scenario is currently unfolding with many webkit-prefixed APIs.)

Option E) Add Event.getPath() or Event.currentPath()
As my concern is the future of the Event.path property, I view this like Option D.

[rniwa]

Where did you get those probabilities?

[foolip]

They are merely the best guesses I'd use if anything were at stake. To justify them somewhat:

10% chance of failure on Option A is because changing the return value after the end of dispatch must be a small minority of the total usage. Most event handlers don't save the event and look at it again later. https://bit.ly/blinkintents lists many small behavioral changes that worked out, many that I'd consider riskier.

80% risk that Event.path will never be removed from Blink is based on the first comment in this thread, that "we have no idea about how risky such a change would be" and thus are stuck. Maybe someone with extreme patience could show it safe using httparchive analysis, or some other new tool, eventually.

20% risk of this spreading due to Blink compat is the most speculative. I want it to be 0%, but it's happened before with IE and WebKit, so doesn't seem too far fetched. Another way it could happen is that Edge has to copy it because they identify as Chrome, and then it's hard to see it ever going away, even if it's not implemented elsewhere.

If my guesses seem wildly wrong, or if they aren't even the right things to guess about, I'm also interested in seeing how others see this playing out.

[rniwa]

I don't think the risk is high enough to revert this change. FWIW, if this is a compat issue, I'd imagine the lack of support for createShadowRoot would be an even bigger compat issue. And, as far as I know, Edge doesn't support createShadowRoot today even though I've heard that Blink can't easily remove createShadowRoot due to legacy content that uses UA string sniffing.

[dcleao]

Impressed how the consistency and quality of the design seems to be put at risk due to an existing implementation of an unfinished spec. Breaking the web? The experimental web is not the web... It's the use at your own risk web.
Please go for the best design. Anything else will have more future costs for everyone, compared to any sparing on early adopters.
Sorry for the "intrusion". I now leave, respectfully.

[foolip]

@rniwa, sure, the createShadowRoot situation is harder, because the only entirely safe path would be to drop attachRoot and give createShadowRoot three modes with v0 as the default, and that would virtually guarantee that v0 can never be removed, where that is currently plausible. event.path looks easier, because sane semantics that are still backwards compatible seem to be within reach.

But, I'm not getting any traction on this, so I'll walk away. Because the current path was agreed upon by all relevant parties (I am not one) I think Blink should be prepared to take greater risks to get rid of v0, even though I think this is one that could have been averted.

[hayatoito]

@foolip, I got it. Let me close this issue without any action.