WICG / webcomponents

Web Components specifications
Other
4.37k stars 371 forks source link

Need "slotchange" event #288

Closed dglazkov closed 8 years ago

dglazkov commented 9 years ago

For developers, it will be good to have an event that is fired whenever slotting algorithm runs within a shadow tree. Maybe runs and produces changes compared to the previous result? Don't know. Also don't know if this is v1 or v2.

annevk commented 9 years ago

I vote v2 or at least not the MVP we can ship when we're ready aligning with all decisions made thus far.

hayatoito commented 9 years ago

For interoperability, should we finish the following task (mentioned in https://github.com/w3c/webcomponents/issues/128):

Enumerate all DOM operations that will trigger the slotting algorithm. This will be defined as a synchronous action.

to guarantee when such an event is fired?

I also vote v2. I'd like to be careful about exposing the timing of running the slotting algorithm to developers.

sedwards2009 commented 9 years ago

In my web components I use a MutationObserver to watch for changes under the host node. It seems to do what you are asking.

sorvell commented 9 years ago

MutationObserver doesn't work since a slot may be the child of an element with a shadowRoot. In this case, the child list of the element may not change but the elements that are rendered in place of the element's slot do change.

sorvell commented 9 years ago

Having worked with Shadow DOM a lot, not having this feature in Chrome's existing implementation is very cumbersome. I'd really like to see this api included in v1 if possible.

If it's ever important to be able to manage your children (menu, tabs, stack of pages, carousel, etc), then you need to know when the elements distributed to your slots have changed.

MutationObservers are quite cumbersome to use for this: To use MO, you have to do something like: (1) MO your own childList, (2) whenever a slot is added to you, (3) find that slot's host element (recurse up the tree to find the shadowRoot.host) and MO that host's childList, and (4) repeat step 2 for that element. This is just to see childList changes. To get the actual distribution changes to a given slot, you have to observe attributes on every one of these children and then, perhaps, do a dirty checking scheme to detect what the changes are.

The platform has all this information as a result of distributing. For the user to do this correctly is both complicated and requires significant duplication of effort.

annevk commented 9 years ago

@sorvell you'd need to propose a concrete processing model for this event and get buy-in from everyone. I'd really rather not put Shadow DOM back in a state where not everything has buy-in from everyone. Shipping across browsers is more important.

sorvell commented 9 years ago

I definitely do not want to jeopardize buy-in. The momentum that Shadow DOM has right now is great.

I'm hoping this is a relatively easy addition, especially with the simplified slots model that has been adopted.

I think it could be a non-bubbling event fired when MutationObservers are. Perhaps it could just be an addition to MutationObservers, and therefore decoupled from the Shadow DOM spec.

esprehn commented 9 years ago

Preferably this would just be an async notification that your distribution changed, perhaps even just MutationObserver can have a type of "distributedChildList" ?

hayatoito commented 8 years ago

Let me add label v1 to this issue to get more attention.

rniwa commented 8 years ago

This is about getting notified when the final distribution is changed or when assigned nodes are changed? If former, I'm opposed to it at least in v1.

JanMiksovsky commented 8 years ago

Generally speaking, I'm excited to see Shadow DOM v1 ship sooner rather than later, even if it means living without the ability to detect changes in slot assignment as discussed here.

That said, I want to make clear that, without this feature, I believe it's prohibitively difficult to build components that can be composed as people expect. We would like people to be able to build custom elements that are every bit as reliable as the standard HTML elements, and have proposed a set of criteria for evaluating that level of quality called the Gold Standard checklist. One of the checklist items, Content Reprojection explicitly encourages component authors to properly handle reprojected content, even when that content changes. The example described on that page — counting the number of children in the final distribution — is trivial, but this issue has come up in practice time and again.

As a more practical example, the Basic Web Components project includes a <basic-carousel> component that implements a sophisticated carousel UI, with full keyboard support, screen reader support, touch/trackpad swiping, etc. People have tried to compose <basic-carousel> into their own components, with mixed results. Among other things, the carousel can show a set of dots along the bottom of the carousel, with one dot corresponding to each assigned node. (Demo) The component does an enormous amount of work — very much along the lines of @sorvell's solution using MutationObserver — but there are still cases the component doesn't handle properly. One result is that the number of dots can fail to update properly. So while this carousel can be useful as-is, it's quite brittle when composed into other components.

Again, I'm not saying Shadow DOM v1 should be held up for this feature. (I'd rather having something than nothing.) All I want to do is make plain the very real limitations of not having this feature.

