918416 - Jump to definition for event listeners

Status: RESOLVED WONTFIX

(DevTools :: Debugger, defect, P3)

Description

[Victor Porof [:vporof][:vp]]

This would be lovely to have.

Right click + list of listeners + clicking could work. Or there could be a button that shows when hovering. Or something else. Followup for bug 800857.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

This could be a good bug for someone who has hacked on the debugger a bit before.

We get the definition site before we start adding listeners to the view: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-controller.js#1461

But then we ignore the line number and just grab the URL in the view's |addListener|: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-panes.js#1834

We should use that url / line info to add the ability to jump to the definition site of the listener function.

Comment 2

[Alexey Novak]

Hi,

I would love to take care of this bug... or at least try :)

Comment 3

[Alexey Novak]

I was re-reading this bug again and again.

Just before I start writing any code it is important for me to understand the scope of this bug.

Even though I need to do some more reading.. there are so many bugs within bugs once you start reading all the issue related 800857 and 918715.. developer can break on a breakpoint which will immediately allow to see a list of Events. I assume you can only disable/enable listeners for those events. We want to add an ability to "jump" to the listener once we click on the event. By "jump" we mean to see function in the main Debugger window with code.
But then what Victor is talking is right click on the event to see all attached listeners and jump to the one selected in the context menu.
..
will do some more reading. But If I'm on the wrong path.. please let me know.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Alexey Novak from comment #3)
> I was re-reading this bug again and again.
> 
> Just before I start writing any code it is important for me to understand
> the scope of this bug.
> 
> Even though I need to do some more reading.. there are so many bugs within
> bugs once you start reading all the issue related 800857 and 918715..
> developer can break on a breakpoint which will immediately allow to see a
> list of Events. I assume you can only disable/enable listeners for those
> events. We want to add an ability to "jump" to the listener once we click on
> the event. By "jump" we mean to see function in the main Debugger window
> with code.
> But then what Victor is talking is right click on the event to see all
> attached listeners and jump to the one selected in the context menu.
> ..
> will do some more reading. But If I'm on the wrong path.. please let me know.

This is just to add a little icon next to each event listener that when you click the icon, the debugger's source editor jumps to the definition of that function. We can reuse the icon that the profiler has for jumping to function definitions from there.

I can't figure out where this icon lives, perhaps Anton can point us in the right direction.

Comment 5

