736078 - [visual event] Show which elements have listeners attached

Status: RESOLVED FIXED

(DevTools :: General, defect, P3)

Description

[Paul Rouget [:paul]]

For interactive UI, web developers attach event listener to DOM elements. mousemove/down/up, click, keypress/down/up, …

It would be interesting to have a list of elements with listeners, with the type of events.

This information could be display on the page, or be a command.

Comment 1

[Paul Rouget [:paul]]

Similar tool: http://www.sprymedia.co.uk/article/Visual+Event

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

From bug 850499:

There is an excellent bookmarklet called Visual Events:
http://www.sprymedia.co.uk/article/Visual+Event+2

This would allow users to see events attached to node (via jQuery etc.)

We should implement an API that allows to gather this information so that it can be displayed:
- When inspecting via the markup view.
- When a node is locked (next to the selector).
- Via GCLI ... page overlay containing all information:
  a. in boxes next to each element containing events.
  b. in a box next to a specific element.

Jim's comments from bug 854439 should be useful here.

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Visual Events Scratchpad

Comment 5

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

In bug 832982 I am implementing an "eventListeners" protocol request that can be used to get this list. The debugger will most likely have a separate pane with the list of these scripts, but we may want to somehow present this information in the inspector, too.

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Panos Astithas [:past] from comment #5)
> In bug 832982 I am implementing an "eventListeners" protocol request that
> can be used to get this list. The debugger will most likely have a separate
> pane with the list of these scripts, but we may want to somehow present this
> information in the inspector, too.

Yes, we want to have:
- A gcli command to display listeners in an overlay
- Hoverable boxes:
  - In the markup view.
  - Next to selector when a node is locked.

We will wait for bug 832982 to land before continuing with this.

Comment 7