rniwa commented 8 years ago

@JanMiksovsky : Do you want an event to detect when the "distributed" (i.e. after unwrapping / flattening slots) nodes are changed or when assigned nodes are changed? Also, do you want this event be synchronous or asynchronous?

JanMiksovsky commented 8 years ago

As a component author, I want to be careful to focus on the scenarios I encounter while writing production components, and leave questions about the actual API design to browser engineers. With that in mind, I only want to call out the component requirements I see here that bear on this question.

In our basic-carousel example, the carousel wants to know how many children (e.g., img elements) it is showing, in order that it might show the correct number of dots. As I understand the distributed/assigned distinction, I believe that basic-carousel wants to know about changes to the set of nodes distributed to its default slot.

If basic-carousel only learned when the set of nodes assigned its default slot had changed, that would seem insufficient to support scenarios in which basic-carousel is composed into other components. For example, some users of basic-carousel want to compose a basic-carousel instance into a component of their own. Let’s call their new component outer-carousel. They create a default <slot> for outer-carousel and place that slot inside the composed basic-carousel instance. Here, the only node assigned to basic-carousel’s default slot would always be outer-carousel’s slot. If basic-carousel only hears about direct assignment changes to its own slot, it would never learn when a new image has been added to outer-carousel.

(If I’ve misunderstood things here, or there’s some other way in which basic-carousel could achieve its composability objectives, please let me know.)

Regarding timing, I don’t think basic-carousel presents any requirement for synchronous timing; asynchronous notification would be fine. Ideally, it would learn about distribution changes before the changes have rendered (so that there’s no visible lag between the appearance of a new image in the carousel and the appearance of a new dot for it). But beyond that, I don’t see any special timing needs.

rniwa commented 8 years ago

After discussing with @JanMiksovsky in person today, I'm pretty convinced that we need to add this event. I think the question is the timing (sync vs. async vs. end-of-mirotask/nanotask) as well as the name. I would like to stay away from the term "distribution" as much as possible.

hayatoito commented 8 years ago

+1. I'm super positive to add this feature.

hayatoito commented 8 years ago

I'm also very welcome any concrete proposal if someone can write it.

I need a help from an expert, if we are going to use MutationObserver.

sorvell commented 8 years ago

It's great to hear 2 major vendors being open to this feature.

The Polymer library recently added support for this type of notification. It is a lot of code to manage and the performance degrades in complex cases due to the need to manage sets of Mutation Observers.

Polymer is fine with an asynchronous notification, ideally with the same basic behavior as Mutation Observers.

rniwa commented 8 years ago

If this follows MutationObserver timing, then it should probably be a new MutationRecord type.

travisleithead commented 8 years ago

+1 for adding to MutationObserver records

sorvell commented 8 years ago

This was discussed at the recent custom elements F2F meeting. I believe there was general agreement that this a critical feature needed for webcomponents 'v1' implementations and that

rniwa commented 8 years ago

Here's a concrete API proposal:

Add boolean slotAssignments = false to MutationObserverInit. We'll also add a mutation record of type slotAssignment.

On a mutation record of this type: target returns the slot element whose list of distributed nodes have been mutated; addedNodes and removedNodes always returns an empty array. previousSibling, nextSibling, attributeName, attributeNamespace, and oldValue all always return null.

We can add a new step to each DOM mutation such as the concept insert to queue a mutation record of type slotAssignment. This would result in each slot synchronously getting a new mutation record of type slotAssignment whenever a slot assignment has changed. This is observable if the mutation observer's takeRecords() is called. If there are multiple mutations that affect slot assignments, each such mutation queues a new record in this model.

Alternatively, we can store the list of distributed nodes for each slot element at the beginning of each micro task, and queue a mutation record if the list of nodes had changed at the end of the micro task.

sorvell commented 8 years ago

Would the user facing api by like this?

new MutationObserver(function() {
  // react to slot changes...
}).observe(slotElement, {slotAssignment: true});

