865871 - Object inspector can't be closed

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Ian Bicking (:ianbicking)]

Attachment: Screenshot of inspector

As far as I can see, there's no way to close the object inspector in the console.  Once you've opened the inspector, it just sits there until you close and reopen the web console.  It can hide functionality as shown in the screenshot.

Comment 1

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

I would dupe this to bug 865788, but I'm not sure if we want one bug per tool or not. Leaving it as it is until someone who knows whether we can define the keybinding in VariablesView or not can comment.

Comment 2

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

(In reply to Panos Astithas [:past] from comment #1)
> I would dupe this to bug 865788, but I'm not sure if we want one bug per
> tool or not. Leaving it as it is until someone who knows whether we can
> define the keybinding in VariablesView or not can comment.

Quick note: the VariablesView has no idea about it's owner, nor how it's contained, so I don't think it should know how to close itself, since this responsibility is different for each tool.

When it comes to keybindings, there's no way of defining custom actions for a VariablesView at this point. It'd make for a nifty API, so I suggest opening a bug about it.

Comment 3

[Mihai Sucan [:msucan]]

Thanks for the bug report. This issue is known, see bug 843280. However, that one is about a generic way to collapse sidebars.

The ESC idea is fine and we can do this in the web console.

Comment 4

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Added an event handler for keypress that listens for the Escape key to close the sidebar, and a test that checks this works as expected. I also cleaned-up small bits of code.

Please let me know if you have any concerns. Thanks!

Try push: https://tbpl.mozilla.org/?tree=Try&rev=0653265b236b

Comment 5

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

Comment on attachment 744220 [details] [diff] [review]
proposed patch

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

It's an improvement, to be sure, but I think we could do better than this. I'm not going to block you if you disagree though.

::: browser/devtools/webconsole/webconsole.js
@@ +3120,5 @@
> +  {
> +    let tag = aEvent.target.nodeName;
> +    if (aEvent.keyCode != Ci.nsIDOMKeyEvent.DOM_VK_ESCAPE || aEvent.shiftKey ||
> +        aEvent.altKey || aEvent.ctrlKey || aEvent.metaKey ||
> +        ["input", "textarea", "select", "textbox"].indexOf(tag) > -1) {

This seems to be too strict in practice. The 2 most common cases for me are:
1. inspect(foo), glance at the sidebar, press ESC
2. console.log(foo), click on [object Foo], glance at the sidebar, press ESC
In neither case the sidebar is hidden, because the focus is on one of these ignored elements.

If the reason for this limit is the case of pressing ESC to dismiss the autocomplete menu, then maybe handling that case explicitly would be better?

Comment 6

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #5)
> Comment on attachment 744220 [details] [diff] [review]
> proposed patch
> 
> Review of attachment 744220 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It's an improvement, to be sure, but I think we could do better than this.
> I'm not going to block you if you disagree though.
> 
> ::: browser/devtools/webconsole/webconsole.js
> @@ +3120,5 @@
> > +  {
> > +    let tag = aEvent.target.nodeName;
> > +    if (aEvent.keyCode != Ci.nsIDOMKeyEvent.DOM_VK_ESCAPE || aEvent.shiftKey ||
> > +        aEvent.altKey || aEvent.ctrlKey || aEvent.metaKey ||
> > +        ["input", "textarea", "select", "textbox"].indexOf(tag) > -1) {
> 
> This seems to be too strict in practice. The 2 most common cases for me are:
> 1. inspect(foo), glance at the sidebar, press ESC
> 2. console.log(foo), click on [object Foo], glance at the sidebar, press ESC
> In neither case the sidebar is hidden, because the focus is on one of these
> ignored elements.
> 
> If the reason for this limit is the case of pressing ESC to dismiss the
> autocomplete menu, then maybe handling that case explicitly would be better?

Good idea. I didn't know if it yesterday. I have updated the patch accordingly.

After I submitted the patch, yesterday, I had the idea of autofocusing vview when you click objects (only then!), also to put focus back in jsterm inputNode after you press Escape in vview. Together with your idea, I think the updated patch nicely improves usability.

Thanks!

Landed:

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

Comment 7

[Mihai Sucan [:msucan]]

Attachment: updated patch

Comment 8

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

Love it, thanks!

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/4d4f26313ab2