[Girish Sharma [:Optimizer]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #6)
> (In reply to Panos Astithas [:past] from comment #5)
> > In bug 832982 I am implementing an "eventListeners" protocol request that
> > can be used to get this list. The debugger will most likely have a separate
> > pane with the list of these scripts, but we may want to somehow present this
> > information in the inspector, too.
> 
> Yes, we want to have:
> - A gcli command to display listeners in an overlay
> - Hoverable boxes:
>   - In the markup view.

Why complicate the UI by using hover-able boxes ? We can have another tab in the sidebar to show events attached to the selected node.

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Take a look at dragonflies HTML panel ... it works great as a hover-able box there. An events panel would be another option though.

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch V2

No longer on for tests by default

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: visual-events-736078.patch

Works fine on content and browser.

- When a DOM0 event is added i.e. onclick="doit()" it is not possible to get the line number of the script as it is dynamically generated.
- Darrin suggested showing a different colored icon if the same event is added twice. The problem with this is that it may be valid to add the same handler twice e.g. when using an object containing handleEvent.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=9f038d177890

Darrin, do you have any comments on the UX? If we really want the variables view widget to support multiline then it should be part of a new bug.

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: browser_markupview_events.html

Test case

Comment 12

[Joe Walker [:jwalker]]

Comment on attachment 8404063 [details] [diff] [review]
visual-events-736078.patch

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

Comments on the UI:

* It seems bizarre to lump window events onto the body. The <html> element would be better wouldn't it (although still not right). Maybe there are better suggestions still?
* What's the use for the "Selector:" field. Isn't it always a link to the thing on the left?
* Except in the case of a window event shown on the body when it's not even a selector
* I'm unclear what allowsUntrusted means to a web developer. Is it ever false?
* I don't think we need both the Handler and the Origin to be links - probably just the Origin will do
* I think we do need the Handler to be multi-line, but we can have a follow-up

I suggest we move the Origin to the header line, and remove the table, replacing it with a multi-line source area.

For example:

> scroll _extras.js:454_ (DOM2, Bubbling)
V load _script.js:103_ (DOM2, Bubbling)
    function onLoad(ev) {
      // blah
    }
> error _script.js:34_ (DOM2, Bubbling)

There is code in the debugger to simplify a full URL to something shorter but still unique IIRC.

::: browser/devtools/markupview/markup-view.css
@@ +173,5 @@
> +  font-size: 0.8em;
> +  font-weight: bold;
> +  line-height: 10px;
> +  border-radius: 50%;
> +  padding: 0px 2px;

I think a number of these should be in theme folder

::: browser/devtools/markupview/markup-view.js
@@ +1146,5 @@
>      this.tooltip.destroy();
>      this.tooltip = null;
>  
> +    this.eventsTooltip.destroy();
> +    this.eventsTooltip = null;

This looks like belt and braces memory leak protection, which always looks dodgy to me. There should be a Right Way, and we should do that rather than scattering '=null' around and hoping. But I'll admit there's form for this behaviour in this file so I'm not going to be picky about it.

::: browser/devtools/markupview/markup-view.xhtml
@@ +29,5 @@
>  
>        <li id="template-more-nodes" class="more-nodes devtools-class-comment" save="${elt}"><span>${showing}</span> <button href="#" onclick="${allButtonClick}">${showAll}</button></li>
>      </ul>
>  
> +    <span id="template-element" save="${elt}" class="editor"><span class="open">&lt;<span save="${tag}" class="tag theme-fg-color3" tabindex="0"></span><span save="${attrList}"></span><span save="${newAttr}" class="newattr" tabindex="0"></span><span class="closing-bracket">&gt;</span></span><span class="close">&lt;/<span save="${closeTag}" class="tag theme-fg-color3"></span>&gt;</span><div save="${eventNode}" class="markupview-events" data-event="true">ev</div></span>

Ug. Could you line wrap this?

I'm not sure why the first &gt; needs to be wrapped in a .closing-bracket, but it's paired &lt; doesn't.

Am I right that you just added
<div save="${eventNode}" class="markupview-events" data-event="true">ev</div>
?

::: browser/devtools/markupview/test/browser_markupview_events.js
@@ +106,5 @@
> +
> +  function checkEventsForNode(node, expected) {
> +    let def = promise.defer();
> +    let container = getContainerForRawNode(node, inspector);
> +    let ev = container.elt.querySelector(".markupview-events");

ev sounds like its an event to me. eventHolder or something?

@@ +110,5 @@
> +    let ev = container.elt.querySelector(".markupview-events");
> +
> +    ev.scrollIntoView();
> +
> +    // Wait for scrollIntoView to complete.

Can we be sure that scrollIntoView is done by the time we get here?

Also how about something like:

function promiseNextTick() {
  let deferred = promise.defer();
  executeSoon(deferred.resolve);
  return deferred.promise;
}

So you can just:

yield promiseNextTick();

@@ +112,5 @@
> +    ev.scrollIntoView();
> +
> +    // Wait for scrollIntoView to complete.
> +    executeSoon(() => {
> +      inspector.markup.eventsTooltip.once("shown", () => {

You can also:

yield inspector.markup.eventsTooltip.once("shown");

::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
@@ +1376,5 @@
> +
> +# LOCALIZATION NOTE (showeventsDesc) These strings describe
> +# the 'media' commands and all available parameters.
> +showeventsDesc=Show all events for page
> +showEventsManual=Show all events for page

s/showeventsDesc/showEventsDesc
But also I think this wander in my mistake?

::: toolkit/devtools/server/actors/inspector.js
@@ +112,5 @@
>  });
>  
> +loader.lazyGetter(this, "Debugger", function() {
> +  let JsDebugger = {};
> +  Cu.import("resource://gre/modules/jsdebugger.jsm", JsDebugger);

Or, simpler:
let JsDebugger = require("resource://gre/modules/jsdebugger.jsm");

@@ +236,5 @@
>        name: this.rawNode.name,
>        publicId: this.rawNode.publicId,
>        systemId: this.rawNode.systemId,
>  
> +      hasEventListeners: this._hasEventListeners,

Can we not assign to hasEventListeners below?

@@ +326,5 @@
> +    let global = node.ownerGlobal ?
> +                 node.ownerGlobal.wrappedJSObject :
> +                 node.wrappedJSObject;
> +    let dbg = new Debugger();
> +    let DOwindow = dbg.addDebuggee(global);

DO? Doesn't feel like it fits the naming convention
See also listenerDO?

@@ +389,5 @@
> +            DOM0: dom0,
> +            capturing: handler.capturing,
> +            allowsUntrusted: handler.allowsUntrusted,
> +            script: scriptSource,
> +            origin: "?"

This is user visible, right? Can't we do something better?

Comment 13

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: visual-events-736078.patch

(In reply to Joe Walker [:jwalker] from comment #12)
> Comment on attachment 8404063 [details] [diff] [review]
> visual-events-736078.patch
> 
> Review of attachment 8404063 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Comments on the UI:
> 
> * It seems bizarre to lump window events onto the body. The <html> element
> would be better wouldn't it (although still not right). Maybe there are
> better suggestions still?

This is because most events added to document.body are actually hoisted to window. e.g.
document.body.addEventListener("load", () => {
  ...
});

Adds a load event listener to window. In this situation the user would expect to see the load event on the body tag although that is not technically correct. It turns out that this is normal for the majority of listeners that can be added to document.body... it is a shame that there is not a better solution for this.

> * What's the use for the "Selector:" field. Isn't it always a link to the
> thing on the left?

In the markup view it is so I am happy to remove them. If we created the overlay version then the selectors would be useful again.

> * Except in the case of a window event shown on the body when it's not even
> a selector

Removed

> * I'm unclear what allowsUntrusted means to a web developer. Is it ever
> false?

It can be true on very rare occasions but I agree that this is normally not useful... I will remove it.

> * I don't think we need both the Handler and the Origin to be links -
> probably just the Origin will do

Changed

> * I think we do need the Handler to be multi-line, but we can have a
> follow-up
> 

Bug 994756 for multi-line variable view. Even though we don't use it any longer it could be useful in the future.

> I suggest we move the Origin to the header line, and remove the table,
> replacing it with a multi-line source area.
> 
> For example:
> 
> > scroll _extras.js:454_ (DOM2, Bubbling)
> V load _script.js:103_ (DOM2, Bubbling)
>     function onLoad(ev) {
>       // blah
>     }
> > error _script.js:34_ (DOM2, Bubbling)
> 
> There is code in the debugger to simplify a full URL to something shorter
> but still unique IIRC.
> 

Now we just show the filename.

> ::: browser/devtools/markupview/markup-view.css
> @@ +173,5 @@
> > +  font-size: 0.8em;
> > +  font-weight: bold;
> > +  line-height: 10px;
> > +  border-radius: 50%;
> > +  padding: 0px 2px;
> 
> I think a number of these should be in theme folder
> 

Moved

> ::: browser/devtools/markupview/markup-view.js
> @@ +1146,5 @@
> >      this.tooltip.destroy();
> >      this.tooltip = null;
> >  
> > +    this.eventsTooltip.destroy();
> > +    this.eventsTooltip = null;
> 
> This looks like belt and braces memory leak protection, which always looks
> dodgy to me. There should be a Right Way, and we should do that rather than
> scattering '=null' around and hoping. But I'll admit there's form for this
> behaviour in this file so I'm not going to be picky about it.
> 

Okay

> ::: browser/devtools/markupview/markup-view.xhtml
> @@ +29,5 @@
> >  
> >        <li id="template-more-nodes" class="more-nodes devtools-class-comment" save="${elt}"><span>${showing}</span> <button href="#" onclick="${allButtonClick}">${showAll}</button></li>
> >      </ul>
> >  
> > +    <span id="template-element" save="${elt}" class="editor"><span class="open">&lt;<span save="${tag}" class="tag theme-fg-color3" tabindex="0"></span><span save="${attrList}"></span><span save="${newAttr}" class="newattr" tabindex="0"></span><span class="closing-bracket">&gt;</span></span><span class="close">&lt;/<span save="${closeTag}" class="tag theme-fg-color3"></span>&gt;</span><div save="${eventNode}" class="markupview-events" data-event="true">ev</div></span>
> 
> Ug. Could you line wrap this?
> 

I can wrap my line but the rest are inline so wrapping them produces extra whitespace.

> I'm not sure why the first &gt; needs to be wrapped in a .closing-bracket,
> but it's paired &lt; doesn't.
> 
> Am I right that you just added
> <div save="${eventNode}" class="markupview-events" data-event="true">ev</div>
> ?
> 

Yup

> ::: browser/devtools/markupview/test/browser_markupview_events.js
> @@ +106,5 @@
> > +
> > +  function checkEventsForNode(node, expected) {
> > +    let def = promise.defer();
> > +    let container = getContainerForRawNode(node, inspector);
> > +    let ev = container.elt.querySelector(".markupview-events");
> 
> ev sounds like its an event to me. eventHolder or something?
> 

Changed

> @@ +110,5 @@
> > +    let ev = container.elt.querySelector(".markupview-events");
> > +
> > +    ev.scrollIntoView();
> > +
> > +    // Wait for scrollIntoView to complete.
> 
> Can we be sure that scrollIntoView is done by the time we get here?
> 
> Also how about something like:
> 
> function promiseNextTick() {
>   let deferred = promise.defer();
>   executeSoon(deferred.resolve);
>   return deferred.promise;
> }
> 
> So you can just:
> 
> yield promiseNextTick();
> 

Done

> @@ +112,5 @@
> > +    ev.scrollIntoView();
> > +
> > +    // Wait for scrollIntoView to complete.
> > +    executeSoon(() => {
> > +      inspector.markup.eventsTooltip.once("shown", () => {
> 
> You can also:
> 
> yield inspector.markup.eventsTooltip.once("shown");
> 

Done

> ::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
> @@ +1376,5 @@
> > +
> > +# LOCALIZATION NOTE (showeventsDesc) These strings describe
> > +# the 'media' commands and all available parameters.
> > +showeventsDesc=Show all events for page
> > +showEventsManual=Show all events for page
> 
> s/showeventsDesc/showEventsDesc
> But also I think this wander in my mistake?
> 

Removed

> ::: toolkit/devtools/server/actors/inspector.js
> @@ +112,5 @@
> >  });
> >  
> > +loader.lazyGetter(this, "Debugger", function() {
> > +  let JsDebugger = {};
> > +  Cu.import("resource://gre/modules/jsdebugger.jsm", JsDebugger);
> 
> Or, simpler:
> let JsDebugger = require("resource://gre/modules/jsdebugger.jsm");
> 

Done

> @@ +236,5 @@
> >        name: this.rawNode.name,
> >        publicId: this.rawNode.publicId,
> >        systemId: this.rawNode.systemId,
> >  
> > +      hasEventListeners: this._hasEventListeners,
> 
> Can we not assign to hasEventListeners below?
> 

No, it is a getter.

> @@ +326,5 @@
> > +    let global = node.ownerGlobal ?
> > +                 node.ownerGlobal.wrappedJSObject :
> > +                 node.wrappedJSObject;
> > +    let dbg = new Debugger();
> > +    let DOwindow = dbg.addDebuggee(global);
> 
> DO? Doesn't feel like it fits the naming convention
> See also listenerDO?
> 

Changed to globalDebugObject and listenerDebugObject.

> @@ +389,5 @@
> > +            DOM0: dom0,
> > +            capturing: handler.capturing,
> > +            allowsUntrusted: handler.allowsUntrusted,
> > +            script: scriptSource,
> > +            origin: "?"
> 
> This is user visible, right? Can't we do something better?

I have replaced this with "URI unavailable."

#################

We also now create our own widget to display the events. I haven't made it reusable as I don't think it will be useful anywhere else.

I am still not happy with the UI... we really should display bubbling, capturing, DOM0 and DOM2 as icons... I have logged bug 1002484 for this.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=be7fd0f24103

Comment 14

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: visual-events-736078.patch

As per our discussion:

Colors
Now matches other colors in theme.

Window vs body
We now ignore hoisting and show window events on the html element.

System events
We now check if the source matches "function () {[native code]}" and do not display the event button on a match. This is as close to "is available to user" as we can get.
Some events in browser scripts still fail to return a URI even though the source code is available. This is because the following does not provide script.url:
let listenerDebugObject = globalDebugObject.makeDebuggeeValue(listener);
let script = listenerDebugObject.script;

Seems like eventHandler methods on an object in jsm files may be the culprit here.

Comment 15

[Joe Walker [:jwalker]]

Comment on attachment 8414467 [details] [diff] [review]
visual-events-736078.patch

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

::: browser/devtools/markupview/markup-view.xhtml
@@ +31,5 @@
>      </ul>
>  
> +    <span id="template-element" save="${elt}" class="editor"><span class="open">&lt;<span save="${tag}" class="tag theme-fg-color3" tabindex="0"></span><span save="${attrList}"></span><span save="${newAttr}" class="newattr" tabindex="0"></span><span class="closing-bracket">&gt;</span></span><span class="close">&lt;/<span save="${closeTag}" class="tag theme-fg-color3"></span>&gt;</span>
> +      <div save="${eventNode}" class="markupview-events" data-event="true">ev</div>
> +    </span>

Could you wrap this please; either in a separate patch or with a comment to say exactly what's changed.

::: browser/devtools/shared/widgets/Tooltip.js
@@ +529,5 @@
> +  setEventContent: function(
> +    {
> +      eventListenerInfos,
> +      toolbox
> +    }) {

Gak - does this need wrapping? It looks wierd.

::: browser/themes/shared/devtools/common.css
@@ +222,5 @@
> +  cursor: pointer;
> +}
> +
> +.event-tooltip-header-label {
> +  margin-left: 0;

-moz-margin-start?

::: toolkit/devtools/server/actors/inspector.js
@@ +66,5 @@
>  const PSEUDO_CLASSES = [":hover", ":active", ":focus"];
>  const HIDDEN_CLASS = "__fx-devtools-hide-shortcut__";
>  const XHTML_NS = "http://www.w3.org/1999/xhtml";
>  const IMAGE_FETCHING_TIMEOUT = 500;
> +const NATIVE_CODE = "function () {\n    [native code]\n}";

This feels very fragile, could we at least have a RegExp that looks for /function\s*\(\)\s*{\s*[native code]\s*}/ or something like that (not tested)?

I also think we need to ask jimb or someone if there is proper way to do this.

@@ +386,5 @@
> +              type: type,
> +              DOM0: dom0,
> +              capturing: handler.capturing,
> +              script: scriptSource,
> +              origin: "?"

I still think we need to chase down when we would get this and show something better to the user. (IIRC - this is user visible, right?)

Comment 16

[Simon Lindholm]

For inspiration, see also Firebug's implementation of this which just landed: https://blog.getfirebug.com/2014/05/16/firebug-2-0-beta-6/

Comment 17

[sys.sgx]

This is a much awaited feature. We could even integrate this with the 3D tool, and use different colors where events are attached.

Comment 18

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1. Main patch

Comment 19

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 2. Added semicolons, removed unused vars etc.

Comment 20

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Try:
https://tbpl.mozilla.org/?tree=Try&rev=0bdeb23d5449

Comment 21

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1.Main Patch

Fixed failing test.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=6ee9477c5419

Comment 22

[Joe Walker [:jwalker]]

Mike - is there an interdiff or am I just starting from scratch?

Comment 23

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Joe Walker [:jwalker] from comment #22)
> Mike - is there an interdiff or am I just starting from scratch?

Unfortunately, almost every part of the patch has changed so an interdiff is fairly pointless... sorry.

Comment 24

[Joe Walker [:jwalker]]

Comment on attachment 8441999 [details] [diff] [review]
1.Main Patch

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

::: browser/devtools/markupview/markup-view.js
@@ +113,5 @@
>    _selectedContainer: null,
>  
>    _initTooltips: function() {
>      this.tooltip = new Tooltip(this._inspector.panelDoc);
> +    this.eventsTooltip = new Tooltip(this._inspector.panelDoc);

It seems confusing that we have an eventsTooltip which is of type Tooltip, but which creates EventTooltips. Is this what you meant?

::: browser/devtools/markupview/markup-view.xhtml
@@ +17,5 @@
>  </head>
>  <body class="theme-body devtools-monospace" role="application">
> +
> +<!-- NOTE THAT WE MAKE EXTENSIVE USE OF HTML COMMENTS IN THIS FILE IN ORDER -->
> +<!-- TO MAKE SPANS READABLE WHILST AVOIDING WHITESPACE                      -->

Nit: *Significant* whitespace

::: browser/devtools/shared/widgets/Tooltip.js
@@ +523,5 @@
> +   *
> +   * @param {array} eventListenerInfos
> +   *        A list of event listeners.
> +   * @param {toolbox} toolbox
> +   *        toolbox used to select debugger panel.

These are properties of the first parameter rather than separate parameters?

::: toolkit/devtools/server/actors/inspector.js
@@ +340,5 @@
>      }
>      return ret;
>    },
>  
> +  getNodeName: function(node) {

It's not really a node name is it? nodeTitle?
Also - doc comment?

@@ +347,5 @@
> +    if (node.id) {
> +      nodeName += "#" + node.id;
> +    }
> +    if (node.className) {
> +      nodeName += "." + node.className;

className could be a string containing spaces. I suggest classList.map(className => '.' + className).join('') or similar.

@@ +349,5 @@
> +    }
> +    if (node.className) {
> +      nodeName += "." + node.className;
> +    }
> +    if (nodeName === "undefined") {

I think we should check for this case at the top of the function by testing against node rather than relying on node.nodeName being undefined.

@@ +355,5 @@
> +    }
> +    return nodeName;
> +  },
> +
> +  getEventListeners: function(node) {

Could you document the return value here

@@ +375,5 @@
> +        // If the listener is an object with a 'handleEvent' method, use that.
> +        if (listenerDO.class === "Object" || listenerDO.class === "XULElement") {
> +          let desc;
> +
> +          while (!desc && listenerDO) {

I think this will break if listenerDO.proto ever == null because we then assume below that we've correctly found a handeEvent prototype.

Can't we use getPropertyDescriptor?

@@ +397,5 @@
> +
> +        let scriptBeforeFunc = scriptSource.substr(0, script.sourceStart);
> +        let lastNewline = scriptBeforeFunc.lastIndexOf("\n");
> +        let lastSemicolon = scriptBeforeFunc.lastIndexOf(";");
> +        let lastClosingBrace = scriptBeforeFunc.lastIndexOf("\n");

That's not the lastClosingBrace!

@@ +398,5 @@
> +        let scriptBeforeFunc = scriptSource.substr(0, script.sourceStart);
> +        let lastNewline = scriptBeforeFunc.lastIndexOf("\n");
> +        let lastSemicolon = scriptBeforeFunc.lastIndexOf(";");
> +        let lastClosingBrace = scriptBeforeFunc.lastIndexOf("\n");
> +        let lastEnding = Math.max(lastNewline, lastSemicolon, lastClosingBrace);

When is a correctly defined lastClosingBrace alone not good enough?
Or, better why isn't trim() what we want here?

@@ +417,5 @@
> +
> +        let line = script.startLine;
> +        let url = script.url;
> +        let origin = url + (dom0 ? "" : ":" + line);
> +        let searchString = "";

I think it makes more sense if we use undefined as a 'no value' value.

@@ +436,5 @@
> +          searchString: searchString,
> +          lineCount: script.lineCount
> +        });
> +      }
> +      if (globalDO) {

we can move this inside "if (listener)" can't we, and move the definition of the globalDO inside too? And then we won't need to check for null.

Then we could do something like "if (!listener) { return [] }" at the top and reduce the indent level.

Comment 25

[Joe Walker [:jwalker]]

Also, I think there's a rebase needed?

Comment 26

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1. Main patch

(In reply to Joe Walker [:jwalker] from comment #24)
> Comment on attachment 8441999 [details] [diff] [review]
> 1.Main Patch
>
> Review of attachment 8441999 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: browser/devtools/markupview/markup-view.js
> @@ +113,5 @@
> >    _selectedContainer: null,
> >
> >    _initTooltips: function() {
> >      this.tooltip = new Tooltip(this._inspector.panelDoc);
> > +    this.eventsTooltip = new Tooltip(this._inspector.panelDoc);
>
> It seems confusing that we have an eventsTooltip which is of type Tooltip,
> but which creates EventTooltips. Is this what you meant?
>

This was a compromise because the default tooltip hides when a target nodes mouseout event is triggered (obviously there were other technical reasons).

Anyhow, we now only use this.tooltip and toggle between a persistent and non-persistent state when showing event tooltips.

> ::: browser/devtools/markupview/markup-view.xhtml
> @@ +17,5 @@
> >  </head>
> >  <body class="theme-body devtools-monospace" role="application">
> > +
> > +<!-- NOTE THAT WE MAKE EXTENSIVE USE OF HTML COMMENTS IN THIS FILE IN ORDER -->
> > +<!-- TO MAKE SPANS READABLE WHILST AVOIDING WHITESPACE                      -->
>
> Nit: *Significant* whitespace
>

Fixed.

> ::: browser/devtools/shared/widgets/Tooltip.js
> @@ +523,5 @@
> > +   *
> > +   * @param {array} eventListenerInfos
> > +   *        A list of event listeners.
> > +   * @param {toolbox} toolbox
> > +   *        toolbox used to select debugger panel.
>
> These are properties of the first parameter rather than separate parameters?
>

Fixed JSDoc comment.

> ::: toolkit/devtools/server/actors/inspector.js
> @@ +340,5 @@
> >      }
> >      return ret;
> >    },
> >
> > +  getNodeName: function(node) {
>
> It's not really a node name is it? nodeTitle?
> ...
> > +    }
> > +    if (nodeName === "undefined") {
>
> I think we should check for this case at the top of the function by testing
> against node rather than relying on node.nodeName being undefined.
>

Turns out that we no longer use it... removed.

> @@ +355,5 @@
> > +    }
> > +    return nodeName;
> > +  },
> > +
> > +  getEventListeners: function(node) {
>
> Could you document the return value here
>

Done

> @@ +375,5 @@
> > +        // If the listener is an object with a 'handleEvent' method, use that.
> > +        if (listenerDO.class === "Object" || listenerDO.class === "XULElement") {
> > +          let desc;
> > +
> > +          while (!desc && listenerDO) {
>
> I think this will break if listenerDO.proto ever == null because we then
> assume below that we've correctly found a handeEvent prototype.
>
> Can't we use getPropertyDescriptor?
>

We do use getPropertyDescriptor but we also need to walk the prototype chain. This does not break when listenerDO.proto == null.

> @@ +397,5 @@
> > +
> > +        let scriptBeforeFunc = scriptSource.substr(0, script.sourceStart);
> > +        let lastNewline = scriptBeforeFunc.lastIndexOf("\n");
> > +        let lastSemicolon = scriptBeforeFunc.lastIndexOf(";");
> > +        let lastClosingBrace = scriptBeforeFunc.lastIndexOf("\n");
>
> That's not the lastClosingBrace!
>

Ouch, fixed.

> @@ +398,5 @@
> > +        let scriptBeforeFunc = scriptSource.substr(0, script.sourceStart);
> > +        let lastNewline = scriptBeforeFunc.lastIndexOf("\n");
> > +        let lastSemicolon = scriptBeforeFunc.lastIndexOf(";");
> > +        let lastClosingBrace = scriptBeforeFunc.lastIndexOf("\n");
> > +        let lastEnding = Math.max(lastNewline, lastSemicolon, lastClosingBrace);
>

scriptSource is the whole script and scriptSource.substr(script.sourceStart, script.sourceLength) returns the handler, something like:
() {
  doSomething();
}

So we need to work *back* to the preceeding \n, ; or } so we can get the appropriate function name etc. e.g.:
() => {
  doSomething();
}

function doit() {
  doSomething();
}

doit: function() {
  doSomething();
}

doit: function dt_doit() {
  doSomething();
}

> When is a correctly defined lastClosingBrace alone not good enough?

Then we could turn out with:
let x=1,y=2;doit: function() {
  doSomething();
}

We only want the handler.

> Or, better why isn't trim() what we want here?
>

Trim only trims whitespace. We do use that later in the function and then we beautify when we show the script in the tooltip.

Anyhow, I have added comments to explain what this code is doing.

> @@ +417,5 @@
> > +
> > +        let line = script.startLine;
> > +        let url = script.url;
> > +        let origin = url + (dom0 ? "" : ":" + line);
> > +        let searchString = "";
>
> I think it makes more sense if we use undefined as a 'no value' value.
>

Agreed, done.

> @@ +436,5 @@
> > +          searchString: searchString,
> > +          lineCount: script.lineCount
> > +        });
> > +      }
> > +      if (globalDO) {
>
> we can move this inside "if (listener)" can't we, and move the definition of
> the globalDO inside too? And then we won't need to check for null.
>

Done.

> Then we could do something like "if (!listener) { return [] }" at the top
> and reduce the indent level.

Done.

Comment 27

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 2. Added semicolons, removed unused vars etc.

Rebase

Comment 28

[Joe Walker [:jwalker]]

Those changes look good to me. I'd like to ask Brian to take a quick look at the CSS.

I'm not keen on the underline for the source, and it seems strange that clicking outside the source area, but not where there is text is different from clicking on the text. How about removing the underline, and adding the event handler to the parent so you can click anywhere in the source area. I think the 'hand' cursor should be a indicator that you can click to go to the context, but how about a tooltip to be sure?

The text "load > ...foo/bar/wibble.js (Bubbling, DOM2)" feels hard to decypher to me. I wonder what we could do to give clues as to what it means?

Suggestions:

* Put the event name in bold - it's the most important bit
* Change the '>' to a '→' so you don't think it's part of a tag. Maybe we could get rid of it altogether?
* Put the filename in monospace so it looks more like source code.

I don't know how much work these are. The underlining text I think we should change, and that's easy. I'd accept follow-ups for the others. Maybe work on them until Brian is done reviewing the CSS?

Thought for a future revision: I wonder if we should have timeouts, intervals, etc along with the other global events.

Comment 29

[Brian Grinstead [:bgrins]]

Comment on attachment 8446493 [details] [diff] [review]
1. Main patch

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

Went ahead and left a few notes, but I ran into a few things when checking this out.  Here is what I did to try it out:

* Open http://getbootstrap.com/.
* Click on the (ev) button on the <html> tag

Here are a couple of things I noticed:

1) Open, close, the reopen one of the events in the tooltip.  Ends up throwing an error: "You can append an editor only once. editor.js:253"
2) Click through one of the events into the debugger.  It appears that the debugger is paused as far as how the line being highlighted.  I'm not sure if this is intentional, but I think simply putting the cursor on the position of the event handler would be fine and less confusing
3) Click on the 'message' handler and go through to the debugger.  Notice that it is on line 1.  Press prettify source from the debugger.  Go back to inspector and click through again - notice that it still goes to line 1, even though the handler is no longer on line 1.

::: browser/devtools/shared/widgets/Tooltip.js
@@ +1055,5 @@
> +      lineWrapping: false,
> +      readOnly: true,
> +      styleActiveLine: true,
> +      extraKeys: {},
> +      underlineOnHover: true,

I'm not a fan of the underlineOnHover.  I think it would be more consistent with the editor widget if clicking on the source did *not* take you to the debugger.  

My gut says the best UX for this would be to add a debugger icon [0] on the header which would link to the debugger if it were clicked on.   Then remove any custom handlers on the editor itself.

[0] chrome://browser/skin/devtools/tool-debugger.svg

@@ +1074,5 @@
> +      } else {
> +        uri = listener.origin;
> +      }
> +
> +      let heading = listener.type + " > " + uri + " (" + phase + ", " + level + ")";

Agree with Joe on this - I don't like using a greater-than here (it has too many other meetings). → would definitely be better

::: browser/themes/shared/devtools/dark-theme.css
@@ +371,5 @@
>    background-color: rgba(24, 29, 32, 1);
>    color: rgba(184, 200, 217, 1);
>  }
>  
> +.markupview-events {

Move these styles from the light-theme and dark-theme css files into the markup-view.css, but just prefixed with .theme-light / .theme-dark.

@@ +372,5 @@
>    color: rgba(184, 200, 217, 1);
>  }
>  
> +.markupview-events {
> +  background-color: #667380;

Please use the relevant colors for the dark theme from https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors.  If this was specifically done to match with another widget, then leave a comment explaining

::: browser/themes/shared/devtools/light-theme.css
@@ +376,5 @@
>  }
>  
> +.markupview-events {
> +  background-color: #667380;
> +  color: #FFF;

You can use #f5f7fa /* Selection Text Color */

::: browser/themes/shared/devtools/markup-view.css
@@ +39,5 @@
>  .tag-line {
>    padding-left: 2px;
>  }
>  
> +.markupview-events {

I don't know about using the string "ev".  Is that going to make sense localized?  Could we use the string "events", then get rid of the font-weight and border-radius props here?  Or use an icon with tooltiptext instead?

@@ +40,5 @@
>    padding-left: 2px;
>  }
>  
> +.markupview-events {
> +  font-size: 0.8em;

Could use 8px (or some px value) here to stay consistent with the units from line-height

Comment 30

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1. Main patch

Comment 31

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 2. Added semicolons, removed unused vars etc.

Rebased

Comment 32

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I forgot to add my review comments:

(In reply to Brian Grinstead [:bgrins] from comment #29)
> Comment on attachment 8446493 [details] [diff] [review]
> 1. Main patch
> 
> Review of attachment 8446493 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Went ahead and left a few notes, but I ran into a few things when checking
> this out.  Here is what I did to try it out:
> 
> * Open http://getbootstrap.com/.
> * Click on the (ev) button on the <html> tag
> 
> Here are a couple of things I noticed:
> 
> 1) Open, close, the reopen one of the events in the tooltip.  Ends up
> throwing an error: "You can append an editor only once. editor.js:253"

Fixed

> 2) Click through one of the events into the debugger.  It appears that the
> debugger is paused as far as how the line being highlighted.  I'm not sure
> if this is intentional, but I think simply putting the cursor on the
> position of the event handler would be fine and less confusing

Fixed

> 3) Click on the 'message' handler and go through to the debugger.  Notice
> that it is on line 1.  Press prettify source from the debugger.  Go back to
> inspector and click through again - notice that it still goes to line 1,
> even though the handler is no longer on line 1.
> 

Bug 1033331 logged.

> ::: browser/devtools/shared/widgets/Tooltip.js
> @@ +1055,5 @@
> > +      lineWrapping: false,
> > +      readOnly: true,
> > +      styleActiveLine: true,
> > +      extraKeys: {},
> > +      underlineOnHover: true,
> 
> I'm not a fan of the underlineOnHover.  I think it would be more consistent
> with the editor widget if clicking on the source did *not* take you to the
> debugger.  
> 
> My gut says the best UX for this would be to add a debugger icon [0] on the
> header which would link to the debugger if it were clicked on.   Then remove
> any custom handlers on the editor itself.
> 
> [0] chrome://browser/skin/devtools/tool-debugger.svg
> 

Done

> @@ +1074,5 @@
> > +      } else {
> > +        uri = listener.origin;
> > +      }
> > +
> > +      let heading = listener.type + " > " + uri + " (" + phase + ", " + level + ")";
> 
> Agree with Joe on this - I don't like using a greater-than here (it has too
> many other meetings). → would definitely be better
> 

Yeah, I have tidied that all up.

> ::: browser/themes/shared/devtools/dark-theme.css
> @@ +371,5 @@
> >    background-color: rgba(24, 29, 32, 1);
> >    color: rgba(184, 200, 217, 1);
> >  }
> >  
> > +.markupview-events {
> 
> Move these styles from the light-theme and dark-theme css files into the
> markup-view.css, but just prefixed with .theme-light / .theme-dark.
> 

Done

> @@ +372,5 @@
> >    color: rgba(184, 200, 217, 1);
> >  }
> >  
> > +.markupview-events {
> > +  background-color: #667380;
> 
> Please use the relevant colors for the dark theme from
> https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors.  If this was
> specifically done to match with another widget, then leave a comment
> explaining
> 

Done

> ::: browser/themes/shared/devtools/light-theme.css
> @@ +376,5 @@
> >  }
> >  
> > +.markupview-events {
> > +  background-color: #667380;
> > +  color: #FFF;
> 
> You can use #f5f7fa /* Selection Text Color */
> 

I did

> ::: browser/themes/shared/devtools/markup-view.css
> @@ +39,5 @@
> >  .tag-line {
> >    padding-left: 2px;
> >  }
> >  
> > +.markupview-events {
> 
> I don't know about using the string "ev".  Is that going to make sense
> localized?  Could we use the string "events", then get rid of the
> font-weight and border-radius props here?  Or use an icon with tooltiptext
> instead?
> 

Speaking to a few non English speakers they seem to think ev is fine in this context.

> @@ +40,5 @@
> >    padding-left: 2px;
> >  }
> >  
> > +.markupview-events {
> > +  font-size: 0.8em;
> 
> Could use 8px (or some px value) here to stay consistent with the units from
> line-height

Done

Comment 33

[Brian Grinstead [:bgrins]]

Comment on attachment 8449460 [details] [diff] [review]
2. Added semicolons, removed unused vars etc.

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

::: browser/themes/shared/devtools/common.css
@@ +172,5 @@
>  .devtools-tooltip[clamped-dimensions] .panel-arrowcontent,
>  .devtools-tooltip[clamped-dimensions-no-min-height] .panel-arrowcontent {
>    overflow: hidden;
>  }
> +.devtools-debugger-link {

Is this supposed to be added?  I don't see this class anywhere else in the patch.

Comment 34

[Brian Grinstead [:bgrins]]

Comment on attachment 8449459 [details] [diff] [review]
1. Main patch

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

Am I supposed to be reviewing the CSS only?

::: browser/devtools/inspector/inspector.css
@@ +28,5 @@
>  }
> +
> +/* Event tooltips */
> +
> +.theme-dark .event-tooltip-attributes {

All of these changes should be in themes/shared/devtools/inspector.css instead of here.

@@ +34,5 @@
> +  color: #343C45;
> +}
> +
> +.theme-light .event-tooltip-debugger-icon {
> +  filter: url(chrome://browser/skin/devtools/filters.svg#invert);

Please move this selector into the big list of filtered icons in toolbars.inc.css

::: browser/devtools/shared/widgets/Tooltip.js
@@ +1159,5 @@
> +      let {editor, handler, doc, lineCount} = eventEditors;
> +
> +      // This is an ugly hack to set the proper height of Codemirror's editor.
> +      // Without this we have issues involving XUL panel sizing.
> +      let style = doc.ownerGlobal.getComputedStyle(content);

What kind of issues do you run into without this?

Could we just set the min height on the iframe to something like 14px and the max height to something like 112px?

I haven't tried any alternatives to this hack, but at the very least it seems that some of this should be moved into editor.js via a getLineHeight() function to make this be less strongly tied to the DOM of codemirror

@@ +1172,5 @@
> +      iframe.setAttribute("style", "width:100%; height: " + height);
> +      // End of ugly hack.
> +
> +      editor.appendTo(content, iframe).then(() => {
> +        let tidied = js_beautify(handler, { indent_size: 2 });

Why would we use this and not pretty-fast?

::: browser/themes/shared/devtools/common.css
@@ +220,5 @@
> +  margin: -4px; /* Compensate for the .panel-arrowcontent padding. */
> +  max-width: 390px;
> +}
> +
> +.event-header {

This file is loaded in browser.xul - we make sure every selector starts with something using a devtools prefix just to be good neighbors

@@ +238,5 @@
> +  font-size: 12px;
> +}
> +
> +.event-tooltip-filename {
> +  font-family: monospace;

I'd rather see this element get a .devtools-monospace class than have monospace set directly here so that the font matches on each platform.  You may have to add font-size: 100% to make sure it doesn't shrink on linux (see the devtools-monospace class above in this file)

@@ +243,5 @@
> +  -moz-margin-end: 0;
> +  margin-top: 6px;
> +}
> +
> +.event-tooltip-attributes {

These elements vertical alignment is not right.  The text is too close to the bottom of the label.

::: browser/themes/shared/devtools/markup-view.css
@@ +94,5 @@
> +  font-weight: bold;
> +  line-height: 10px;
> +  border-radius: 50%;
> +  padding: 0px 2px;
> +  -moz-margin-start: 5px;

-moz-user-select: none;

Comment 35

[Joe Walker [:jwalker]]

(In reply to Brian Grinstead [:bgrins] from comment #34)
> Comment on attachment 8449459 [details] [diff] [review]
> 1. Main patch
> 
> Review of attachment 8449459 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Am I supposed to be reviewing the CSS only?

That's all I was asking for.

Comment 36

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1. Main patch

(In reply to Brian Grinstead [:bgrins] from comment #34)
> Comment on attachment 8449459 [details] [diff] [review]
> 1. Main patch
> 
> Review of attachment 8449459 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Am I supposed to be reviewing the CSS only?
> 
> ::: browser/devtools/inspector/inspector.css
> @@ +28,5 @@
> >  }
> > +
> > +/* Event tooltips */
> > +
> > +.theme-dark .event-tooltip-attributes {
> 
> All of these changes should be in themes/shared/devtools/inspector.css
> instead of here.
> 

Moved

> @@ +34,5 @@
> > +  color: #343C45;
> > +}
> > +
> > +.theme-light .event-tooltip-debugger-icon {
> > +  filter: url(chrome://browser/skin/devtools/filters.svg#invert);
> 
> Please move this selector into the big list of filtered icons in
> toolbars.inc.css
> 

Done

> ::: browser/devtools/shared/widgets/Tooltip.js
> @@ +1159,5 @@
> > +      let {editor, handler, doc, lineCount} = eventEditors;
> > +
> > +      // This is an ugly hack to set the proper height of Codemirror's editor.
> > +      // Without this we have issues involving XUL panel sizing.
> > +      let style = doc.ownerGlobal.getComputedStyle(content);
> 
> What kind of issues do you run into without this?
> 
> Could we just set the min height on the iframe to something like 14px and
> the max height to something like 112px?
> 
> I haven't tried any alternatives to this hack, but at the very least it
> seems that some of this should be moved into editor.js via a getLineHeight()
> function to make this be less strongly tied to the DOM of codemirror
> 

Seems that since moving to CodeMirror we no longer need this hack.

Removed.

> @@ +1172,5 @@
> > +      iframe.setAttribute("style", "width:100%; height: " + height);
> > +      // End of ugly hack.
> > +
> > +      editor.appendTo(content, iframe).then(() => {
> > +        let tidied = js_beautify(handler, { indent_size: 2 });
> 
> Why would we use this and not pretty-fast?
> 

Because the handler is just a section of a script and may not be valid JS e.g. myFunc: function() { ... }.

pretty-fast can only process valid scripts whereas JSBeautify can process even partially written scripts.

> ::: browser/themes/shared/devtools/common.css
> @@ +220,5 @@
> > +  margin: -4px; /* Compensate for the .panel-arrowcontent padding. */
> > +  max-width: 390px;
> > +}
> > +
> > +.event-header {
> 
> This file is loaded in browser.xul - we make sure every selector starts with
> something using a devtools prefix just to be good neighbors
> 

Yup, fixed.

> @@ +238,5 @@
> > +  font-size: 12px;
> > +}
> > +
> > +.event-tooltip-filename {
> > +  font-family: monospace;
> 
> I'd rather see this element get a .devtools-monospace class than have
> monospace set directly here so that the font matches on each platform.  You
> may have to add font-size: 100% to make sure it doesn't shrink on linux (see
> the devtools-monospace class above in this file)
> 

Done

> @@ +243,5 @@
> > +  -moz-margin-end: 0;
> > +  margin-top: 6px;
> > +}
> > +
> > +.event-tooltip-attributes {
> 
> These elements vertical alignment is not right.  The text is too close to
> the bottom of the label.
> 

They look fine to me, I will add an attachment for you to look at.

> ::: browser/themes/shared/devtools/markup-view.css
> @@ +94,5 @@
> > +  font-weight: bold;
> > +  line-height: 10px;
> > +  border-radius: 50%;
> > +  padding: 0px 2px;
> > +  -moz-margin-start: 5px;
> 
> -moz-user-select: none;

Added.

Comment 37

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: attributes-misaligned.png

bgrins: Does it look misaligned to you?

Comment 38

[Brian Grinstead [:bgrins]]

Attachment: attributes-misaligned-2.png

> bgrins: Does it look misaligned to you?

Here is what it looks like to me

Comment 39

[Brian Grinstead [:bgrins]]

Comment on attachment 8451707 [details] [diff] [review]
1. Main patch

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

Looking good - a few notes and there is one more issue with sizing on the tooltip when expanding a bunch of events. I'm not sure how easy it will be to work around, but I will attach a gif.

::: browser/themes/shared/devtools/common.css
@@ +215,5 @@
>  }
>  
> +/* Tooltip: Events */
> +
> +#devtools-tooltip-events-container {

I believe that all of these rules can (and should) be moved into themes/shared/devtools/inspector.css.  The #devtools-tooltip-events-container prefix probably won't be required on all of the selectors anymore.  I missed it the first time through, but if we have a way to *not* include something in common.css, we probably should do so.

@@ +251,5 @@
> +  -moz-margin-start: 5px;
> +  font-size: 9px;
> +}
> +
> +#devtools-tooltip-events-container .event-tooltip-debugger-icon {

I'd prefer to see the debugger button icon on the far right or the far left of the tooltip, so that if there are a bunch in a row all of the clickable buttons line up (right now, they are misaligned if you have different types of event binding lined up - see the <html> element on http://getbootstrap.com/ for an example).

::: browser/themes/shared/devtools/markup-view.css
@@ +92,5 @@
> +.markupview-events {
> +  font-size: 8px;
> +  font-weight: bold;
> +  line-height: 10px;
> +  border-radius: 50%;

IMO it looks better with a smaller border radius - something like 3px to be consistent with .event-tooltip-attributes.  But I'll leave that up to you

Comment 40

[Brian Grinstead [:bgrins]]

Attachment: events-expanding.gif

What I see when expanding multiple events - it seems to run out of space and doesn't allow scrolling on the tooltip to see the overflow.  It may be tricky to negotiate because the editors themselves allow scrolling, but it seems that right now it is hiding all of the overflow.

Comment 41

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1. Main patch

(In reply to Brian Grinstead [:bgrins] from comment #39)
> Comment on attachment 8451707 [details] [diff] [review]
> 1. Main patch
> 
> Review of attachment 8451707 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good - a few notes and there is one more issue with sizing on the
> tooltip when expanding a bunch of events. I'm not sure how easy it will be
> to work around, but I will attach a gif.
> 
> Created attachment 8451897 [details]
> events-expanding.gif
> 
> What I see when expanding multiple events - it seems to run out of space and
> doesn't allow scrolling on the tooltip to see the overflow.  It may be
> tricky to negotiate because the editors themselves allow scrolling, but it
> seems that right now it is hiding all of the overflow.
> 

OSX and Linux both use the mouse position to choose the scroll target so we can't add a scrollbar to the panel.

To work around this we now have use accordian style expand / collapse. This is a far better UX when working with multiple events.

> ::: browser/themes/shared/devtools/common.css
> @@ +215,5 @@
> >  }
> >  
> > +/* Tooltip: Events */
> > +
> > +#devtools-tooltip-events-container {
> 
> I believe that all of these rules can (and should) be moved into
> themes/shared/devtools/inspector.css.

Done

> The
> #devtools-tooltip-events-container prefix probably won't be required on all
> of the selectors anymore.

Removed

> I missed it the first time through, but if we
> have a way to *not* include something in common.css, we probably should do
> so.
> 

/me nods head.

> @@ +251,5 @@
> > +  -moz-margin-start: 5px;
> > +  font-size: 9px;
> > +}
> > +
> > +#devtools-tooltip-events-container .event-tooltip-debugger-icon {
> 
> I'd prefer to see the debugger button icon on the far right or the far left
> of the tooltip, so that if there are a bunch in a row all of the clickable
> buttons line up (right now, they are misaligned if you have different types
> of event binding lined up - see the <html> element on
> http://getbootstrap.com/ for an example).
> 

Good spot. The far left makes the most sense from a UX point of view so we will put it there.

> ::: browser/themes/shared/devtools/markup-view.css
> @@ +92,5 @@
> > +.markupview-events {
> > +  font-size: 8px;
> > +  font-weight: bold;
> > +  line-height: 10px;
> > +  border-radius: 50%;
> 
> IMO it looks better with a smaller border radius - something like 3px to be
> consistent with .event-tooltip-attributes.  But I'll leave that up to you

Agreed, fixed.

Comment 42

[Girish Sharma [:Optimizer]]

Are there any plans to move this from the tooltip to the sidebar ? I think since the amount of information (multiple event listeners plus the handler methods' source) is too much to be in a tooltip.

If the data is in the sidebar, then it can be easily interacted with and no positioning or resizing issues will appear too.

Comment 43

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Girish Sharma [:Optimizer] from comment #42)
> Are there any plans to move this from the tooltip to the sidebar ? I think
> since the amount of information (multiple event listeners plus the handler
> methods' source) is too much to be in a tooltip.
> 
> If the data is in the sidebar, then it can be easily interacted with and no
> positioning or resizing issues will appear too.

The same API could easily be used for a page overlay or sidebar. The problem is that to get the detailed information we need to start the debugger and that is costly. Displaying a list and only populating the contents when they are expanded would be very easy though.

Other browsers do have this but I think if we add it we should have an option to enable / disable sidebar items.

You are very welcome to log a bug.

Comment 44

[Brian Grinstead [:bgrins]]

Comment on attachment 8453006 [details] [diff] [review]
1. Main patch

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

The frontend changes look fine, r+ for those.  I've added a suggestion you can take or leave with one way of how to handle the vertical alignment issue on 'capturing/bubbling/DOM 0' attributes.

FYI I'm seeing an error on google.com when opening the popup on the <html> element -

TypeError: script is undefined
Stack trace:
exports.NodeActor<.getEventListeners@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:396:11
exports.NodeActor<.getEventListenerInfo<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:515:1
actorProto/</handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:943:19
DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1170:15
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/transport/transport.js:545:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14
 protocol.js:854

::: browser/themes/shared/devtools/inspector.css
@@ +54,5 @@
> +.event-tooltip-filename,
> +.event-tooltip-attributes {
> +  -moz-margin-start: 0;
> +  cursor: pointer;
> +  margin-top: 5px;

I'm still having some text alignment issues on the attributes (see Attachment 8451879 [details]).  This layout could probably be reorganized with a `display: flex; align-items: center;` on the .event-header, then setting `flex-shrink: 0` on everything except for the event-tooltip-filename.  In this case most of the hardcoded margins and paddings should be able to be dropped.  Anyway, I'll leave it up to you, this could be handled in a follow up if you'd like

Comment 45

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1. Main patch

Addressed review comments and made test e10s compatible. The test also had to be disabled in e10s mode due to leaks caused by a failing CodeMirror editor.destroy() method.

Comment 46

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

https://hg.mozilla.org/integration/fx-team/rev/89669f18bd2d

Comment 47

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8458666 [details] [diff] [review]
1. Main patch

Backed out https://hg.mozilla.org/integration/fx-team/rev/04fc2abf8ac6

Try was green so we must have a conflict.

Comment 48

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: visual-events-736078.patch

Disabled test due to over 40 seconds of GC after test starts causing task, and eventually, test timeouts (see bug 1041284).

Comment 49

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8459284 [details] [diff] [review]
visual-events-736078.patch

https://hg.mozilla.org/integration/fx-team/rev/f659f3d98472

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f659f3d98472

Comment 51

[Sylvestre Ledru [:Sylvestre]]

Added to the release notes: "Developer Tools: Display which elements have listeners attached"

Don't hesitate if you have a better wording.