[Victor Porof [:vporof][:vp]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #4)
> 
> This is just to add a little icon next to each event listener that when you
> click the icon, the debugger's source editor jumps to the definition of that
> function. We can reuse the icon that the profiler has for jumping to
> function definitions from there.

I don't think that will work, since multiple event listeners of the same type and on the same target node are collapsed in the UI, even if they're created in two different locations of the same source. A context menu might be better suited.

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Ah yes, I forgot about that.

So you'd want a sub menu? Like

Jump to definition > listener 1
                     listener 2
                     listener 3
                     listener 4
                     listener 5
                     ...

Comment 7

[Victor Porof [:vporof][:vp]]

Yeah, that makes sense.

Comment 8

[Alexey Novak]

Ugh... sorry guys, I found myself a bit overwhelmed for the past few days :( should have more time to look into it soon.

Do you want to see a context menu or a submenu for this?

What about user interaction? What user supposed to do in order to see this menu?
Left click or Right click or mouse Hover(in case if it is a context menu) ?

(Victor, you mentioned clicking in the bug's description but I want to be clear on the requirements)

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

You should be able to right click on an event listener item in the UI and have a context menu come up which has a sub menu labeled "Jump to listener definition" (or something like that) which has each individual event listener function listed in the sub menu. Selecting one of the items from the sub menu should jump you to the source in the editor.

Comment 10

[Alexey Novak]

Thanks a lot for clarification!

Comment 11

[Anton Kovalyov (:anton)]

The icon is in browser/devtools/profiler/cleopatra/images/circlearrow.svg but I think it's better to just use a unicode right-pointed black triangle for these purposes.

Comment 12

[Alexey Novak]

Would I still need an arrow icon if we are going to use an icon if we are going to right click on the event listener.
But then the event listeners are just eventType labels in bold, how user would know that the right click would show a context menu. Sounds a bit ambiguous for me.

just an update so that no one thinks that I disappeared
Still reading through the code.
It seems that I have to call something similar to:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-panes.js#654
When user does a right click (which I need to attach) on an item which is created at:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-panes.js#2470

_onCheck and _onClick should be modified since they capture both left & right clicks to check/uncheck the checkbox.

Not sure about submenus in the context menus (have not seen one) but it is (probably) possible to create a non-clickable menu item "Jump to listener definition" then a separator and then list of listeners.

Comment 13

[Alexey Novak]

I have a very basic n00b question.

I was playing around with the code trying to do different things but they did not work for me so that I decided to debug it. Running gdb probably would be an overkill and not sure how to do this so I decided to do a basic variable output.

In previous bugs there was a suggestion to use |console.log| or |console.error| or |dump| functions.
So the following code was added:
    console.log('a1');
    console.error('a2');
    dump('a3');

to 
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-panes.js#2500
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-panes.js#2296
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-panes.js#2531

The problem that I do not see any messages in Console neither in Terminal.
Any suggestions on how to debug things and best ways to do that ?

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

Are you sure you are looking at the Browser Console? console.log() output appears there for me. dump() should appear in the terminal if you have browser.dom.window.dump.enabled set to true.

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Make sure that both

  browser.dom.window.dump.enabled
  devtools.debugger.log

are set to true in about:config. The first should make sure that dump calls actually print to stdout, and the second gives you extra logging (mostly RDP packets, generally valuable for this kind of stuff).

As Panos said, make sure you are using the *browser* console (Cmd+Shift+J), not the content toolbox console.

I also suggest using the browser debugger (whole toolbox now, really): https://developer.mozilla.org/en-US/docs/Debugging_JavaScript#JavaScript_Debugger

Comment 16

[Alexey Novak]

Thanks guys! I will give it a shot once I get home from work.

Ugh... I should start bringing my laptop to work so that it is possible to try such things right away.

Comment 17

[Alexey Novak]

Wow this is so awesome! I have been using FireFox for ages but never seen such cool tools as Browser Toolbox or *browser* console... this would make my life SO much easier. Thanks again.

That's amazing how you can set breakpoints in the browser's code. Although was weird that I could not find debugger-panes.js in the list of Sources :S
But have my all kind of logging setup, console and dumps. All works after changing settings as advised ^

Comment 18

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Glad that you're making progress ;)

Sources won't show up in the list until they have actually been loaded, so if you don't have the debugger open, you probably won't see debugger-panes.js. If you need to break on initial start up code in debugger-panes.js, try using a debugger statement[0].

[0] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/debugger

Comment 19

[Alexey Novak]

Attachment: 918416.1.diff

This bug is very hard.. probably because I'm still fresh to the codebase. Should've thought twice before taking it.

After so many failures just decided to focus on smaller tasks.

1- make context menu(CM) to show up for the proper UI event row when user right clicks
2- show proper listeners in the popup. By knowing what row was clicked
3- create "jump" functionality when you click an element
4- style CM properly

This diff is just my result for (1) (after days of searching I based it on one of the context menus in the inspector-panel.js)

Next is (2) and not quite sure how I would link that XUL (target) row which was clicked to the |selectors| in |AddListener| function.


VERY slowly progressing in this bug

Comment 20

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

You don't need to style code mirror at all for this patch, just call `DebuggerView.setEditorLocation`[0]. Will take a look at your patch.

[0] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-view.js#448

Comment 21

[Alexey Novak]

Oh that file is FAR away from being a patch... just slowly progressing. Was happy to see that could display a popup at least.

Comment 22

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8375305 [details] [diff] [review]
918416.1.diff

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

This is a good start, keep with it :)

See hints below.

::: browser/devtools/debugger/debugger-panes.js
@@ +2312,5 @@
> +
> +    debugger;
> +
> +    this._listenersMenu = document.getElementById("debuggerEventListenersViewContextMenu");
> +    this.lastListenersMenuItem = this._listenersMenu.lastChild;

It is better to rely on ID than order of the children.

@@ +2553,5 @@
> +  _onClick: function({ target, button }) {
> +    // Show a context menu with event listeners for right click
> +    if (button === 2) {
> +      debugger;
> +      this._listenersMenu.openPopup(target, "before_start", 0, 0, true, false);

Rather than managing the popup yourself, you should just connect it to the element via the "context" attribute and let the platform open it for you[0]. You just give your popup an ID and then set the "context" attribute to that ID.

You should create the popup and do the plumbing to connect it to the given item from `_createItemView`[1]. Another example of a dynamically created context menu is the context menu for breakpoint items[2].

[0] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-panes.js#2028
[1] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-panes.js#2470
[2] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-panes.js#654

Comment 23

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://developer.mozilla.org/en-US/docs/XUL/Attribute/context

Comment 24

[Alexey Novak]

Hi there,

I will skip on this bug. I better go and fix some basic ones before I jump into something like this one.

Comment 25

[Panos Astithas (he/him) [:past] (please ni?)]

Sure thing, thanks for the note!

Comment 26

[Kevin Wern]

Hi, is this feature still available? I'd be up to give it a shot.

Comment 27

[:Harald Kirschner :digitarald]

Thanks for offering help, Kevin! The new Debugger frontend does not have an event listener panel, which makes this bug void.

Good news is that we have plans to a new Event Breakpoint panel, which eventually will not just land in the Debugger but also add better event handler inspection to the Inspector. Work is happening in https://github.com/devtools-html/debugger.html/issues/7785 and we will file bugs in bugzilla for the server-side work that is just starting.