1409976 - Add `slotchange` event

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Jessica Jong [:jessica] (inactive)]

https://dom.spec.whatwg.org/#signaling-slot-change

Comment 1

[Jessica Jong [:jessica] (inactive)]

My plan for implementing this is as below:

1. Add a `signal slot list` in nsDOMMutationObserver (should be an array of HTMLSlotElement)
2. To signal a slot change, append the slot to the `signal slot list` and call `nsDOMMutationObserver::ScheduleForRun()`
3. In `nsDOMMutationObserver::HandleMutation()`, fire `slotchange` event for each slot in the `signal slot list` and clear the list

Olli, do you think this is the right approach?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I'm not sure about 1. Where would you add the list there? What if there are no MutationObservers?
Couldn't DocGroup have the list ("unit of related similar-origin browsing contexts"), and then when 
nsDOMMutationObserver::HandleMutations() is called, one would check whether there are pending slot changes, and not only pending mutations.
There should be then a list of pending DocGroups or slotchanges

Comment 3

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #2)
> I'm not sure about 1. Where would you add the list there? What if there are
> no MutationObservers?
> Couldn't DocGroup have the list ("unit of related similar-origin browsing
> contexts"), and then when 
> nsDOMMutationObserver::HandleMutations() is called, one would check whether
> there are pending slot changes, and not only pending mutations.
> There should be then a list of pending DocGroups or slotchanges

Oh, right, nsDOMMutationObserver exists only when there MutationObservers, so putting the list in DocGroup makes more sense. Thanks.

Comment 4

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: add support for signaling slotchange, v1.

Comment 5

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: Signal slotchange, v1.

Comment 6

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8937888 [details] [diff] [review]
Part 2: Signal slotchange, v1.

Review of attachment 8937888 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMMutationObserver.cpp
@@ +925,5 @@
>      delete DocGroup::sPendingDocGroups;
>      DocGroup::sPendingDocGroups = nullptr;
>    }
>  
> +  if (sScheduledMutationObservers) {

I found this issue when running this testcase: https://searchfox.org/mozilla-central/rev/ff462a7f6b4dd3c3fdc53c9bc0b328f42a5b3d2b/testing/web-platform/tests/shadow-dom/slotchange-event.html#540-594.

Mutations generated in mutation observer handlers should be delivered in the next microtask checkpoint, so we don't need to keep processing sScheduledMutationObservers here.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

They should be delivered during the same microtask checkpoint but during different microtask.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

and we do that here https://searchfox.org/mozilla-central/rev/ff462a7f6b4dd3c3fdc53c9bc0b328f42a5b3d2b/xpcom/base/CycleCollectedJSContext.cpp#514-523

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8937887 [details] [diff] [review]
Part 1: add support for signaling slotchange, v1.

>+HTMLSlotElement::FireSlotChangeEvent()
>+{
>+  nsContentUtils::DispatchTrustedEvent(OwnerDoc(),
>+                                       static_cast<nsIContent*>(this),
>+                                       NS_LITERAL_STRING("slotchange"), true,
>+                                       true);
false. Shouldn't be cancelable.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8937887 [details] [diff] [review]
Part 1: add support for signaling slotchange, v1.

>+  const nsTArray<RefPtr<HTMLSlotElement>>& GetSignalSlotList() const
Nit, this should be without Get -prefix.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8937888 [details] [diff] [review]
Part 2: Signal slotchange, v1.

>+  // If parentâs root is a shadow root, and parent is a slot whose assigned
Something weird in the comment

>+  if (aContainer && aContainer == GetHost()) {
>+    nsAutoString slotName;
>+    aChild->GetAttr(kNameSpaceID_None, nsGkAtoms::slot, slotName);
>+    if (const HTMLSlotElement* slot = UnassignSlotFor(aChild, slotName)) {
>+      slot->EnqueueSlotChangeEvent();
>+    }
>+    return;
>+  }
>+
>+  // If parentâs root is a shadow root, and parent is a slot whose assigned
Some weird characters in the comment.


The patch 1 may enqueue compound task even though there is already such queued, but the latter task just does then nothing so it should be ok.

Comment 12

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #8)
> and we do that here
> https://searchfox.org/mozilla-central/rev/
> ff462a7f6b4dd3c3fdc53c9bc0b328f42a5b3d2b/xpcom/base/CycleCollectedJSContext.
> cpp#514-523

Ah, right, so it's in the same microtask checkpoint. Thanks for the correction.

Comment 13

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: add support for signaling slotchange, v2.

Addressed review comment 9 and 10.

Comment 14

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: Signal slotchange, v2.

fixed weird characters in the comment.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d6f43b1c0ef
Part 1: Add support for `slotchange` event. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/51a3a1b8513c
Part 2: Signal `slotchange` when slot's assigned nodes changes. r=smaug

Comment 16

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/1d6f43b1c0ef
https://hg.mozilla.org/mozilla-central/rev/51a3a1b8513c

Comment 17

[Gerry Chang [:gchang]]

Too late for FF58. Mark 58 won't fix.

Comment 18

[Chris Mills [:cmills]]

Documented:

https://developer.mozilla.org/en-US/docs/Web/Events/slotchange