972404 - Browser Toolbox: Add a mouse shortcut for 'inspect element' on chrome content -> shift+right click

Status: NEW

(DevTools :: Framework, defect, P3)

Description

[Brian Grinstead [:bgrins]]

Just putting this here so I don't forget.  It would be wonderful if I were to have a context menu item handy when right clicking on chrome elements that would open up the Browser Toolbox and select said element (if I have enabled chrome/remote debugging).

This would be helpful when doing frontend changes to browser content.  The current workflow is Tools->Web Developer->Browser Toolbox->Inspector->Click to select element->Click element on page.

I am using Inspect Context (https://addons.mozilla.org/en-US/firefox/addon/inspect-context) to get similar functionality with the DOM Inspector addon.

Comment 2

[Brian Grinstead [:bgrins]]

After trying out the "Element Inspector" addon (https://addons.mozilla.org/en-US/firefox/addon/element-inspector), I realize that I prefer that ux (shift+right click) to adding a context menu.  This makes it very easy to open elements that don't usually have context menus associated with them.

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: shift-right-click-WIP.patch

This still needs some work, but it adds a shortcut when the BT is opened

Comment 4

[Brian Grinstead [:bgrins]]

Discussed with dmose about having this shortcut also automatically remove the 'autohide' attribute on a xul panel if it was the target of the click, making it much easier to work with these elements that automatically disappear when the window loses focus.

Comment 5

[Brian Grinstead [:bgrins]]

Attachment: shift-right-click.patch

Here's my tentative plan:

1 Bind a mousedown / contextmenu listener on the browser in gDevTools browser depending on whether remote debugging is enabled
2 Open the Browser Toolbox (or get a reference to the currently opened one)
3 Using the loader, call a function in the inspector module
4 Fire up the Inspector Front immediately on toolbox startup (this shouldn't be costly - I'm still waiting to fire up the Walker until something else requests it)
5 Detect calls from 3 by the Inspector Actor (which works b/c the module is running in the same process as the actors)
6 Send an event from the Inspector actor to the toolbox, and inspect the node

This *doesn't* cover the initial shift+right click.  I tried doing some things like firing the event immediately upon the Inspector Actor starting up, but that seemed to run into some race conditions with selectTool/loadTool in the toolbox, among other problems.

Let me know if you think this all seems generally sane and if you have any ideas how to handle the initial inspection

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8613151 [details] [diff] [review]
shift-right-click.patch

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

Overall, I think it seems good, but could be combined with the regular tab path.

It seems like we should be able to get the event working on actor startup.  What was the precise race issue you saw?  If it just calls the same picked handler, shouldn't it properly wait for the tool to start?

The hacky version to do this before BT is loaded would be to:

1. Add another URL arg to BT when created[1]
2. Grab the arg in toolbox-process-window[2] to do whatever you need

The hacky approach feels really silly to me, since we already build a communication channel with our actors.  We just need to make them do what we want here.

[1]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/ToolboxProcess.jsm#203
[2]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox-process-window.js#36

::: toolkit/devtools/server/actors/inspector.js
@@ +187,5 @@
>  exports.setInspectingNode = function(val) {
>    gInspectingNode = val;
>  };
>  
> +exports.setInspectingNodeBrowserToolbox = function(val) {

Instead of adding a special path for BT, could we take this event approach for both BT and regular tabs?

It seems like it should work for both without adding a special case.

@@ +189,5 @@
>  };
>  
> +exports.setInspectingNodeBrowserToolbox = function(val) {
> +  gInspectingNode = val;
> +  events.emit(exports.setInspectingNodeBrowserToolbox, "called");

As noted elsewhere, I think we only need one path, not a separate on for BT.

I find it a bit odd to emit on this function...  Maybe this could be moved to be a static / class function on |WalkerActor|, the only thing that uses |gInspectingNode|.  Something like:

WalkerActor.setInspectingNode = function(node) {
  WalkerActor.inspectingNode = node;
  events.emit(WalkerActor, "inspecting-node");
};

Mainly just a style point to avoid a global.

Comment 7

[Brian Grinstead [:bgrins]]

Unassigning - not going to work on this in the near future.  Also, this might become a lot simpler if bug 1246715 works

Comment 8

[Sebastian Zartner [:sebo]]

With the Browser Toolbox enabled, I'd expect to also get a context menu option for this like we have it for the web content.
May this bug also cover that or should I file a new one for it?

Sebastian

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Sebastian Zartner [:sebo] from comment #8)
> With the Browser Toolbox enabled, I'd expect to also get a context menu
> option for this like we have it for the web content.
> May this bug also cover that or should I file a new one for it?

I prefered a shortcut because it makes it easier to select things that go away when they lose focus or do not allow right clicking on.  i.e. if some kind of popup is opened you'd be unable to right click on that popup to show the 'inspect element' context menu item.  This is from experience with DOMi and using two different extensions, one that added a context menu item and one that used a keyboard shortcut + right click - and I found the latter more helpful personally.

Comment 10

[Sebastian Zartner [:sebo]]

Ok, so I'll file a new issue and we can discuss it there.

Sebastian