Web Inspector: Elements: event listener change events should only be fired for the...
authordrousso@apple.com <drousso@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 15 Apr 2019 17:26:39 +0000 (17:26 +0000)
committerdrousso@apple.com <drousso@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 15 Apr 2019 17:26:39 +0000 (17:26 +0000)
https://bugs.webkit.org/show_bug.cgi?id=196887
<rdar://problem/49870627>

Reviewed by Timothy Hatcher.

Source/WebCore:

Test: inspector/dom/event-listener-add-remove.html
      inspector/dom/event-listener-inspected-node.html

* inspector/agents/InspectorDOMAgent.h:
* inspector/agents/InspectorDOMAgent.cpp:
(WebCore::InspectorDOMAgent::getEventListenersForNode):
(WebCore::InspectorDOMAgent::setInspectedNode):
(WebCore::InspectorDOMAgent::didAddEventListener):
(WebCore::InspectorDOMAgent::willRemoveEventListener):

Source/WebInspectorUI:

* UserInterface/Models/DOMNode.js:
(WI.DOMNode.prototype.getEventListeners):

LayoutTests:

* inspector/dom/event-listener-inspected-node.html: Added.
* inspector/dom/event-listener-inspected-node-expected.txt: Added.

* inspector/dom/event-listener-add-remove.html:
* inspector/dom/event-listener-add-remove-expected.txt:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@244267 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/inspector/dom/event-listener-add-remove-expected.txt
LayoutTests/inspector/dom/event-listener-add-remove.html
LayoutTests/inspector/dom/event-listener-inspected-node-expected.txt [new file with mode: 0644]
LayoutTests/inspector/dom/event-listener-inspected-node.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/inspector/agents/InspectorDOMAgent.cpp
Source/WebCore/inspector/agents/InspectorDOMAgent.h
Source/WebInspectorUI/ChangeLog
Source/WebInspectorUI/UserInterface/Models/DOMNode.js

index 907e439..d39dc93 100644 (file)
@@ -1,3 +1,17 @@
+2019-04-15  Devin Rousso  <drousso@apple.com>
+
+        Web Inspector: Elements: event listener change events should only be fired for the selected node and it's ancestors
+        https://bugs.webkit.org/show_bug.cgi?id=196887
+        <rdar://problem/49870627>
+
+        Reviewed by Timothy Hatcher.
+
+        * inspector/dom/event-listener-inspected-node.html: Added.
+        * inspector/dom/event-listener-inspected-node-expected.txt: Added.
+
+        * inspector/dom/event-listener-add-remove.html:
+        * inspector/dom/event-listener-add-remove-expected.txt:
+
 2019-04-15  Shawn Roberts  <sroberts@apple.com>
 
         inspector/canvas/recording-webgl-snapshots.html is a flaky failure
index cc3707d..c66711b 100644 (file)
@@ -1,5 +1,6 @@
 Testing events for adding and removing event listeners.
 
+Changing inspected node to #document...
 
 == Running test suite: DOM.eventListeners
 -- Running test case: DOM.eventListeners.Initial
index 978619b..14e65dd 100644 (file)
@@ -209,15 +209,15 @@ function test() {
 
                         addAttribute("onload", 2);
                     });
-                } else if (eventCount === 2)
+                } else if (eventCount === 2) {
                     InspectorTest.log("Attribute event listener replaced.");
-                else if (eventCount === 3) {
+
                     logListeners(1).then(() => {
                         InspectorTest.log("");
 
                         removeAttribute("onload");
                     });
-                } else if (eventCount === 4)
+                } else if (eventCount === 3)
                     logListeners(0).then(resolve, reject);
                 else
                     reject("Event listeners changed too many times.");
@@ -227,9 +227,17 @@ function test() {
         }
     });
 
