Closed Bug 1052736 Opened 10 years ago Closed 8 years ago

Event listeners popup does not show Document listeners

Categories

(DevTools :: Inspector, defect, P2)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: aoprea, Assigned: miker)

References

Details

(Whiteboard: [polish-backlog][difficulty=easy])

Attachments

(2 files, 1 obsolete file)

show-listeners-on-document-element-1052736.patch
(deleted), patch
Details | Diff | Splinter Review
testcase-event.html
(deleted), text/html
Details 
      No description provided.
Summary: Devtools → Event listeners popup does not show React events
Andrei mentioned this on IRC.

It looks like the issue is that React adds listeners like "click" to the Document, not a specific DOM node.

I am guessing that's why they are currently missed in the Inspector's event popup.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
This part is more general than just React, as it would affect anything that listens on the Document, which happens to also include React.

Mike, perhaps these could be grouped with listeners shown the root node?
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Summary: Event listeners popup does not show React events → Event listeners popup does not show Document listeners
I think it would make sense to add listeners bound to the document on either the doctype node in the markup view or the <html> node.  Mike, what do you think?
Flags: needinfo?(mratcliffe)
(In reply to Brian Grinstead [:bgrins] from comment #6)
> I think it would make sense to add listeners bound to the document on either
> the doctype node in the markup view or the <html> node.  Mike, what do you
> think?

This is similar to jQuery live events. A listener is added to the document so it bubbles through all nodes. On nodes matching a given selector the event will be triggered.

We show window listeners on the html node because body listeners are hoisted to the window object and we want them to be visible... I guess we could include document listeners there.

There is some debate about whether we should support React events out of the box or whether Firebug Next should provide support for React. We provide jQuery support because our tools are useless for lots of people if we don't support jQuery but we are /probably/ going to leave it to Firebug Next to support other libraries.

We will need to discuss this before continuing.
Assignee: nobody → mratcliffe
Flags: needinfo?(mratcliffe)
Well, whether or not we support React events, I do think we should have support in our core tools for displaying the listeners bound to document somewhere.

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #7)
> We will need to discuss this before continuing.

Are you referring to React only here, or also whether to show the document listeners at all?
(In reply to J. Ryan Stinnett [:jryans] from comment #8)
> Well, whether or not we support React events, I do think we should have
> support in our core tools for displaying the listeners bound to document
> somewhere.
> 
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #7)
> > We will need to discuss this before continuing.
> 
> Are you referring to React only here, or also whether to show the document
> listeners at all?

Sorry, we should totally show document listeners on the HTML node.
Comment on attachment 8496797 [details] [diff] [review]
show-listeners-on-document-element-1052736.patch

Review of attachment 8496797 [details] [diff] [review]:
-----------------------------------------------------------------

This does fix events on document.documentElement, but it is still not showing up for document. See http://bgrins.github.io/devtools-demos/inspector/iframe.html, which has a DOMContentLoaded event bound to document.
Attachment #8496797 - Flags: review?(bgrinstead)
- Now checks document & documentElement.
- Added diff.js, which can be used by all tools for simple text diffs.
- Updated tests.
Attachment #8496797 - Attachment is obsolete: true
Attachment #8498152 - Flags: review?(bgrinstead)
Comment on attachment 8498152 [details] [diff] [review]
show-listeners-on-document-element-1052736.patch

Review of attachment 8498152 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the slow review.  I have a couple of questions:

::: browser/devtools/shared/diff.js
@@ +1,1 @@
> +/**

I wonder if this file should be left in a test directory as a helper js file.  Bug 960987 is introducing diff_match_patch - there is a patch that introduces it here: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=960987&attachment=8481606 and a patch that implements a getDiff function in showDifferences.js here: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=960987&attachment=8481607.  Do you think we could easily reuse that functionality?  If not, I'd be inclined to put this instead in a test/ directory since it will probably only be used for this textDiff function.

::: toolkit/devtools/event-parsers.js
@@ +8,5 @@
>  "use strict";
>  
>  const {Cc, Ci, Cu} = require("chrome");
>  
> +const JQUERY_LIVE_REGEX = /return typeof \w+.*.event\.triggered[\s\S]*\.event\.(dispatch|handle).*arguments/;

I don't understand what this is doing.  Is this trying to match the contents of a particular function from jQuery?  I'm also concerned about having hardcoded jQuery specific stuff inside the "DOM Events" parser - in theory any library should be able to provide a similar mechanism, right?

::: toolkit/devtools/server/actors/inspector.js
@@ +548,4 @@
>      if (this.rawNode.nodeName.toLowerCase() === "html") {
> +      let winListeners = this.getEventListeners(node.ownerGlobal) || [];
> +      let docElementListeners = this.getEventListeners(node) || [];
> +      let docListeners = this.getEventListeners(node.parentNode) || [];

Can always assume that if it's an HTML tag then the parentNode will be the document? Maybe just use node.ownerDocument to be a bit more explicit if that works
Attachment #8498152 - Flags: review?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #13)
> Comment on attachment 8498152 [details] [diff] [review]
> show-listeners-on-document-element-1052736.patch
> 
> Review of attachment 8498152 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the slow review.  I have a couple of questions:
> 
> ::: browser/devtools/shared/diff.js
> @@ +1,1 @@
> > +/**
> 
> I wonder if this file should be left in a test directory as a helper js
> file.  Bug 960987 is introducing diff_match_patch - there is a patch that
> introduces it here:
> https://bugzilla.mozilla.org/page.cgi?id=splinter.
> html&bug=960987&attachment=8481606 and a patch that implements a getDiff
> function in showDifferences.js here:
> https://bugzilla.mozilla.org/page.cgi?id=splinter.
> html&bug=960987&attachment=8481607.  Do you think we could easily reuse that
> functionality?  If not, I'd be inclined to put this instead in a test/
> directory since it will probably only be used for this textDiff function.
> 
> ::: toolkit/devtools/event-parsers.js
> @@ +8,5 @@
> >  "use strict";
> >  
> >  const {Cc, Ci, Cu} = require("chrome");
> >  
> > +const JQUERY_LIVE_REGEX = /return typeof \w+.*.event\.triggered[\s\S]*\.event\.(dispatch|handle).*arguments/;
> 
> I don't understand what this is doing.  Is this trying to match the contents
> of a particular function from jQuery?  I'm also concerned about having
> hardcoded jQuery specific stuff inside the "DOM Events" parser - in theory
> any library should be able to provide a similar mechanism, right?

So the jQuery live parser has already seen this particular handler when it added it for the live selector.  Could there be some way for any parser to indicate that it is going to handle this function and the DOM Events parser should ignore it?  Maybe a function that receives a node and returns a boolean or some sort of WeakMap that contains known handler objects that should be ignored by other parsers.
Whiteboard: [devedition-40][difficulty=easy]
Priority: -- → P2
Whiteboard: [devedition-40][difficulty=easy] → [polish-backlog][difficulty=easy]
> Can always assume that if it's an HTML tag then the parentNode will be the document?

No.  Nor should you assume the root element is <html>.

Checking node.ownerDocument.documentElement == node is the reliable way to detect the root element.
Priority: P2 → P3
Assignee: mratcliffe → nobody
Priority: P3 → P2
I worked on this during an all-hands but was told to drop it in favor of another feature.

This patch is very bitrotted and files have moved but if we can find the locations of the methods in event-parsers.js and inspector.js it is an easy fix.

The jQuery regex is only there because we need to be able to exclude jQuery events... this regex could just as easily come from devtools/server/event-parsers.js.
Attached file testcase-event.html (deleted) — 
(attaching the testcase of bug 1299456)
There's been many duplicate of this over time. We really need to provide a fix for this. Adding this to the top-inspector-bugs list.
Blocks: top-inspector-bugs
No longer blocks: 1315639
Depends on: 1315639
This was fixed by bug 1315639.
Assignee: nobody → mratcliffe
Status: NEW → RESOLVED
Closed: 10 years ago → 8 years ago
Resolution: --- → FIXED
I can confirm that this is fixed in Firefox 53.0a1 2016-11-30.

Sebastian
Status: RESOLVED → VERIFIED
No longer blocks: top-inspector-bugs
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: