Attachment 357032 Details for Bug 183586 – Aligns WebKit's behavior with Chrome/Firefox

[patch] Aligns WebKit's behavior with Chrome/Firefox

bug-183586-20181210203202.patch (text/plain), 14.93 KB, created by Ryosuke Niwa on 2018-12-10 20:32:02 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Ryosuke Niwa

Created: 2018-12-10 20:32:02 PST

Size: 14.93 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 239068)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,42 @@
>+2018-12-10  Ryosuke Niwa  <rniwa@webkit.org>
>+
>+        connectedCallback is invoked during the removal of the element inside another element's connectedCallback
>+        https://bugs.webkit.org/show_bug.cgi?id=183586
>+        <rdar://problem/38403504>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Align WebKit's behavior with Chrome/Firefox with regards to https://github.com/w3c/webcomponents/issues/760
>+
>+        After much discussion, it's unclear that there is a clear path forward to fixing the oddness that
>+        the presence of a custom element reaction changes the timing at which another reaction callback gets invoked.
>+        So matching Chrome/Firefox behaviors in this case seems the path of the least resistance to interoperability.
>+
>+        Namely, this patch makes WebKit not insert a custom element to the appropriate element queue when the element
>+        does not have a matching reaction callback. Put it another way, steps 3-5 in would be done before step 6 in:
>+        https://html.spec.whatwg.org/multipage/custom-elements.html#enqueue-a-custom-element-callback-reaction
>+            1. Let definition be element's custom element definition.
>+            2. Let callback be the value of the entry in definition's lifecycle callbacks with key callbackName.
>+            3. If callback is null, then return
>+            4. If callbackName is "attributeChangedCallback", then:
>+                1. Let attributeName be the first element of args.
>+                2. If definition's observed attributes does not contain attributeName, then return.
>+            5. Add a new callback reaction to element's custom element reaction queue, with callback function callback
>+               and arguments args.
>+            6. Enqueue an element on the appropriate element queue given element.
>+
>+        Test: fast/custom-elements/enqueue-custom-element-callback-reactions-inside-another-callback.html
>+
>+        * dom/CustomElementReactionQueue.cpp:
>+        (WebCore::CustomElementReactionQueue::enqueueElementUpgrade):
>+        (WebCore::CustomElementReactionQueue::enqueueConnectedCallbackIfNeeded):
>+        (WebCore::CustomElementReactionQueue::enqueueDisconnectedCallbackIfNeeded):
>+        (WebCore::CustomElementReactionQueue::enqueueAdoptedCallbackIfNeeded):
>+        (WebCore::CustomElementReactionQueue::enqueueAttributeChangedCallbackIfNeeded):
>+        (WebCore::CustomElementReactionQueue::enqueuePostUpgradeReactions):
>+        (WebCore::CustomElementReactionQueue::enqueueElementOnAppropriateElementQueue): Renamed from ensureCurrentQueue.
>+        * dom/CustomElementReactionQueue.h:
>+
> 2018-12-10  Don Olmstead  <don.olmstead@sony.com>
> 
>         Move ENABLE_RESOURCE_LOAD_STATISTICS to FeatureDefines.xcconfig
>Index: Source/WebCore/dom/CustomElementReactionQueue.cpp
>===================================================================
>--- Source/WebCore/dom/CustomElementReactionQueue.cpp	(revision 239062)
>+++ Source/WebCore/dom/CustomElementReactionQueue.cpp	(working copy)
>@@ -120,12 +120,15 @@ void CustomElementReactionQueue::clear()
> void CustomElementReactionQueue::enqueueElementUpgrade(Element& element, bool alreadyScheduledToUpgrade)
> {
>     ASSERT(CustomElementReactionDisallowedScope::isReactionAllowed());
>-    auto& queue = ensureCurrentQueue(element);
>+    ASSERT(element.reactionQueue());
>+    auto& queue = *element.reactionQueue();
>     if (alreadyScheduledToUpgrade) {
>         ASSERT(queue.m_items.size() == 1);
>         ASSERT(queue.m_items[0].type() == CustomElementReactionQueueItem::Type::ElementUpgrade);
>-    } else
>+    } else {
>         queue.m_items.append({CustomElementReactionQueueItem::Type::ElementUpgrade});
>+        enqueueElementOnAppropriateElementQueue(element);
>+    }
> }
> 
> void CustomElementReactionQueue::enqueueElementUpgradeIfDefined(Element& element)
>@@ -152,9 +155,12 @@ void CustomElementReactionQueue::enqueue
>     ASSERT(CustomElementReactionDisallowedScope::isReactionAllowed());
>     ASSERT(element.isDefinedCustomElement());
>     ASSERT(element.document().refCount() > 0);
>-    auto& queue = ensureCurrentQueue(element);
>-    if (queue.m_interface->hasConnectedCallback())
>-        queue.m_items.append({CustomElementReactionQueueItem::Type::Connected});
>+    ASSERT(element.reactionQueue());
>+    auto& queue = *element.reactionQueue();
>+    if (!queue.m_interface->hasConnectedCallback())
>+        return;
>+    queue.m_items.append({CustomElementReactionQueueItem::Type::Connected});
>+    enqueueElementOnAppropriateElementQueue(element);
> }
> 
> void CustomElementReactionQueue::enqueueDisconnectedCallbackIfNeeded(Element& element)
>@@ -163,9 +169,12 @@ void CustomElementReactionQueue::enqueue
>     ASSERT(element.isDefinedCustomElement());
>     if (element.document().refCount() <= 0)
>         return; // Don't enqueue disconnectedCallback if the entire document is getting destructed.
>-    auto& queue = ensureCurrentQueue(element);
>-    if (queue.m_interface->hasDisconnectedCallback())
>-        queue.m_items.append({CustomElementReactionQueueItem::Type::Disconnected});
>+    ASSERT(element.reactionQueue());
>+    auto& queue = *element.reactionQueue();
>+    if (!queue.m_interface->hasDisconnectedCallback())
>+        return;
>+    queue.m_items.append({CustomElementReactionQueueItem::Type::Disconnected});
>+    enqueueElementOnAppropriateElementQueue(element);
> }
> 
> void CustomElementReactionQueue::enqueueAdoptedCallbackIfNeeded(Element& element, Document& oldDocument, Document& newDocument)
>@@ -173,9 +182,12 @@ void CustomElementReactionQueue::enqueue
>     ASSERT(CustomElementReactionDisallowedScope::isReactionAllowed());
>     ASSERT(element.isDefinedCustomElement());
>     ASSERT(element.document().refCount() > 0);
>-    auto& queue = ensureCurrentQueue(element);
>-    if (queue.m_interface->hasAdoptedCallback())
>-        queue.m_items.append({oldDocument, newDocument});
>+    ASSERT(element.reactionQueue());
>+    auto& queue = *element.reactionQueue();
>+    if (!queue.m_interface->hasAdoptedCallback())
>+        return;
>+    queue.m_items.append({oldDocument, newDocument});
>+    enqueueElementOnAppropriateElementQueue(element);
> }
> 
> void CustomElementReactionQueue::enqueueAttributeChangedCallbackIfNeeded(Element& element, const QualifiedName& attributeName, const AtomicString& oldValue, const AtomicString& newValue)
>@@ -183,9 +195,12 @@ void CustomElementReactionQueue::enqueue
>     ASSERT(CustomElementReactionDisallowedScope::isReactionAllowed());
>     ASSERT(element.isDefinedCustomElement());
>     ASSERT(element.document().refCount() > 0);
>-    auto& queue = ensureCurrentQueue(element);
>-    if (queue.m_interface->observesAttribute(attributeName.localName()))
>-        queue.m_items.append({attributeName, oldValue, newValue});
>+    ASSERT(element.reactionQueue());
>+    auto& queue = *element.reactionQueue();
>+    if (!queue.m_interface->observesAttribute(attributeName.localName()))
>+        return;
>+    queue.m_items.append({attributeName, oldValue, newValue});
>+    enqueueElementOnAppropriateElementQueue(element);
> }
> 
> void CustomElementReactionQueue::enqueuePostUpgradeReactions(Element& element)
>@@ -195,18 +210,18 @@ void CustomElementReactionQueue::enqueue
>     if (!element.hasAttributes() && !element.isConnected())
>         return;
> 
>-    auto* queue = element.reactionQueue();
>-    ASSERT(queue);
>+    ASSERT(element.reactionQueue());
>+    auto& queue = *element.reactionQueue();
> 
>     if (element.hasAttributes()) {
>         for (auto& attribute : element.attributesIterator()) {
>-            if (queue->m_interface->observesAttribute(attribute.localName()))
>-                queue->m_items.append({attribute.name(), nullAtom(), attribute.value()});
>+            if (queue.m_interface->observesAttribute(attribute.localName()))
>+                queue.m_items.append({attribute.name(), nullAtom(), attribute.value()});
>         }
>     }
> 
>-    if (element.isConnected() && queue->m_interface->hasConnectedCallback())
>-        queue->m_items.append({CustomElementReactionQueueItem::Type::Connected});
>+    if (element.isConnected() && queue.m_interface->hasConnectedCallback())
>+        queue.m_items.append({CustomElementReactionQueueItem::Type::Connected});
> }
> 
> bool CustomElementReactionQueue::observesStyleAttribute() const
>@@ -273,20 +288,20 @@ inline void CustomElementReactionQueue::
>     }
> }
> 
>-CustomElementReactionQueue& CustomElementReactionQueue::ensureCurrentQueue(Element& element)
>+// https://html.spec.whatwg.org/multipage/custom-elements.html#enqueue-an-element-on-the-appropriate-element-queue
>+void CustomElementReactionQueue::enqueueElementOnAppropriateElementQueue(Element& element)
> {
>     ASSERT(element.reactionQueue());
>     if (!CustomElementReactionStack::s_currentProcessingStack) {
>         auto& queue = ensureBackupQueue();
>         queue.add(element);
>-        return *element.reactionQueue();
>+        return;
>     }
> 
>     auto*& queue = CustomElementReactionStack::s_currentProcessingStack->m_queue;
>     if (!queue) // We use a raw pointer to avoid genearing code to delete it in ~CustomElementReactionStack.
>         queue = new ElementQueue;
>     queue->add(element);
>-    return *element.reactionQueue();
> }
> 
> #if !ASSERT_DISABLED
>Index: Source/WebCore/dom/CustomElementReactionQueue.h
>===================================================================
>--- Source/WebCore/dom/CustomElementReactionQueue.h	(revision 239062)
>+++ Source/WebCore/dom/CustomElementReactionQueue.h	(working copy)
>@@ -77,7 +77,7 @@ public:
>     };
> 
> private:
>-    static CustomElementReactionQueue& ensureCurrentQueue(Element&);
>+    static void enqueueElementOnAppropriateElementQueue(Element&);
>     static ElementQueue& ensureBackupQueue();
>     static ElementQueue& backupElementQueue();
> 
>Index: LayoutTests/ChangeLog
>===================================================================
>--- LayoutTests/ChangeLog	(revision 239062)
>+++ LayoutTests/ChangeLog	(working copy)
>@@ -1,3 +1,16 @@
>+2018-12-10  Ryosuke Niwa  <rniwa@webkit.org>
>+
>+        connectedCallback is invoked during the removal of the element inside another element's connectedCallback
>+        https://bugs.webkit.org/show_bug.cgi?id=183586
>+        <rdar://problem/38403504>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Added a W3C style testharness test.
>+
>+        * fast/custom-elements/enqueue-custom-element-callback-reactions-inside-another-callback-expected.txt: Added.
>+        * fast/custom-elements/enqueue-custom-element-callback-reactions-inside-another-callback.html: Added.
>+
> 2018-12-10  Rob Buis  <rbuis@igalia.com>
> 
>         XMLHttpRequest removes spaces from content-types before processing
>Index: LayoutTests/fast/custom-elements/enqueue-custom-element-callback-reactions-inside-another-callback-expected.txt
>===================================================================
>--- LayoutTests/fast/custom-elements/enqueue-custom-element-callback-reactions-inside-another-callback-expected.txt	(nonexistent)
>+++ LayoutTests/fast/custom-elements/enqueue-custom-element-callback-reactions-inside-another-callback-expected.txt	(working copy)
>@@ -0,0 +1,4 @@
>+
>+PASS Disconnecting an element with disconnectedCallback while it has a connectedCallback in its custom element reaction queue must result in connectedCallback getting invoked before the removal completes 
>+PASS Disconnecting an element without disconnectedCallback while it has a connectedCallback in its custom element reaction queue must not result in connectedCallback getting invoked before the removal completes 
>+
>Index: LayoutTests/fast/custom-elements/enqueue-custom-element-callback-reactions-inside-another-callback.html
>===================================================================
>--- LayoutTests/fast/custom-elements/enqueue-custom-element-callback-reactions-inside-another-callback.html	(nonexistent)
>+++ LayoutTests/fast/custom-elements/enqueue-custom-element-callback-reactions-inside-another-callback.html	(working copy)
>@@ -0,0 +1,71 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<title>Custom Elements: must enqueue an element on the appropriate element queue after checking callback is null and the attribute name</title>
>+<meta name="author" title="Ryosuke Niwa" href="mailto:rniwa@webkit.org">
>+<meta name="assert" content="To enqueue a custom element callback reaction, the callback must be checked of being null and whether the attribute name is observed or not">
>+<link rel="help" content="https://html.spec.whatwg.org/multipage/custom-elements.html#enqueue-a-custom-element-callback-reaction">
>+<link rel="help" content="https://github.com/w3c/webcomponents/issues/760">
>+<script src="../../resources/testharness.js"></script>
>+<script src="../../resources/testharnessreport.js"></script>
>+<script src="../../imported/w3c/web-platform-tests/custom-elements/resources/custom-elements-helpers.js"></script>
>+</head>
>+<body>
>+<script>
>+
>+test_with_window((contentWindow, contentDocument) => {
>+    class ParentElement extends contentWindow.HTMLElement {
>+        connectedCallback()
>+        {
>+            logs.push('begin');
>+            const child = this.firstChild;
>+            child.remove();
>+            logs.push('end');
>+        }
>+    }
>+    contentWindow.customElements.define('parent-element', ParentElement);
>+
>+    const logs = [];
>+    class ChildElement extends contentWindow.HTMLElement {
>+        connectedCallback() { logs.push('connected'); }
>+        disconnectedCallback() { logs.push('disconnected'); }
>+    }
>+    contentWindow.customElements.define('child-element', ChildElement);
>+
>+    const parent = new ParentElement;
>+    const child = new ChildElement;
>+    parent.appendChild(child);
>+
>+    contentDocument.body.appendChild(parent);
>+    assert_array_equals(logs, ['begin', 'connected', 'disconnected', 'end']);
>+}, 'Disconnecting an element with disconnectedCallback while it has a connectedCallback in its custom element reaction queue must result in connectedCallback getting invoked before the removal completes');
>+
>+test_with_window((contentWindow, contentDocument) => {
>+    class ParentElement extends contentWindow.HTMLElement {
>+        connectedCallback()
>+        {
>+            logs.push('begin');
>+            const child = this.firstChild;
>+            child.remove();
>+            logs.push('end');
>+        }
>+    }
>+    contentWindow.customElements.define('parent-element', ParentElement);
>+
>+    const logs = [];
>+    class ChildElement extends contentWindow.HTMLElement {
>+        connectedCallback() { logs.push('connected'); }
>+    }
>+    contentWindow.customElements.define('child-element', ChildElement);
>+
>+    const parent = new ParentElement;
>+    const child = new ChildElement;
>+    parent.appendChild(child);
>+
>+    contentDocument.body.appendChild(parent);
>+    assert_array_equals(logs, ['begin', 'end', 'connected']);
>+}, 'Disconnecting an element without disconnectedCallback while it has a connectedCallback in its custom element reaction queue must not result in connectedCallback getting invoked before the removal completes');
>+
>+</script>
>+</body>
>+</html>

Flags:

fred.wang: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 183586: 335672 | 340977 | 340981 | 341004 | 341117 | 341135 | 341186 | 341211 | 341272 | 341278 | 345570 | 345834 | 346614 | 347145 | 357032