-    WI.domManager.requestDocument((documentNode) => {
+    WI.domManager.requestDocument(async (documentNode) => {
         node = documentNode;
 
+        InspectorTest.assert(node !== WI.domManager.inspectedNode);
+        InspectorTest.log("Changing inspected node to #document...");
+        await Promise.all([
+            WI.domManager.awaitEvent(WI.DOMManager.Event.InspectedNodeChanged),
+            WI.domManager.setInspectedNode(node),
+        ]);
+        InspectorTest.assert(node === WI.domManager.inspectedNode);
+
         suite.runTestCasesAndFinish();
     });
 }
diff --git a/LayoutTests/inspector/dom/event-listener-inspected-node-expected.txt b/LayoutTests/inspector/dom/event-listener-inspected-node-expected.txt
new file mode 100644 (file)
index 0000000..4b23bda
--- /dev/null
@@ -0,0 +1,56 @@
+Testing that event listener add/remove events are only fired for the inspected node and its ancestors.
+
+Changing inspected node to #child...
+
+== Running test suite: DOM.eventListeners
+-- Running test case: DOM.eventListeners.InspectedNode
+PASS: There should not be an event listener for #child.
+PASS: There should not be an event listener for #parent.
+Adding listener to #child...
+PASS: Adding an event listener to the inspected node should notify the frontend.
+PASS: There should be an event listener for #child.
+PASS: There should not be an event listener for #parent.
+Removing listener from #child...
+PASS: Removing an event listener from the inspected node should notify the frontend.
+PASS: There should not be an event listener for #child.
+PASS: There should not be an event listener for #parent.
+
+-- Running test case: DOM.eventListeners.InspectedNode.ancestor
+PASS: There should not be an event listener for #child.
+PASS: There should not be an event listener for #parent.
+Adding listener to #parent...
+PASS: Adding an event listener to an ancestor of the inspected node should notify the frontend.
+PASS: There should be an event listener for #child.
+PASS: There should be an event listener for #parent.
+Removing listener from #parent...
+PASS: Removing an event listener from an ancestor of the inspected node should notify the frontend.
+PASS: There should not be an event listener for #child.
+PASS: There should not be an event listener for #parent.
+
+-- Running test case: DOM.eventListeners.NotInspectedNode
+PASS: There should not be an event listener for #child.
+PASS: There should not be an event listener for #parent.
+Adding listener to #sibling...
+PASS: Adding an event listener to a node other than the inspected node should not notify the frontend.
+PASS: There should not be an event listener for #child.
+PASS: There should not be an event listener for #parent.
+Removing listener from #sibling...
+PASS: Removing an event listener from a node other than the inspected node should not notify the frontend.
+PASS: There should not be an event listener for #child.
+PASS: There should not be an event listener for #parent.
+
+-- Running test case: DOM.eventListeners.SubsequentBeforeGet
+PASS: There should not be an event listener for #child.
+PASS: There should not be an event listener for #parent.
+Adding listener to #child...
+PASS: Adding an event listener to the inspected node should notify the frontend.
+Adding listener to #parent...
+PASS: Adding an event listener to an ancestor of the inspected node should not notify the frontend if the frontend hasn't requested the new list since the last update.
+Requesting event listeners...
+PASS: There should be an event listener for #child.
+PASS: There should be an event listener for #parent.
+Removing listener from #child...
+PASS: Removing an event listener from the inspected node should notify the frontend.
+Removing listener from #parent...
+PASS: Removing an event listener from an ancestor of the inspected node should not notify the frontend if the frontend hasn't requested the new list since the last update.
+
diff --git a/LayoutTests/inspector/dom/event-listener-inspected-node.html b/LayoutTests/inspector/dom/event-listener-inspected-node.html
new file mode 100644 (file)
index 0000000..6e7426c
--- /dev/null
@@ -0,0 +1,177 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../../http/tests/inspector/resources/inspector-test.js"></script>
+<script>
+const callback = (event) => { };
+
+function addListener(selector) {
+    document.querySelector(selector).addEventListener("click", callback);
+    TestPage.dispatchEventToFrontend("addListener");
+}
+
+function removeListener(selector) {
+    document.querySelector(selector).removeEventListener("click", callback);
+    TestPage.dispatchEventToFrontend("removeListener");
+}
+
+function test() {
+    let parentNode = null;
+    let childNode = null;
+    let siblingNode = null;
+
+    async function addListener(node) {
+        let promise = new WI.WrappedPromise;
+        let listener = node.addEventListener(WI.DOMNode.Event.EventListenersChanged, (event) => {
+            if (!promise.settled)
+                promise.resolve(true);
+        });
+        InspectorTest.singleFireEventListener("addListener", (event) => {
+            node.removeEventListener(WI.DOMNode.Event.EventListenersChanged, listener);
+
+            if (!promise.settled)
+                promise.resolve(false);
+        });
+
+        let selector = node.escapedIdSelector;
+        InspectorTest.log(`Adding listener to ${selector}...`);
+        await InspectorTest.evaluateInPage(`addListener("${selector}")`);
+
+        return promise.promise;
+    }
+
+    async function removeListener(node) {
+        let promise = new WI.WrappedPromise;
+        let listener = node.addEventListener(WI.DOMNode.Event.EventListenersChanged, (event) => {
+            if (!promise.settled)
+                promise.resolve(true);
+        });
+        InspectorTest.singleFireEventListener("removeListener", (event) => {
+            node.removeEventListener(WI.DOMNode.Event.EventListenersChanged, listener);
+
+            if (!promise.settled)
+                promise.resolve(false);
+        });
+
+        let selector = node.escapedIdSelector;
+        InspectorTest.log(`Removing listener from ${selector}...`);
+        await InspectorTest.evaluateInPage(`removeListener("${selector}")`);
+
+        return promise.promise;
+    }
+
+    async function checkTestEventListener(node, expected) {
+        let {listeners} = await DOMAgent.getEventListenersForNode(node.id);
+        listeners = listeners.map((listener) => listener.type);
+
+        let selector = node.escapedIdSelector;
+        if (expected)
+            InspectorTest.expectThat(listeners.includes("click"), `There should be an event listener for ${selector}.`);
+        else
+            InspectorTest.expectFalse(listeners.includes("click"), `There should not be an event listener for ${selector}.`);
+    }
+
+    let suite = InspectorTest.createAsyncSuite("DOM.eventListeners");
+
+    suite.addTestCase({
+        name: "DOM.eventListeners.InspectedNode",
+        description: "Test that event listener add/remove events for the inspected node are sent to the frontend.",
+        async test() {
+            await checkTestEventListener(childNode, false);
+            await checkTestEventListener(parentNode, false);
+
+            InspectorTest.expectThat(await addListener(childNode), "Adding an event listener to the inspected node should notify the frontend.");
+            await checkTestEventListener(childNode, true);
+            await checkTestEventListener(parentNode, false);
+
+            InspectorTest.expectThat(await removeListener(childNode), "Removing an event listener from the inspected node should notify the frontend.");
+            await checkTestEventListener(childNode, false);
+            await checkTestEventListener(parentNode, false);
+        },
+    });
+
+    suite.addTestCase({
+        name: "DOM.eventListeners.InspectedNode.ancestor",
+        description: "Test that event listener add/remove events for ancestors of the inspected node are sent to the frontend.",
+        async test() {
+            await checkTestEventListener(childNode, false);
+            await checkTestEventListener(parentNode, false);
+
+            InspectorTest.expectThat(await addListener(parentNode), "Adding an event listener to an ancestor of the inspected node should notify the frontend.");
+            await checkTestEventListener(childNode, true);
+            await checkTestEventListener(parentNode, true);
+
+            InspectorTest.expectThat(await removeListener(parentNode), "Removing an event listener from an ancestor of the inspected node should notify the frontend.");
+            await checkTestEventListener(childNode, false);
+            await checkTestEventListener(parentNode, false);
+        },
+    });
+
+    suite.addTestCase({
+        name: "DOM.eventListeners.NotInspectedNode",
+        description: "Test that event listener add/remove events for nodes other than the inspected node are not sent to the frontend.",
+        async test() {
+            await checkTestEventListener(childNode, false);
+            await checkTestEventListener(parentNode, false);
+
+            InspectorTest.expectFalse(await addListener(siblingNode), "Adding an event listener to a node other than the inspected node should not notify the frontend.");
+            await checkTestEventListener(childNode, false);
+            await checkTestEventListener(parentNode, false);
+
+            InspectorTest.expectFalse(await removeListener(siblingNode), "Removing an event listener from a node other than the inspected node should not notify the frontend.");
+            await checkTestEventListener(childNode, false);
+            await checkTestEventListener(parentNode, false);
+        },
+    });
+
+    suite.addTestCase({
+        name: "DOM.eventListeners.SubsequentBeforeGet",
+        description: "Test that subsequent event listener add/remove events are not sent to the frontend if the frontend hasn't requested the new list since the last update.",
+        async test() {
+            await checkTestEventListener(childNode, false);
+            await checkTestEventListener(parentNode, false);
+
+            InspectorTest.expectThat(await addListener(childNode), "Adding an event listener to the inspected node should notify the frontend.");
+            InspectorTest.expectFalse(await addListener(parentNode), "Adding an event listener to an ancestor of the inspected node should not notify the frontend if the frontend hasn't requested the new list since the last update.");
+
+            InspectorTest.log("Requesting event listeners...");
+            await checkTestEventListener(childNode, true);
+            await checkTestEventListener(parentNode, true);
+
+            InspectorTest.expectThat(await removeListener(childNode), "Removing an event listener from the inspected node should notify the frontend.");
+            InspectorTest.expectFalse(await removeListener(parentNode), "Removing an event listener from an ancestor of the inspected node should not notify the frontend if the frontend hasn't requested the new list since the last update.");
+        },
+    });
+
+    WI.domManager.requestDocument(async (documentNode) => {
+        let [parentNodeId, childNodeId, siblingNodeId] = await Promise.all([
+            WI.domManager.querySelector(documentNode, "#parent"),
+            WI.domManager.querySelector(documentNode, "#child"),
+            WI.domManager.querySelector(documentNode, "#sibling"),
+        ]);
+
+        parentNode = WI.domManager.nodeForId(parentNodeId);
+        childNode = WI.domManager.nodeForId(childNodeId);
+        siblingNode = WI.domManager.nodeForId(siblingNodeId);
+
+        InspectorTest.assert(childNode !== WI.domManager.inspectedNode);
+        InspectorTest.log(`Changing inspected node to ${childNode.escapedIdSelector}...`);
+        await Promise.all([
+            WI.domManager.awaitEvent(WI.DOMManager.Event.InspectedNodeChanged),
+            WI.domManager.setInspectedNode(childNode),
+        ]);
+        InspectorTest.assert(childNode === WI.domManager.inspectedNode);
+
+        suite.runTestCasesAndFinish();
+    });
+}
+</script>
+</head>
+<body onload="runTest()">
+    <p>Testing that event listener add/remove events are only fired for the inspected node and its ancestors.</p>
+    <div id="parent">
+        <div id="child"></div>
+    </div>
+    <div id="sibling"></div>
+</body>
+</html>
index 64487eb..7dce4f6 100644 (file)
@@ -1,3 +1,21 @@
+2019-04-15  Devin Rousso  <drousso@apple.com>
+
+        Web Inspector: Elements: event listener change events should only be fired for the selected node and it's ancestors
+        https://bugs.webkit.org/show_bug.cgi?id=196887
+        <rdar://problem/49870627>
+
+        Reviewed by Timothy Hatcher.
+
+        Test: inspector/dom/event-listener-add-remove.html
+              inspector/dom/event-listener-inspected-node.html
+
+        * inspector/agents/InspectorDOMAgent.h:
+        * inspector/agents/InspectorDOMAgent.cpp:
+        (WebCore::InspectorDOMAgent::getEventListenersForNode):
+        (WebCore::InspectorDOMAgent::setInspectedNode):
+        (WebCore::InspectorDOMAgent::didAddEventListener):
+        (WebCore::InspectorDOMAgent::willRemoveEventListener):
+
 2019-04-15  Antoine Quint  <graouts@apple.com>
 
         Ensure iOS layout traits are used for media controls in modern compatibility mode
index 6863443..27d960f 100644 (file)
@@ -952,6 +952,9 @@ void InspectorDOMAgent::getEventListenersForNode(ErrorString& errorString, int n
                 addListener(*listener, info);
         }
     }