If so, this seems fine. Thanks!

ajklein commented 8 years ago

Is there a reason not to just use an event for this? Creating a new MutationRecord type that contains none of the usual MutationRecord fields seems odd. The trickiest part of using an event is getting it to fire at microtask timing, but I suspect we already have enough spec machinery to do that.

rniwa commented 8 years ago

Yeah, now that I'm thinking about this more, it seems more natural to add an event on HTMLSlotElement whenever the distributed nodes change.

hayatoito commented 8 years ago

I'm afraid that an event will be too spammy if we define the timing of "distributed node change" naively.

The trickiest part is how we can define the timing of "distribution nodes change" so that we do not have an interoperability issue.

In the current spec, UA can delay the calculation of distributed nodes until it is actually demanded as long as developers can not see the difference.

rniwa commented 8 years ago

We can just check that at the end of each micro task.

hayatoito commented 8 years ago

That might be unacceptable because it could be super expensive.

To make the topic more understandable, suppose that if we had an imaginary "ComputedStyle Change" event, when should we dispatch such a "ComputedStyle change" event?

UA do not want to calculate ComputedStyle at the end of each micro task, I think.

Does someone have any idea? I am aware that supporting this feature request, "Slotting change event", is very important for developers. I need a workable solution.

rniwa commented 8 years ago

No, checking this condition is pretty easy & efficient since you can determine whether a node belongs to a given slot or not in O(1). Essentially, each shadow root needs to store the names of all slots its tree contains in a hash set/map. When its host' children changes, it can notify the shadow root that the slot assignments have changed for a specific name / default slot (keep repeating this process if the slot's parent also has a shadow root if you wanted to get notified of changes in distributed nodes).

Now, at the end of micro task, you visit all those shadow roots that have been notified of such potential changes and those that do have relevant event listeners. If the slot element had been inserted or removed from a given shadow root, you may need to resolve the first slot for each slot name in O(n). Otherwise, dispatch the event for each slot for which the shadow root had been notified of the change in its node assignments.

Having said that, I'm not opposed to running this at the end of the task (before painting essentially) although that might be painfully late for some use cases. Exposing style resolution timing is not acceptable for us.

hayatoito commented 8 years ago

We are supposed to support an event of change of slot.getAssignedNodes({flatten: true}), rather than slot.getAssingedNodes(), aren't we?

In this case, one DOM mutation potentially would have a global effect, which would cause the update of distributed nodes of any slots in descendant trees. Yeah, this is a cascading effect and this could not be done in O(1). In practice, this might not be so expensive, but this could be expensive, in theory.

Blink requires a huge engine change if Blink supports this in an efficient way due to the historical reason: Blink has to support both v0 and v1 distributions. :(

If we are supposed to support only the change of "slot.getAssingedNodes", yeah, it would be simpler. That could be possible in O(1) because it has only local effect within one shadow tree.

hayatoito commented 8 years ago

Thus, here is a question:

Is slot.getAssingedNodes() changed event, instead of slot.getAssingedNodes({flatten: true}) changed event, good enough for developers, as the first step? In other words, can developers live in a world without the support of slot.getAssingedNodes({flatten: true}) changed event?

@sorvell , @JanMiksovsky WDTY?

sorvell commented 8 years ago

To support the use cases described in this issue, the notification must be for changes to distributed nodes.

Because <slot> can be composed, an assigned nodes notification is not enough.

rniwa commented 8 years ago

We are supposed to support an event of change of slot.getAssignedNodes({flatten: true}), rather than slot.getAssingedNodes(), aren't we?

In this case, one DOM mutation potentially would have a global effect, which would cause the update of distributed nodes of any slots in descendant trees. Yeah, this is a cascading effect and this could not be done in O(1). In practice, this might not be so expensive, but this could be expensive, in theory.

Not necessarily. It all depends on how you implement this feature but there is a strategy by which detecting that is O(1) as well. For example, when a slot element A is inserted as a child of a shadow host C, it could look up whether A is assigned to another slot B in its shadow DOM of C. If it is, it can notify the shadow root that contains B as a direct descendent that B now contains C. We can then store reference to B's shadow root in A. When a node is assigned into A, it can then notify B via its shadow root. You can do this recursively so that A remembers all slots into which it could be flattened into. One downside of this approach is that it could result in O(k) storage space where k is the number of slots into which a slot gets flattened through as well as potential O(k) runtime when a slot is inserted into / removed from a shadow DOM on the premise that slots are rarely inserted/removed.

Alternatively, you can take the approach I described earlier, and walk up the "flattening chain" in O(k). I think this latter approach is better because a node getting kept being re-distribute in rare in practice and it tends to be bounded by a small number and doesn't require an extra storage in memory.

Anyway, this O(k) runtime/memory cost is precisely the reason we didn't want to support detecting changes to the list of distributed nodes but if all JS libraries are going to implement themselves, it would be MUCH worse.

hayatoito commented 8 years ago

@sorvell , thanks! I understand the the notification must be for changes to distributed nodes.

Let me raise the priority of this feature request. I need some time to make a proof of a concept in Blink to make sure the feasibility of this feature before writing a spec.

Anyway, this O(k) runtime/memory cost is precisely the reason we didn't want to support detecting changes to the list of distributed nodes but if all JS libraries are going to implement themselves, it would be MUCH worse

Yeah, I totally agree. We do not want JS libraries to implement this. It would be MUCH worse.

treshugart commented 8 years ago

Hopefully you guys don't mind me chiming in (I'm new here). I'd +1 a MutationObserver implementation, or something that does mutation batching with microtask timing.

