Bug 177451 - Web Inspector: provide a way to enable/disable event listeners
Summary: Web Inspector: provide a way to enable/disable event listeners
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 178476
Blocks:
  Show dependency treegraph
 
Reported: 2017-09-25 12:09 PDT by Devin Rousso
Modified: 2017-10-25 21:08 PDT (History)
15 users (show)

See Also:


Attachments
Patch (30.36 KB, patch)
2017-09-25 12:43 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (65.89 KB, image/png)
2017-09-25 12:43 PDT, Devin Rousso
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.16 MB, application/zip)
2017-09-25 13:47 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-elcapitan (1003.63 KB, application/zip)
2017-09-25 13:50 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-elcapitan (1.76 MB, application/zip)
2017-09-25 14:13 PDT, Build Bot
no flags Details
Patch (35.84 KB, patch)
2017-09-25 22:26 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (20.58 KB, image/png)
2017-09-25 22:27 PDT, Devin Rousso
no flags Details
Patch (35.98 KB, patch)
2017-10-13 17:06 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (482.34 KB, image/png)
2017-10-13 17:07 PDT, Devin Rousso
no flags Details
Patch (36.98 KB, patch)
2017-10-14 02:46 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[PATCH] For Bots (36.15 KB, patch)
2017-10-25 17:56 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2017-09-25 12:09:06 PDT
I often find that if I am testing the implementation of an event listener in the console, I need to refresh the page after each test as otherwise I have to deal with multiple event listeners firing for the same event.  It would be nice to have a way to remove event listeners from within the UI.
Comment 1 Brian Burg 2017-09-25 12:19:23 PDT
It should be doable in the event listeners sidebar, right?
Comment 2 Devin Rousso 2017-09-25 12:43:15 PDT
Created attachment 321721 [details]
Patch
Comment 3 Devin Rousso 2017-09-25 12:43:37 PDT
Created attachment 321722 [details]
[Image] After Patch is applied
Comment 4 Joseph Pecoraro 2017-09-25 13:26:41 PDT
I think we can improve on this. An issue with removing an event listener is that it is hard to add it back. In that case being able enable/disable an Event Listener would be even more useful than remove. Maybe we would keep remove, but I think enable/disable would be far superior.

---