+
+    if (m_inspectedNode == node)
+        m_suppressEventListenerChangedEvent = false;
 }
 
 void InspectorDOMAgent::setEventListenerDisabled(ErrorString& errorString, int eventListenerId, bool disabled)
@@ -1398,8 +1401,12 @@ void InspectorDOMAgent::setInspectedNode(ErrorString& errorString, int nodeId)
         return;
     }
 
+    m_inspectedNode = node;
+
     if (CommandLineAPIHost* commandLineAPIHost = static_cast<WebInjectedScriptManager&>(m_injectedScriptManager).commandLineAPIHost())
         commandLineAPIHost->addInspectedObject(std::make_unique<InspectableNode>(node));
+
+    m_suppressEventListenerChangedEvent = false;
 }
 
 void InspectorDOMAgent::resolveNode(ErrorString& errorString, int nodeId, const String* objectGroup, RefPtr<Inspector::Protocol::Runtime::RemoteObject>& result)
@@ -2388,10 +2395,19 @@ void InspectorDOMAgent::didAddEventListener(EventTarget& target)
     if (!is<Node>(target))
         return;
 
-    int nodeId = boundNodeId(&downcast<Node>(target));
+    auto& node = downcast<Node>(target);
+    if (!node.contains(m_inspectedNode.get()))
+        return;
+
+    int nodeId = boundNodeId(&node);
     if (!nodeId)
         return;
 