@ajklein re:

Is there a reason not to just use an event for this? Creating a new MutationRecord type that contains none of the usual MutationRecord fields seems odd. The trickiest part of using an event is getting it to fire at microtask timing, but I suspect we already have enough spec machinery to do that.

I assume you mean non-bubbling as per a previous comment you'd made. Would these events batch together mutations or would they fire for every mutation that was made?

FWIW, I'd vote for a MutationObserver implementation for a few reasons:

ajklein commented 8 years ago

@treshugart I wasn't suggesting an event-per-distributed-node, but rather an event-per-slot-element-distributed-to. I'd expect that MutationObserver wouldn't be able, in the common case, to provide much batching here, since each slot element would likely have a different MutationObserver observing it (owned by the custom element whose slot element is affected).

rniwa commented 8 years ago

Whether we do batching or not is a separate question from whether we use MutationObservers or not. We can always spec the event to be fired at the end of micro-task.

hayatoito commented 8 years ago

@sorvell

If we use an event, how the user-facing event should be?

slot.addEventListner("slotted", (e) => {
  console.log(e.target) // -> slot
  console.log(e.target.getAssignedNodes({flatten: true}) // -> Show the slot's current distributed nodes, which is not the snapshot when the event happened.
});

Is that enough as an API? I am assuming that we do not need addedNodes / removedNodes / oldAssignedNodesBeforeChange or anything else. In other words, the event will not have any new property.

treshugart commented 8 years ago

@ajklein ah ok, that makes sense then. The event API does have better ergonomics.

@hayatoito re:

Is that enough as an API?

I'd expect to have some way of seeing what was added and / or removed. One thing that comes to mind is a todo list that persists changes. I would expect to be able to listen for slotted events and then persist the changes to some sort of store. I could somehow get the previous state and diff it with the slot.getAssignedNodes(), but that would be a lot of effort for something that could easily be provided to me by the event.

For example, if the event is triggered for every distributed node:

slot.addEventListener('slotted', function (e) {
  console.log(e.target); // -> <slot />
  console.log(e.addedNode); // -> `null` or `Node`
  console.log(e.removedNode); // -> `null` or `Node`
});

Or if they're batched into a single event:

slot.addEventListener('slotted', function (e) {
  console.log(e.target); // <slot />
  console.log(e.addedNodes); // `null` or `DOMNodeList`
  console.log(e.removedNodes); // `null` or `DOMNodeList`
});

I have my reservations about the former since I have first-hand experience with polyfilling mutation observers with mutation events, as well as with using them in massive DOMs (Jira, BitBucket, etc) and the effects on performance they have. Though, if they're non-bubbling, then maybe it's not an issue? Either way, I'm not sure if there's any way to hide the implementation detail of node vs node-list if this information is exposed, so I'd say deciding whether it will be batched or not should be part of this issue.

rniwa commented 8 years ago

We don't want to provide the list of nodes being inserted or removed because computing that list requires O(n) operation especially in the "distributed" nodes case. Since authors can easily store the list of flattened assigned nodes (a.k.a. distributed nodes), we should just let authors take the diff as required in a given use case. We shouldn't be penalizing all other use cases in which the list of inserted or removed nodes are not needed.

treshugart commented 8 years ago

Yeah, true, especially since they'll be catering the diff to their use-cases and may not even need that information. +1 for keeping it simple, especially for v1.

Thanks for putting up with me :)

