Attachment 349994 Details for Bug 180378 – Patch for landing

[patch] Patch for landing

bug-180378-20180917204157.patch (text/plain), 17.53 KB, created by Ryosuke Niwa on 2018-09-17 20:41:57 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Ryosuke Niwa

Created: 2018-09-17 20:41:57 PDT

Size: 17.53 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 236099)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,68 @@
>+2018-09-12  Ryosuke Niwa  <rniwa@webkit.org>
>+
>+        Update composedPath to match the latest spec
>+        https://bugs.webkit.org/show_bug.cgi?id=180378
>+        <rdar://problem/42843004>
>+
>+        Reviewed by Darin Adler.
>+
>+        This patch makes the check for whether a given node in the event path be included in composedPath
>+        pre-determined at the time of the event dispatching per https://github.com/whatwg/dom/issues/525.
>+        This was a fix for the issue that if an event listener in a closed shadow tree removes a node in the
>+        same tree in the event path, then composedPath called on its shadow host, for example, will include
>+        the removed node since it's no longer in the closed shadow tree.
>+
>+        Naively, implementing this behavior would require remembering the original document or shadow root
>+        of each node in the event path as well as its parent shadow root, or worse which node is disclosed
>+        to every other node at the time of computing the event path.
>+
>+        This patch takes a more novel and efficient approach to implement the new behavior by adding a single
>+        integer indicating the number of closed-mode shadow root ancestors of each node in the event path.
>+        In computePathUnclosedToTarget, any node whose *depth* is greater than the context object is excluded.
>+
>+        Consider the following example:
>+        div ------- ShadowRoot (closed)
>+          +- span     +- slot
>+        If an event is dispatched on span, then the event path would be [span, slot, ShadowRoot, div]. Then
>+        the values of integers assigned to each node would be: [0, 1, 1, 0] respectively. When composedPath
>+        is called on span or div, slot and ShadowRoot are excluded because they have a greater *depth* value.
>+
>+        Unfortunately, this simplistic solution doesn't work when there are multiple shadow roots of the same
>+        depth through which an event is dispatched as in:
>+        section -- ShadowRoot (closed, SR2)
>+          |          +- slot (s2)
>+          +div ------ ShadowRoot (closed, SR1)
>+            +- span     +- slot (s1)
>+        If an event is dispatched on span, the event path would be [span, s1, SR1, div, s2, SR2, section].
>+        The values of integers assigned are: [0, 1, 1, 0, 1, 1, 0] respectively. When composedPath is called
>+        on SR1, the simplistic approach would include s2 and SR2, which would be wrong.
>+
>+        To account for this case, in computePathUnclosedToTarget, we traverse the event path upwards (i.e.
>+        ancestors) and downwards (i.e. descendants) from the context object and decrease the *allowed depth*
>+        of shadow trees when we traverse out of a shadow tree in either direction. When traversing upwards,
>+        therefore, moving out of a shadow root to its host would would decrease the allowed depth. When
>+        traversing dowards, moving from a slot element to its assigned node would decrease the allowed depth.
>+
>+        Note that the depths can be negative when a composed event is dispatched inside a closed shadow tree,
>+        and it gets out of its shadow host.
>+
>+        Unfortunately, the latest DOM specification has a bug and doesn't match the behavior of Chrome. This
>+        patch proposes a new algorithm which can be adopted in https://github.com/whatwg/dom/issues/684.
>+
>+        Test: imported/w3c/web-platform-tests/shadow-dom/event-composed-path-after-dom-mutation.html
>+
>+        * dom/EventContext.cpp:
>+        (WebCore::EventContext::EventContext):
>+        (WebCore::MouseOrFocusEventContext::MouseOrFocusEventContext):
>+        (WebCore::TouchEventContext::TouchEventContext):
>+        * dom/EventContext.h:
>+        (WebCore::EventContext::closedShadowDepth const): Added.
>+        * dom/EventPath.cpp:
>+        (WebCore::WindowEventContext::WindowEventContext):
>+        (WebCore::EventPath::buildPath): Compute the closed shadow tree's depths for each node in the path.
>+        (WebCore::computePathUnclosedToTarget const): Implemented the aforementioned algorithm.
>+        (WebCore::EventPath::EventPath):
>+
> 2018-09-17  Jer Noble  <jer.noble@apple.com>
> 
>         Add support for HEVC codec types in Media Capabilities
>Index: Source/WebCore/dom/EventContext.cpp
>===================================================================
>--- Source/WebCore/dom/EventContext.cpp	(revision 236099)
>+++ Source/WebCore/dom/EventContext.cpp	(working copy)
>@@ -35,10 +35,11 @@
> 
> namespace WebCore {
> 
>-EventContext::EventContext(Node* node, EventTarget* currentTarget, EventTarget* target)
>-    : m_node(node)
>-    , m_currentTarget(currentTarget)
>-    , m_target(target)
>+EventContext::EventContext(Node* node, EventTarget* currentTarget, EventTarget* target, int closedShadowDepth)
>+    : m_node { node }
>+    , m_currentTarget { currentTarget }
>+    , m_target { target }
>+    , m_closedShadowDepth { closedShadowDepth }
> {
>     ASSERT(!isUnreachableNode(m_target.get()));
> }
>@@ -66,8 +67,8 @@ bool EventContext::isTouchEventContext()
>     return false;
> }
> 
>-MouseOrFocusEventContext::MouseOrFocusEventContext(Node& node, EventTarget* currentTarget, EventTarget* target)
>-    : EventContext(&node, currentTarget, target)
>+MouseOrFocusEventContext::MouseOrFocusEventContext(Node& node, EventTarget* currentTarget, EventTarget* target, int closedShadowDepth)
>+    : EventContext(&node, currentTarget, target, closedShadowDepth)
> {
> }
> 
>@@ -87,8 +88,8 @@ bool MouseOrFocusEventContext::isMouseOr
> 
> #if ENABLE(TOUCH_EVENTS)
> 
>-TouchEventContext::TouchEventContext(Node& node, EventTarget* currentTarget, EventTarget* target)
>-    : EventContext(&node, currentTarget, target)
>+TouchEventContext::TouchEventContext(Node& node, EventTarget* currentTarget, EventTarget* target, int closedShadowDepth)
>+    : EventContext(&node, currentTarget, target, closedShadowDepth)
>     , m_touches(TouchList::create())
>     , m_targetTouches(TouchList::create())
>     , m_changedTouches(TouchList::create())
>Index: Source/WebCore/dom/EventContext.h
>===================================================================
>--- Source/WebCore/dom/EventContext.h	(revision 236099)
>+++ Source/WebCore/dom/EventContext.h	(working copy)
>@@ -38,12 +38,13 @@ class EventContext {
> public:
>     using EventInvokePhase = EventTarget::EventInvokePhase;
> 
>-    EventContext(Node*, EventTarget* currentTarget, EventTarget*);
>+    EventContext(Node*, EventTarget* currentTarget, EventTarget*, int closedShadowDepth);
>     virtual ~EventContext();
> 
>     Node* node() const { return m_node.get(); }
>     EventTarget* currentTarget() const { return m_currentTarget.get(); }
>     EventTarget* target() const { return m_target.get(); }
>+    int closedShadowDepth() const { return m_closedShadowDepth; }
> 
>     virtual void handleLocalEvents(Event&, EventInvokePhase) const;
> 
>@@ -58,11 +59,12 @@ protected:
>     RefPtr<Node> m_node;
>     RefPtr<EventTarget> m_currentTarget;
>     RefPtr<EventTarget> m_target;
>+    int m_closedShadowDepth { 0 };
> };
> 
> class MouseOrFocusEventContext final : public EventContext {
> public:
>-    MouseOrFocusEventContext(Node&, EventTarget* currentTarget, EventTarget*);
>+    MouseOrFocusEventContext(Node&, EventTarget* currentTarget, EventTarget*, int closedShadowDepth);
>     virtual ~MouseOrFocusEventContext();
> 
>     Node* relatedTarget() const { return m_relatedTarget.get(); }
>@@ -79,7 +81,7 @@ private:
> 
> class TouchEventContext final : public EventContext {
> public:
>-    TouchEventContext(Node&, EventTarget* currentTarget, EventTarget*);
>+    TouchEventContext(Node&, EventTarget* currentTarget, EventTarget*, int closedShadowDepth);
>     virtual ~TouchEventContext();
> 
>     enum TouchListType { Touches, TargetTouches, ChangedTouches };
>Index: Source/WebCore/dom/EventPath.cpp
>===================================================================
>--- Source/WebCore/dom/EventPath.cpp	(revision 236099)
>+++ Source/WebCore/dom/EventPath.cpp	(working copy)
>@@ -35,13 +35,13 @@ namespace WebCore {
> 
> class WindowEventContext final : public EventContext {
> public:
>-    WindowEventContext(Node&, DOMWindow&, EventTarget&);
>+    WindowEventContext(Node&, DOMWindow&, EventTarget&, int closedShadowDepth);
> private:
>     void handleLocalEvents(Event&, EventInvokePhase) const final;
> };
> 
>-inline WindowEventContext::WindowEventContext(Node& node, DOMWindow& currentTarget, EventTarget& target)
>-    : EventContext(&node, &currentTarget, &target)
>+inline WindowEventContext::WindowEventContext(Node& node, DOMWindow& currentTarget, EventTarget& target, int closedShadowDepth)
>+    : EventContext(&node, &currentTarget, &target, closedShadowDepth)
> {
> }
> 
>@@ -111,28 +111,31 @@ EventPath::EventPath(Node& originalTarge
> 
> void EventPath::buildPath(Node& originalTarget, Event& event)
> {
>-    using MakeEventContext = std::unique_ptr<EventContext> (*)(Node&, EventTarget*, EventTarget*);
>-    MakeEventContext makeEventContext = [] (Node& node, EventTarget* currentTarget, EventTarget* target) {
>-        return std::make_unique<EventContext>(&node, currentTarget, target);
>+    using MakeEventContext = std::unique_ptr<EventContext> (*)(Node&, EventTarget*, EventTarget*, int closedShadowDepth);
>+    MakeEventContext makeEventContext = [] (Node& node, EventTarget* currentTarget, EventTarget* target, int closedShadowDepth) {
>+        return std::make_unique<EventContext>(&node, currentTarget, target, closedShadowDepth);
>     };
>     if (is<MouseEvent>(event) || event.isFocusEvent()) {
>-        makeEventContext = [] (Node& node, EventTarget* currentTarget, EventTarget* target) -> std::unique_ptr<EventContext> {
>-            return std::make_unique<MouseOrFocusEventContext>(node, currentTarget, target);
>+        makeEventContext = [] (Node& node, EventTarget* currentTarget, EventTarget* target, int closedShadowDepth) -> std::unique_ptr<EventContext> {
>+            return std::make_unique<MouseOrFocusEventContext>(node, currentTarget, target, closedShadowDepth);
>         };
>     }
> #if ENABLE(TOUCH_EVENTS)
>     if (is<TouchEvent>(event)) {
>-        makeEventContext = [] (Node& node, EventTarget* currentTarget, EventTarget* target) -> std::unique_ptr<EventContext> {
>-            return std::make_unique<TouchEventContext>(node, currentTarget, target);
>+        makeEventContext = [] (Node& node, EventTarget* currentTarget, EventTarget* target, int closedShadowDepth) -> std::unique_ptr<EventContext> {
>+            return std::make_unique<TouchEventContext>(node, currentTarget, target, closedShadowDepth);
>         };
>     }
> #endif
> 
>     Node* node = nodeOrHostIfPseudoElement(&originalTarget);
>     Node* target = node ? eventTargetRespectingTargetRules(*node) : nullptr;
>+    int closedShadowDepth = 0;
>+    // Depths are used to decided which nodes are excluded in event.composedPath when the tree is mutated during event dispatching.
>+    // They could be negative for nodes outside the shadow tree of the target node.
>     while (node) {
>         while (node) {
>-            m_path.append(makeEventContext(*node, eventTargetRespectingTargetRules(*node), target));
>+            m_path.append(makeEventContext(*node, eventTargetRespectingTargetRules(*node), target, closedShadowDepth));
> 
>             if (is<ShadowRoot>(*node))
>                 break;
>@@ -144,7 +147,7 @@ void EventPath::buildPath(Node& original
>                     ASSERT(target);
>                     if (target) {
>                         if (auto* window = downcast<Document>(*node).domWindow())
>-                            m_path.append(std::make_unique<WindowEventContext>(*node, *window, *target));
>+                            m_path.append(std::make_unique<WindowEventContext>(*node, *window, *target, closedShadowDepth));
>                     }
>                 }
>                 return;
>@@ -153,6 +156,8 @@ void EventPath::buildPath(Node& original
>             auto* shadowRootOfParent = parent->shadowRoot();
>             if (UNLIKELY(shadowRootOfParent)) {
>                 if (auto* assignedSlot = shadowRootOfParent->findAssignedSlot(*node)) {
>+                    if (shadowRootOfParent->mode() != ShadowRootMode::Open)
>+                        closedShadowDepth++;
>                     // node is assigned to a slot. Continue dispatching the event at this slot.
>                     parent = assignedSlot;
>                 }
>@@ -165,6 +170,8 @@ void EventPath::buildPath(Node& original
>         if (!shouldEventCrossShadowBoundary(event, shadowRoot, originalTarget))
>             return;
>         node = shadowRoot.host();
>+        if (shadowRoot.mode() != ShadowRootMode::Open)
>+            closedShadowDepth--;
>         if (exitingShadowTreeOfTarget)
>             target = eventTargetRespectingTargetRules(*node);
>     }
>@@ -251,32 +258,55 @@ void EventPath::retargetTouchLists(const
> #endif
> 
> // https://dom.spec.whatwg.org/#dom-event-composedpath
>+// Any node whose depth computed in EventPath::buildPath is greater than the context object is excluded.
>+// Because we can exit out of a closed shadow tree and re-enter another closed shadow tree via a slot,
>+// we decrease the *allowed depth* whenever we moved to a "shallower" (closer-to-document) tree.
> Vector<EventTarget*> EventPath::computePathUnclosedToTarget(const EventTarget& target) const
> {
>     Vector<EventTarget*> path;
>-    path.reserveInitialCapacity(m_path.size());
>-    const Node* targetNode = nullptr;
>-    if (is<Node>(target))
>-        targetNode = &downcast<Node>(target);
>-    else if (is<DOMWindow>(target)) {
>-        targetNode = downcast<DOMWindow>(target).document();
>-        ASSERT(targetNode);
>-    }
>-    for (auto& context : m_path) {
>-        auto* currentTarget = context->currentTarget();
>-        if (!is<Node>(currentTarget) || !targetNode || !targetNode->isClosedShadowHidden(downcast<Node>(*currentTarget)))
>-            path.uncheckedAppend(currentTarget);
>-    }
>+    auto pathSize = m_path.size();
>+    RELEASE_ASSERT(pathSize);
>+    path.reserveInitialCapacity(pathSize);
>+
>+    auto currentTargetIndex = m_path.findMatching([&target] (auto& context) {
>+        return context->currentTarget() == &target;
>+    });
>+    RELEASE_ASSERT(currentTargetIndex != notFound);
>+    auto currentTargetDepth = m_path[currentTargetIndex]->closedShadowDepth();
>+
>+    auto appendTargetWithLesserDepth = [&path] (const EventContext& currentContext, int& currentDepthAllowed) {
>+        auto depth = currentContext.closedShadowDepth();
>+        bool contextIsInsideInnerShadowTree = depth > currentDepthAllowed;
>+        if (contextIsInsideInnerShadowTree)
>+            return;
>+        bool movedOutOfShadowTree = depth < currentDepthAllowed;
>+        if (movedOutOfShadowTree)
>+            currentDepthAllowed = depth;
>+        path.uncheckedAppend(currentContext.currentTarget());
>+    };
>+
>+    auto currentDepthAllowed = currentTargetDepth;
>+    auto i = currentTargetIndex;
>+    do {
>+        appendTargetWithLesserDepth(*m_path[i], currentDepthAllowed);
>+    } while (i--);
>+    path.reverse();
>+
>+    currentDepthAllowed = currentTargetDepth;
>+    for (auto i = currentTargetIndex + 1; i < pathSize; ++i)
>+        appendTargetWithLesserDepth(*m_path[i], currentDepthAllowed);
>+    
>     return path;
> }
> 
> EventPath::EventPath(const Vector<Element*>& targets)
> {
>+    // FIXME: This function seems wrong. Why are we not firing events in the closed shadow trees?
>     for (auto* target : targets) {
>         ASSERT(target);
>         Node* origin = *targets.begin();
>         if (!target->isClosedShadowHidden(*origin))
>-            m_path.append(std::make_unique<EventContext>(target, target, origin));
>+            m_path.append(std::make_unique<EventContext>(target, target, origin, 0));
>     }
> }
> 
>@@ -285,7 +315,7 @@ EventPath::EventPath(const Vector<EventT
>     for (auto* target : targets) {
>         ASSERT(target);
>         ASSERT(!is<Node>(target));
>-        m_path.append(std::make_unique<EventContext>(nullptr, target, *targets.begin()));
>+        m_path.append(std::make_unique<EventContext>(nullptr, target, *targets.begin(), 0));
>     }
> }
> 
>Index: LayoutTests/imported/w3c/ChangeLog
>===================================================================
>--- LayoutTests/imported/w3c/ChangeLog	(revision 236099)
>+++ LayoutTests/imported/w3c/ChangeLog	(working copy)
>@@ -1,3 +1,15 @@
>+2018-09-12  Ryosuke Niwa  <rniwa@webkit.org>
>+
>+        Update composedPath to match the latest spec
>+        https://bugs.webkit.org/show_bug.cgi?id=180378
>+        <rdar://problem/42843004>
>+
>+        Reviewed by Darin Adler.
>+
>+        Rebaselined the test now that all test cases pass.
>+
>+        * web-platform-tests/shadow-dom/event-composed-path-after-dom-mutation-expected.txt:
>+
> 2018-09-15  Rob Buis  <rbuis@igalia.com>
> 
>         XMLHttpRequest::createResponseBlob() should create a Blob with type for empty response
>Index: LayoutTests/imported/w3c/web-platform-tests/shadow-dom/event-composed-path-after-dom-mutation-expected.txt
>===================================================================
>--- LayoutTests/imported/w3c/web-platform-tests/shadow-dom/event-composed-path-after-dom-mutation-expected.txt	(revision 236099)
>+++ LayoutTests/imported/w3c/web-platform-tests/shadow-dom/event-composed-path-after-dom-mutation-expected.txt	(working copy)
>@@ -1,4 +1,4 @@
> 
>-FAIL Event.composedPath() should return the same result even if DOM is mutated (1/2) assert_array_equals: lengths differ, expected 3 got 2
>-FAIL Event.composedPath() should return the same result even if DOM is mutated (2/2) assert_array_equals: lengths differ, expected 5 got 2
>+PASS Event.composedPath() should return the same result even if DOM is mutated (1/2) 
>+PASS Event.composedPath() should return the same result even if DOM is mutated (2/2) 
>

Actions: View | Formatted Diff | Diff

Attachments on bug 180378: 349272 | 349533 | 349534 | 349994