While we are on the subject of EventListeners, there are some improvements we talked about on IRC. The ability to link to the Function or Log the Function to the console (while this is somewhat less useful since you can't really modify it).

The current `DOM.EventListener` type is:

    {
        "id": "EventListener",
        "type": "object",
        "description": "A structure holding event listener properties.",
        "properties": [
            { "name": "type", "type": "string", "description": "<code>EventListener</code>'s type." },
            { "name": "useCapture", "type": "boolean", "description": "<code>EventListener</code>'s useCapture." },
            { "name": "isAttribute", "type": "boolean", "description": "<code>EventListener</code>'s isAttribute." },
            { "name": "nodeId", "$ref": "NodeId", "description": "Target <code>DOMNode</code> id." },
            { "name": "handlerBody", "type": "string", "description": "Event handler function body." },
            { "name": "location", "$ref": "Debugger.Location", "optional": true, "description": "Handler code location." },
            { "name": "sourceName", "type": "string", "optional": true, "description": "Source script URL." },
            { "name": "handler", "$ref": "Runtime.RemoteObject", "optional": true, "description": "Event handler function value." },
            { "name": "passive", "type": "boolean", "optional": true, "description": "<code>EventListener</code>'s passive." },
            { "name": "once", "type": "boolean", "optional": true, "description": "<code>EventListener</code>'s once." }
        ]
    },

Given that `handler` is a `Runtime.RemoteObject` we should have enough to get all of that data.

In fact, we had a discussion about this a couple years ago:
https://bugs.webkit.org/show_bug.cgi?id=146886

I stand by my comments in:
https://bugs.webkit.org/show_bug.cgi?id=146886#c3

It would be trivial to implement that, but lets continue that discussion over there where we have more context.
Comment 5 Joseph Pecoraro 2017-09-25 13:30:45 PDT
Comment on attachment 321721 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321721&action=review

> Source/JavaScriptCore/inspector/protocol/DOM.json:18
> +            "id": "EventListenerId",
> +            "type": "string",

Could we make this just an integer? That is far cheaper in most circumstances.

One thing to be aware of, do not use 0 as a key in a HashMap<>. 0 is one of the special values (deletedKey() or emptyKey()) and you would see ASSERTs in a debug build. Instead start off the eventListenerId counter at 1. Basically, match NodeId's counter (m_lastNodeId).
Comment 6 Build Bot 2017-09-25 13:47:28 PDT
Comment on attachment 321721 [details]
Patch

Attachment 321721 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4652948

New failing tests:
inspector/dom/event-listener-add-remove.html
Comment 7 Build Bot 2017-09-25 13:47:29 PDT
Created attachment 321733 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 8 Build Bot 2017-09-25 13:50:47 PDT
Comment on attachment 321721 [details]
Patch

Attachment 321721 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4653035

New failing tests:
inspector/dom/event-listener-add-remove.html
Comment 9 Build Bot 2017-09-25 13:50:48 PDT
Created attachment 321735 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 10 Build Bot 2017-09-25 14:13:22 PDT
Comment on attachment 321721 [details]
Patch

Attachment 321721 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4653049

New failing tests:
inspector/dom/event-listener-add-remove.html
Comment 11 Build Bot 2017-09-25 14:13:24 PDT
Created attachment 321738 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 12 Devin Rousso 2017-09-25 18:01:59 PDT
(In reply to Joseph Pecoraro from comment #4)
> I think we can improve on this. An issue with removing an event listener is
> that it is hard to add it back. In that case being able enable/disable an
> Event Listener would be even more useful than remove. Maybe we would keep
> remove, but I think enable/disable would be far superior.
I agree.  I'll rework this to provide the ability to enable/disable.

> ---
> 
> While we are on the subject of EventListeners, there are some improvements
> we talked about on IRC. The ability to link to the Function or Log the
> Function to the console (while this is somewhat less useful since you can't
> really modify it).
> 
> The current `DOM.EventListener` type is:
> 
>     {
>         "id": "EventListener",
>         "type": "object",
>         "description": "A structure holding event listener properties.",
>         "properties": [
>             { "name": "type", "type": "string", "description":
> "<code>EventListener</code>'s type." },
>             { "name": "useCapture", "type": "boolean", "description":
> "<code>EventListener</code>'s useCapture." },
>             { "name": "isAttribute", "type": "boolean", "description":
> "<code>EventListener</code>'s isAttribute." },
>             { "name": "nodeId", "$ref": "NodeId", "description": "Target
> <code>DOMNode</code> id." },
>             { "name": "handlerBody", "type": "string", "description": "Event
> handler function body." },
>             { "name": "location", "$ref": "Debugger.Location", "optional":
> true, "description": "Handler code location." },
>             { "name": "sourceName", "type": "string", "optional": true,
> "description": "Source script URL." },
>             { "name": "handler", "$ref": "Runtime.RemoteObject", "optional":
> true, "description": "Event handler function value." },
>             { "name": "passive", "type": "boolean", "optional": true,
> "description": "<code>EventListener</code>'s passive." },
>             { "name": "once", "type": "boolean", "optional": true,
> "description": "<code>EventListener</code>'s once." }
>         ]
>     },
> 
> Given that `handler` is a `Runtime.RemoteObject` we should have enough to
> get all of that data.
> 
> In fact, we had a discussion about this a couple years ago:
> https://bugs.webkit.org/show_bug.cgi?id=146886
> 
> I stand by my comments in:
> https://bugs.webkit.org/show_bug.cgi?id=146886#c3
> 
> It would be trivial to implement that, but lets continue that discussion
> over there where we have more context.
Agreed.
Comment 13 Devin Rousso 2017-09-25 22:26:43 PDT
Created attachment 321793 [details]
Patch
Comment 14 Devin Rousso 2017-09-25 22:27:34 PDT
Created attachment 321794 [details]
[Image] After Patch is applied

Similar to Shader Program enable/disable, the checkbox is always visible if the event listener is disabled.  If it is enabled, however, it is only visible when hovering the event listener's section.
Comment 15 Joseph Pecoraro 2017-10-13 16:21:27 PDT
(In reply to Devin Rousso from comment #14)
> Created attachment 321794 [details]
> [Image] After Patch is applied
> 
> Similar to Shader Program enable/disable, the checkbox is always visible if
> the event listener is disabled.  If it is enabled, however, it is only
> visible when hovering the event listener's section.

As mentioned in person, always visible all the time would probably be best. The checkbox is very small.

How does this handle overlap with a long event name? For example:

    webkitplaybacktargetavailabilitychanged

I would expect the event name to truncate like "webkitplaybacktargetavailab...".
Comment 16 Devin Rousso 2017-10-13 17:06:16 PDT
Created attachment 323777 [details]
Patch
Comment 17 Devin Rousso 2017-10-13 17:07:19 PDT
Created attachment 323778 [details]
[Image] After Patch is applied
Comment 18 Joseph Pecoraro 2017-10-13 18:03:58 PDT
Comment on attachment 323777 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323777&action=review

r=me

> Source/JavaScriptCore/ChangeLog:9
> +        Add `setEventListenerDisabled` command that sets the `disabled` flag on the given event

Instead of "that sets the `disabled` flag" is obscure. There is no public disabled flag. This should be a higher level description like "that disables or enables a specific event listener during event dispatch".

> Source/WebCore/inspector/InspectorDOMAgent.h:281
> +        EventTarget* eventTarget { nullptr };

Having just attended the WebKit Contributors meeting we may want to make this a RefPtr and avoid having a raw pointer.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:760
> +        if (event.target == this.domNode || event.target.isAncestor(this.domNode))

Style: ===

> Source/WebInspectorUI/UserInterface/Views/EventListenerSectionGroup.js:55
> +        rows.push(this._createDisabledToggleRow());

This should feature check support.

    if (DOMAgent.setEventListenerDisabled)
        rows.push(this._createDisabledToggleRow());

> LayoutTests/inspector/dom/setEventListenerDisabled.html:39
> +                    setTimeout(() => {
> +                        InspectorTest.pass("Click event listener did not fire.");
> +
> +                        InspectorTest.removeEventListener(listener);
> +                        resolve();
> +                    });

This could be flakey if the page actually did trigger the event. The `setTimeout` is not safe, you should wait for some event from the page itself.

I suggest:

    function clickBody() {
        document.body.click();
        TestPage.dispatchEventToFrontend("AfterClick");
    }

And here:

    InspectorTest.singleFireEventListener("AfterClick", () => {
        ...
    });

This guarantees no race, the setTimeout here doesn't.

> LayoutTests/inspector/dom/setEventListenerDisabled.html:46
> +    suite.addTestCase({
> +        name: "DOM.setEventListenerDisabled.ReenabledClickEvent",

You're missing a test for:

    DOM.getEventListenersForNode

When the node has a disabled event listener. Should be easy.

> LayoutTests/inspector/dom/setEventListenerDisabled.html:110
> +            TestPage.dispatchEventToFrontend("Clicked");

Nit: A name like "TestPageDocumentClicked" might be easier to search.
Comment 19 Devin Rousso 2017-10-14 02:46:36 PDT
Created attachment 323805 [details]
Patch
Comment 20 WebKit Commit Bot 2017-10-14 11:56:35 PDT
Comment on attachment 323805 [details]
Patch

Clearing flags on attachment: 323805

Committed r223321: <https://trac.webkit.org/changeset/223321>
Comment 21 WebKit Commit Bot 2017-10-14 11:56:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Radar WebKit Bug Importer 2017-10-14 11:57:03 PDT
<rdar://problem/34994925>
Comment 23 WebKit Commit Bot 2017-10-18 11:19:09 PDT
Re-opened since this is blocked by bug 178476
Comment 24 Joseph Pecoraro 2017-10-25 17:10:30 PDT
I'm going to take a look at getting this landed and then remanding it.
Comment 25 Joseph Pecoraro 2017-10-25 17:50:51 PDT
Comment on attachment 323805 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323805&action=review

> Source/WebInspectorUI/UserInterface/Views/EventListenerSectionGroup.js:55
> +        if (DOMAgent.setEventListenerDisabled)

I'm going to limit this to a non-zero event listener id. So:

    if (DOMAgent.setEventListenerDisabled && this._eventListener.eventListenerId)

I recognize that eventListenerId is a required property and we don't expect it to be zero but for Augmenting Agents support right now I think its safe to say "if its 0, just ignore it". We don't currently use the id for anything other than enable/disable.
Comment 26 Joseph Pecoraro 2017-10-25 17:56:58 PDT
Created attachment 324931 [details]
[PATCH] For Bots
Comment 27 WebKit Commit Bot 2017-10-25 21:08:03 PDT
Comment on attachment 324931 [details]
[PATCH] For Bots

Clearing flags on attachment: 324931

Committed r224002: <https://trac.webkit.org/changeset/224002>
Comment 28 WebKit Commit Bot 2017-10-25 21:08:05 PDT
All reviewed patches have been landed.  Closing bug.