Skip to content
This repository has been archived by the owner on Jun 24, 2022. It is now read-only.

Commit

Permalink
slotchange event doesn't get fired when inserting, removing, or renam…
Browse files Browse the repository at this point in the history
…ing slot elements

https://bugs.webkit.org/show_bug.cgi?id=189144
<rdar://problem/43871061>

Reviewed by Antti Koivisto.

LayoutTests/imported/w3c:

* web-platform-tests/shadow-dom/slotchange-customelements-expected.txt:
* web-platform-tests/shadow-dom/slotchange-event-expected.txt:
* web-platform-tests/shadow-dom/slotchange-expected.txt:

Source/WebCore:

This patch implements `slotchange` event when a slot element is inserted, removed, or renamed in the DOM tree.
Let us consider each scenario separately.

Insertion (https://dom.spec.whatwg.org/#concept-node-insert): In this case, we must fire `slotchange` event on
slot elements whose assigned nodes have changed in the tree order. When there is at most one slot element for
each name, this can be done by simply checking whether each slot has assigned nodes or not. When there are more
than one slot element, however, the newly inserted slot element may now become the first slot of a given name,
and gain assined nodes while the previously first element loses its assigned nodes. To see if the newly inserted
slot element is the first of its kind, we must travere the DOM tree to check the order of that and the previously
first slot element. To do this, we resolve the slot elements before start inserting nodes in
executeNodeInsertionWithScriptAssertion via SlotAssignment::resolveSlotsBeforeNodeInsertionOrRemoval. Note that
when the DOM tree has at most one slot element of its kind, resolveSlotsBeforeNodeInsertionOrRemoval is a no-op
and addSlotElementByName continues to operate in O(1). Becasue addSlotElementByName is called on each inserted
slot element in the tree order, we do the tree traversal upon finding the first slot element which has more than
one of its kind in the current tree. In this case, we resolve all other slot elements and enqueues slotchange
event as needed to avoid doing the tree traversal more than once.

Removal (https://dom.spec.whatwg.org/#concept-node-remove): In removal, we're concerned with removing the first
slot element of its kind. We must fire slotchange event on the remaining slot elements which became the first of
its kind after the removal as well as the ones which got removed from the tree if they had assigned nodes.
Furthermore, the DOM specification mandates that we first fire slotchange events in the tree from which a node
was removed and then in the removed subtree. Because we must only fire slotchange event on the first slot element
of its kind which has been removed, we must resolve the first slot elements of each kind before a node removal
in removeAllChildrenWithScriptAssertion and removeNodeWithScriptAssertion as we've done for insertion. Again,
in the case there was at most one slot element of each kind, resolveSlotsBeforeNodeInsertionOrRemoval is a no-op
and removeSlotElementByName would continue to operate in O(1). When there are multiple slot elements for a given
kind, we immediately enqueue slotchange event on the slot elements which newly became the first of its kind but
delay the enqueuing of slotchange event on the removed slot elements until removeSlotElementByName is called on
that element so that enqueuing of slotchange events on the slot elements still remaining in the in the tree would
happen before those which got removed as the DOM specification mandates.

Rename (https://dom.spec.whatwg.org/#shadow-tree-slots): In the case the slot element's name content attribute
is changed, the renamed element might have become the first of its kind or ceased to be the first of its kind.
There could be two other slot elements appearing later in the tree order which might have gained or lost assigned
nodes as a result. In this case, we invoke the algorithms for removing & inserting the slot with a key difference:
we enqueue slotchange event on the renamed slot immediately if it has assigned nodes.

To enqueue slotchange event in the tree order, this patch adds oldElement, which is a WeakPtr to a slot element,
to SlotAssignment::Slot. This WeakPtr is set to the slot element which used to have assigned nodes prior to the
node insertion, removal, or rename but no longer has after the mutation. This patch also adds a slot mutation
version number, m_slotMutationVersion, which is incremented whenever a node is about to be inserted or removed,
and slot resolution version, m_slotResolutionVersion, which is set to the current slot mutation version number
when the full slot resolution is triggered during slot mutations. They are used to avoid redundant tree traversals
in resolveSlotsAfterSlotMutation. This patch also makes m_needsToResolveSlotElements compiled in release builds
to avoid resolving slot elements when there is at most one slot element of each kind.

For insertion, oldElement is set to the slot which used to be the first of its kind before getting set to a slot
element being inserted in resolveSlotsAfterSlotMutation. We enqueue slotchange event on the newly inserted slot
element at that point (1). Since the slot element which used to be the first of its kind appears after the newly
inserted slot element by definition, we're guaranteed to see this oldElement later in the tree traversal upon
which we enqueue slotchange event. Note that if this slot element was the first of its kind, then we're simply
hitting (1), which is O(1) and does not invoke the tree traversal.

For removal, oldElement is set to the slot which used to be the first of its kind. Because this slot element is
getting removed, slotchange event must be enqueud after slotchange events have been enqueued on all slot elements
still remaining in the tree. To do this, we enqueue slotchange event immediately on the first slot element of
its kind during the tree traversal as we encounter it (2), and set oldElement to the previosuly-first-but-removed
slot element. slotchange event is enqueued on this slot element when removeSlotElementByName is later invoked via
HTMLSlotElement::removedFromAncestor which traverses each removed element in the tree order. Again, if this was
the last slot of its kind, we'd simply expedite (2) by enqueuing slotchange event during removeSlotElementByName,
which is O(1).

When the DOM invokes the concept to replace all children (https://dom.spec.whatwg.org/#concept-node-replace-all),
however, this algorithm isn't sufficient because the removal of each child happens one after another. We would
either need to resolve slots between each removal, or treat the removal of all children as a single operation.
While the DOM specification currently specifies the former behavior, this patch implements the latter behavior
to avoid useless work. See the DOM spec issue: WICG/webcomponents#764

Test: fast/shadow-dom/slotchange-for-slot-mutation.html

* dom/ContainerNode.cpp:
(WebCore::ContainerNode::removeAllChildrenWithScriptAssertion): Call resolveSlotsBeforeNodeInsertionOrRemoval
before start removing children.
(WebCore::ContainerNode::removeNodeWithScriptAssertion): Ditto.
(WebCore::executeNodeInsertionWithScriptAssertion): Ditto before inserting children.
* dom/ShadowRoot.cpp:
(WebCore::ShadowRoot::~ShadowRoot): Set m_hasBegunDeletingDetachedChildren to true. This flag is used to supress
slotchange events during the shadow tree destruction.
(WebCore::ShadowRoot::renameSlotElement): Added.
(WebCore::ShadowRoot::removeSlotElementByName): Added oldParentOfRemovedTree as an argument.
* dom/ShadowRoot.h:
(WebCore::ShadowRoot::shouldFireSlotchangeEvent): Added.
* dom/SlotAssignment.cpp:
(WebCore::findSlotElement): Added.
(WebCore::nextSlotElementSkippingSubtree): Added.
(WebCore::SlotAssignment::hasAssignedNodes): Added. Returns true if the slot of a given name has assigned nodes.
(WebCore::SlotAssignment::renameSlotElement): Added.
(WebCore::SlotAssignment::addSlotElementByName): Call resolveSlotsAfterSlotMutation when slotchange event needs
to be dispatched for the current slot and there are more than one slot elements.
(WebCore::SlotAssignment::removeSlotElementByName): Ditto. When the slot's oldElement is set to the current slot
element, meaning that this slot element used to have assigned nodes, then enqueue slotchange event. It also has
a special case for oldParentOfRemovedTree is null when renaming a slot element. In this case, we want to enqueue
slot change event immediately on the renamed slot element and any affected elements as in a node insertion since
removeSlotElementByName would never be called on a slot element which newly become the first of its kind after
a slot rename.
(WebCore::SlotAssignment::resolveSlotsAfterSlotMutation): Added. This is the slot resolution algorithm invoked
when there are more than one slot elements for a given name. It has two modes dealing with insertion & removal.
The insertion mode is also used for renaming a slot element. The firs
(WebCore::SlotAssignment::resolveSlotsBeforeNodeInsertionOrRemoval): Added. Resolves all slot elements prior to
inserting or removing nodes. In many cases, this should be a no-op since m_needsToResolveSlotElements is set to
true only when there are more than one slot element of its kind.
(WebCore::SlotAssignment::willRemoveAllChildren): Ditto. Also sets m_willBeRemovingAllChildren to true.
(WebCore::SlotAssignment::didChangeSlot):
(WebCore::SlotAssignment::resolveAllSlotElements): Use seenFirstElement instead of element to indicate whether
we have seen a slot element of given name for consistency with resolveSlotsAfterSlotMutation.
* dom/SlotAssignment.h:
(WebCore::SlotAssignment::Slot): Added oldElement and seenFirstElement.
(WebCore::SlotAssignment): Always compile m_needsToResolveSlotElements. Added m_willBeRemovingAllChildren,
m_slotMutationVersion, and m_slotResolutionVersion.
(WebCore::ShadowRoot::resolveSlotsBeforeNodeInsertionOrRemoval): Added. Calls the one in SlotAssignment.
(WebCore::ShadowRoot::willRemoveAllChildren): Ditto.
* html/HTMLSlotElement.cpp:
(WebCore::HTMLSlotElement::removedFromAncestor):
(WebCore::HTMLSlotElement::attributeChanged): Calls ShadowRoot::renameSlotElement instead of
removeSlotElementByName and addSlotElementByName pair.

LayoutTests:

Added a W3C style testharness.js test for inserting, removing, and renaming slot elements.

It has 62 distinct test cases for closed/open shadow roots in connected and disconnected trees
for the total of 248 test cases.

This test presumes the resolution of WICG/webcomponents#764 in our favor.

Chrome fails 48 test cases because it doesn't follow the tree order when dispatching slotchange event
on the previously first slot element, and Firefox fails 84 test cases because it fails to fire slotchange
in the tree order when a node is inserted.

* fast/shadow-dom/slotchange-for-slot-mutation-expected.txt: Added.
* fast/shadow-dom/slotchange-for-slot-mutation.html: Added.


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@235650 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
[email protected] committed Sep 5, 2018
1 parent e70478b commit c0b08af
Show file tree
Hide file tree
Showing 14 changed files with 967 additions and 53 deletions.
22 changes: 22 additions & 0 deletions LayoutTests/ChangeLog
Original file line number Diff line number Diff line change
@@ -1,3 +1,25 @@
2018-09-04 Ryosuke Niwa <[email protected]>

slotchange event doesn't get fired when inserting, removing, or renaming slot elements
https://bugs.webkit.org/show_bug.cgi?id=189144
<rdar://problem/43871061>

Reviewed by Antti Koivisto.

Added a W3C style testharness.js test for inserting, removing, and renaming slot elements.

It has 62 distinct test cases for closed/open shadow roots in connected and disconnected trees
for the total of 248 test cases.

This test presumes the resolution of https://github.com/w3c/webcomponents/issues/764 in our favor.

Chrome fails 48 test cases because it doesn't follow the tree order when dispatching slotchange event
on the previously first slot element, and Firefox fails 84 test cases because it fails to fire slotchange
in the tree order when a node is inserted.

* fast/shadow-dom/slotchange-for-slot-mutation-expected.txt: Added.
* fast/shadow-dom/slotchange-for-slot-mutation.html: Added.

2018-09-04 Simon Fraser <[email protected]>

CSS reference filter that references a tiled feTurbulence is blank
Expand Down
250 changes: 250 additions & 0 deletions LayoutTests/fast/shadow-dom/slotchange-for-slot-mutation-expected.txt

Large diffs are not rendered by default.

310 changes: 310 additions & 0 deletions LayoutTests/fast/shadow-dom/slotchange-for-slot-mutation.html

Large diffs are not rendered by default.

12 changes: 12 additions & 0 deletions LayoutTests/imported/w3c/ChangeLog
Original file line number Diff line number Diff line change
@@ -1,3 +1,15 @@
2018-09-04 Ryosuke Niwa <[email protected]>

slotchange event doesn't get fired when inserting, removing, or renaming slot elements
https://bugs.webkit.org/show_bug.cgi?id=189144
<rdar://problem/43871061>

Reviewed by Antti Koivisto.

* web-platform-tests/shadow-dom/slotchange-customelements-expected.txt:
* web-platform-tests/shadow-dom/slotchange-event-expected.txt:
* web-platform-tests/shadow-dom/slotchange-expected.txt:

2018-09-04 Rob Buis <[email protected]>

Adjust XMLHttpRequest username/password precedence rules
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@

FAIL slotchange must fire on initialization of custom elements with slotted children assert_true: expected true got false
PASS slotchange must fire on initialization of custom elements with slotted children

Original file line number Diff line number Diff line change
@@ -1,9 +1,3 @@
CONSOLE MESSAGE: line 2659: Error: assert_equals: slotchange must not be fired on a slot element if the assigned nodes changed after the slot was removed expected 2 but got 0
CONSOLE MESSAGE: line 2659: Error: assert_equals: slotchange must not be fired on a slot element if the assigned nodes changed after the slot was removed expected 2 but got 0
CONSOLE MESSAGE: line 2659: Error: assert_equals: slotchange must not be fired on a slot element if the assigned nodes changed after the slot was removed expected 2 but got 0
CONSOLE MESSAGE: line 2659: Error: assert_equals: slotchange must not be fired on a slot element if the assigned nodes changed after the slot was removed expected 2 but got 0

Harness Error (FAIL), message = Error: assert_equals: slotchange must not be fired on a slot element if the assigned nodes changed after the slot was removed expected 2 but got 0

PASS slotchange event must fire on a default slot element inside an open shadow root in a document
PASS slotchange event must fire on a default slot element inside a closed shadow root in a document
Expand All @@ -17,10 +11,10 @@ PASS slotchange event must not fire on a slot element inside an open shadow root
PASS slotchange event must not fire on a slot element inside a closed shadow root in a document when another slot's assigned nodes change
PASS slotchange event must not fire on a slot element inside an open shadow root not in a document when another slot's assigned nodes change
PASS slotchange event must not fire on a slot element inside a closed shadow root not in a document when another slot's assigned nodes change
FAIL slotchange event must fire on a slot element when a shadow host has a slotable and the slot was inserted and must not fire when the shadow host was mutated after the slot was removed inside an open shadow root in a document assert_equals: slotchange must be fired on a slot element if there is assigned nodes when the slot was inserted expected 1 but got 0
FAIL slotchange event must fire on a slot element when a shadow host has a slotable and the slot was inserted and must not fire when the shadow host was mutated after the slot was removed inside a closed shadow root in a document assert_equals: slotchange must be fired on a slot element if there is assigned nodes when the slot was inserted expected 1 but got 0
FAIL slotchange event must fire on a slot element when a shadow host has a slotable and the slot was inserted and must not fire when the shadow host was mutated after the slot was removed inside an open shadow root not in a document assert_equals: slotchange must be fired on a slot element if there is assigned nodes when the slot was inserted expected 1 but got 0
FAIL slotchange event must fire on a slot element when a shadow host has a slotable and the slot was inserted and must not fire when the shadow host was mutated after the slot was removed inside a closed shadow root not in a document assert_equals: slotchange must be fired on a slot element if there is assigned nodes when the slot was inserted expected 1 but got 0
PASS slotchange event must fire on a slot element when a shadow host has a slotable and the slot was inserted and must not fire when the shadow host was mutated after the slot was removed inside an open shadow root in a document
PASS slotchange event must fire on a slot element when a shadow host has a slotable and the slot was inserted and must not fire when the shadow host was mutated after the slot was removed inside a closed shadow root in a document
PASS slotchange event must fire on a slot element when a shadow host has a slotable and the slot was inserted and must not fire when the shadow host was mutated after the slot was removed inside an open shadow root not in a document
PASS slotchange event must fire on a slot element when a shadow host has a slotable and the slot was inserted and must not fire when the shadow host was mutated after the slot was removed inside a closed shadow root not in a document
PASS slotchange event must fire on a slot element inside an open shadow root in a document even if the slot was removed immediately after the assigned nodes were mutated
PASS slotchange event must fire on a slot element inside a closed shadow root in a document even if the slot was removed immediately after the assigned nodes were mutated
PASS slotchange event must fire on a slot element inside an open shadow root not in a document even if the slot was removed immediately after the assigned nodes were mutated
Expand All @@ -29,10 +23,10 @@ PASS slotchange event must fire on a slot element inside an open shadow root in
PASS slotchange event must fire on a slot element inside a closed shadow root in a document when innerHTML modifies the children of the shadow host
PASS slotchange event must fire on a slot element inside an open shadow root not in a document when innerHTML modifies the children of the shadow host
PASS slotchange event must fire on a slot element inside a closed shadow root not in a document when innerHTML modifies the children of the shadow host
FAIL slotchange event must fire on a slot element inside an open shadow root in a document when nested slots's contents change assert_equals: slotchange event's target must be the inner slot element at 1st slotchange expected Element node <slot></slot> but got Element node <slot></slot>
FAIL slotchange event must fire on a slot element inside a closed shadow root in a document when nested slots's contents change assert_equals: slotchange event's target must be the inner slot element at 1st slotchange expected Element node <slot></slot> but got Element node <slot></slot>
FAIL slotchange event must fire on a slot element inside an open shadow root not in a document when nested slots's contents change assert_equals: slotchange event's target must be the inner slot element at 1st slotchange expected Element node <slot></slot> but got Element node <slot></slot>
FAIL slotchange event must fire on a slot element inside a closed shadow root not in a document when nested slots's contents change assert_equals: slotchange event's target must be the inner slot element at 1st slotchange expected Element node <slot></slot> but got Element node <slot></slot>
PASS slotchange event must fire on a slot element inside an open shadow root in a document when nested slots's contents change
PASS slotchange event must fire on a slot element inside a closed shadow root in a document when nested slots's contents change
PASS slotchange event must fire on a slot element inside an open shadow root not in a document when nested slots's contents change
PASS slotchange event must fire on a slot element inside a closed shadow root not in a document when nested slots's contents change
PASS slotchange event must fire at the end of current microtask after mutation observers are invoked inside an open shadow root in a document when slots's contents change
PASS slotchange event must fire at the end of current microtask after mutation observers are invoked inside a closed shadow root in a document when slots's contents change
PASS slotchange event must fire at the end of current microtask after mutation observers are invoked inside an open shadow root not in a document when slots's contents change
Expand Down
Original file line number Diff line number Diff line change
@@ -1,19 +1,17 @@

Harness Error (TIMEOUT), message = null

PASS slotchange event: Append a child to a host.
PASS slotchange event: Remove a child from a host.
PASS slotchange event: Remove a child before adding an event listener.
PASS slotchange event: Change slot= attribute to make it un-assigned.
TIMEOUT slotchange event: Change slot's name= attribute so that none is assigned. Test timed out
PASS slotchange event: Change slot's name= attribute so that none is assigned.
PASS slotchange event: Change slot= attribute to make it assigned.
TIMEOUT slotchange event: Change slot's name= attribute so that a node is assigned to the slot. Test timed out
PASS slotchange event: Change slot's name= attribute so that a node is assigned to the slot.
PASS slotchange event: Add a fallback content.
PASS slotchange event: Remove a fallback content.
PASS slotchange event: Add a fallback content to nested slots.
PASS slotchange event: Remove a fallback content from nested slots.
TIMEOUT slotchange event: Insert a slot before an existing slot. Test timed out
TIMEOUT slotchange event: Remove a preceding slot. Test timed out
PASS slotchange event: Insert a slot before an existing slot.
PASS slotchange event: Remove a preceding slot.
PASS slotchange event: A slot is assigned to another slot.
PASS slotchange event: Even if distributed nodes do not change, slotchange should be fired if assigned nodes are changed.

Loading

0 comments on commit c0b08af

Please sign in to comment.