hayatoito commented 8 years ago

Now, I am working on a proof of a concept to support slotchange event, a tentative name, in Blink.

I am thinking that slotchange event's scoped flag must be true in default. The reason: it does not make much sense that a node in a parent tree receives a slotchange event which is dispatched in a slot in a child tree. From the view of a node in the parent tree, the event's target is not the slot element due to event target retargeting.

We might want to scope slotchange events within a shadow tree.

WDTY?

rniwa commented 8 years ago

I don't think slotchange event should bubble at all. It should only fire on slot elements themselves through which a node is distributed.

hayatoito commented 8 years ago

Yeah, I agree, but the point is that we still have a capturing phase. It does not seem that the current event spec has a way to skip a capturing phase.

Is there any existing event which skips a capturing phase? If we have a precedent, I support that slotchange should skip a capturing phase as well as a bubbling phase, too.

hayatoito commented 8 years ago

If we have none, how about introducing preventCapturing property, (any better name is welcome), to an Event to control a capturing phase?

dictionary EventInit {
  boolean bubbles = false;
  boolean preventCapturing = false;
  boolean cancelable = false;
};

I think our engines would get a benefit becuase it reduces a burden of an event dispatching.

TakayoshiKochi commented 8 years ago

XHR (onreadystatechange event et al.) works similar, though the target is not a object in DOM tree, it doesn't capture/bubble, IIUC.

hayatoito commented 8 years ago

"XHR" has a capturing phase, in theory, in an event dispatching. We can not obverse a capturing phase because its event path is empty.

In a slotchage event dispatching, we have a non-empty event path.

annevk commented 8 years ago

We haven't really defined yet how you obtain the event path. Say that you obtain the event path from the target. We could make it so that if you obtain the event path from slot elements, it's empty, but maybe that is making slot too magical again as it would affect all events dispatched on it. (If we did do this I'd need to think it through a bit more.)

Speaking about events, if slot elements are part of the composed tree in WebKit, are they part of the event path too then?

hayatoito commented 8 years ago

Yeah, AFAIK, we do not have any mechanism to make an event path be empty when the target node has a parent node. In other words, a parent node should always receive an event (at least in a capturing phase) when its child node receives an event. We should honor this.

Since we do not want to make a slotchange event too magical, all we can do here is to make slotchange event's scoped flag be true. That's the best we can do, if we use an event.

Speaking about events, if slot elements are part of the composed tree in WebKit, are they part of the event path too then?

Slots and shadow roots can be part of the event path, as per the spec. That's intentionally designed. In short: event path == (ancestors in the flat tree) + (shadow roots) + (slots)

This section explains why I designed an event path in such a way: http://w3c.github.io/webcomponents/spec/shadow/#event-paths-example

hayatoito commented 8 years ago

FYI. I've made a proof of concept for a slotchange event in Blink. https://codereview.chromium.org/1695163003

It's still premature and I'm still wondering how to define this event precisely, e.g. the timing, the condition, the number of the event dispatching per slot, and so on. However, I can say supporting this event is doable at some level.

If there is an early adopter, please play with this and give us early feedback. I'll try to write a spec later if I can feel it's ready.

esprehn commented 8 years ago

I don't think this should be an event, using an event is much heavier weight and doesn't really make sense since the event path itself depends on slotting, it also seems super weird to fire this event at microtask timing instead of just using MutationObserver which is the "observe the DOM changing at microtask" system that already exists. This is duplicating that notification system, but using way more CPU and power to do it. ex. if 10 slots change, this fires 10 events, which are very expensive. Dispatching 10 mutation records is massively cheaper.