+    if (m_suppressEventListenerChangedEvent)
+        return;
+
+    m_suppressEventListenerChangedEvent = true;
+
     m_frontendDispatcher->didAddEventListener(nodeId);
 }
 
@@ -2399,7 +2415,10 @@ void InspectorDOMAgent::willRemoveEventListener(EventTarget& target, const Atomi
 {
     if (!is<Node>(target))
         return;
+
     auto& node = downcast<Node>(target);
+    if (!node.contains(m_inspectedNode.get()))
+        return;
 
     int nodeId = boundNodeId(&node);
     if (!nodeId)
@@ -2420,6 +2439,11 @@ void InspectorDOMAgent::willRemoveEventListener(EventTarget& target, const Atomi
         return entry.value.matches(target, eventType, listener, capture);
     });
 
+    if (m_suppressEventListenerChangedEvent)
+        return;
+
+    m_suppressEventListenerChangedEvent = true;
+
     m_frontendDispatcher->willRemoveEventListener(nodeId);
 }
 
index 4a58b0f..6a244c5 100644 (file)
@@ -251,11 +251,13 @@ private:
     std::unique_ptr<RevalidateStyleAttributeTask> m_revalidateStyleAttrTask;
     RefPtr<Node> m_nodeToFocus;
     RefPtr<Node> m_mousedOverNode;
