Web Inspector: provide a way to enable/disable event listeners

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

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.

[BJ Burg]

It should be doable in the event listeners sidebar, right?

[Devin Rousso]

Created attachment 321721 [details]
Patch

[Devin Rousso]

Created attachment 321722 [details]
[Image] After Patch is applied

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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).

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Devin Rousso]

(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.

[Devin Rousso]

Created attachment 321793 [details]
Patch

[Devin Rousso]

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.

[Joseph Pecoraro]

(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...".

[Devin Rousso]

Created attachment 323777 [details]
Patch

[Devin Rousso]

Created attachment 323778 [details]
[Image] After Patch is applied

[Joseph Pecoraro]

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.

[Devin Rousso]

Created attachment 323805 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 323805 [details]
Patch

Clearing flags on attachment: 323805

Committed r223321: <https://trac.webkit.org/changeset/223321>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34994925>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 178476

[Joseph Pecoraro]

I'm going to take a look at getting this landed and then remanding it.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 324931 [details]
[PATCH] For Bots

[WebKit Commit Bot]

Comment on attachment 324931 [details]
[PATCH] For Bots

Clearing flags on attachment: 324931

Committed r224002: <https://trac.webkit.org/changeset/224002>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.