Closed Bug 1409976 Opened 2 years ago Closed 2 years ago

Add `slotchange` event

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: jessica, Assigned: jessica)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 2 obsolete files)

Priority: -- → P2
Assignee: nobody → jjong
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?
Flags: needinfo?(bugs)
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
Flags: needinfo?(bugs)
(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.
Attachment #8937887 - Flags: review?(bugs)
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.
Attachment #8937888 - Flags: review?(bugs)
They should be delivered during the same microtask checkpoint but during different microtask.
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.
Attachment #8937887 - Flags: review?(bugs) → review+
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 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.
Attachment #8937888 - Flags: review?(bugs) → review+
(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.
Addressed review comment 9 and 10.
Attachment #8937887 - Attachment is obsolete: true
Attachment #8938266 - Flags: review+
fixed weird characters in the comment.
Attachment #8937888 - Attachment is obsolete: true
Attachment #8938267 - Flags: review+
Pushed by jjong@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/1d6f43b1c0ef
https://hg.mozilla.org/mozilla-central/rev/51a3a1b8513c
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Too late for FF58. Mark 58 won't fix.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.