+    RefPtr<Node> m_inspectedNode;
     std::unique_ptr<HighlightConfig> m_inspectModeHighlightConfig;
     std::unique_ptr<InspectorHistory> m_history;
     std::unique_ptr<DOMEditor> m_domEditor;
     bool m_searchingForNode { false };
     bool m_suppressAttributeModifiedEvent { false };
+    bool m_suppressEventListenerChangedEvent { false };
     bool m_documentRequested { false };
 
 #if ENABLE(VIDEO)
index 9c4a76b..a264098 100644 (file)
@@ -1,5 +1,16 @@
 2019-04-15  Devin Rousso  <drousso@apple.com>
 
+        Web Inspector: Elements: event listener change events should only be fired for the selected node and it's ancestors
+        https://bugs.webkit.org/show_bug.cgi?id=196887
+        <rdar://problem/49870627>
+
+        Reviewed by Timothy Hatcher.
+
+        * UserInterface/Models/DOMNode.js:
+        (WI.DOMNode.prototype.getEventListeners):
+
+2019-04-15  Devin Rousso  <drousso@apple.com>
+
         Web Inspector: replace all uses of `this` with `WI` in Main.js/Test.js
         https://bugs.webkit.org/show_bug.cgi?id=196795
         <rdar://problem/49796618>
index 555c398..3981841 100644 (file)
@@ -587,6 +587,7 @@ WI.DOMNode = class DOMNode extends WI.Object
 
     getEventListeners(callback)
     {
+        console.assert(WI.domManager.inspectedNode === this);
         DOMAgent.getEventListenersForNode(this.id, callback);
     }