871630 - Add the collapse/expand button to the Console toolbar to show/hide the VariablesView

Status: RESOLVED INVALID

(DevTools :: Console, defect, P3)

Description

[Rob Campbell [:rc] (:robcee)]

we should have the control from Debugger and Netmonitor to collapse and expand the sidebar.

Comment 1

[Rob Campbell [:rc] (:robcee)]

Attachment: Add the collapse/expand button to the Console toolbar to show/hide the VariablesView

Comment 2

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

\o/

Comment 3

[Rob Campbell [:rc] (:robcee)]

Attachment: Add the collapse/expand button to the Console toolbar to show/hide the VariablesView

Comment 4

[Mihai Sucan [:msucan]]

Comment on attachment 748984 [details] [diff] [review]
Add the collapse/expand button to the Console toolbar to show/hide the VariablesView

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

Thank you for the patch.

One problem: the toggle button doesn't work as a toggle. After close it doesn't reopen. Also, shouldn't we make the button disabled until the first sidebar open? Or something...

One nit: the test does more than needed. It could be simpler: only execute inspect(window), wait for variablesview-fetched, click the collapse sidebar button and wait for the sidebar-closed event.

::: browser/devtools/webconsole/test/browser_bug_871630_variable_view_close_on_button.js
@@ +2,5 @@
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +// Check that the variables view sidebar can be closed by pressing Escape in the

Escape?

@@ +91,5 @@
> +
> +function onFooFound(aResults)
> +{
> +  gJSTerm.once("sidebar-closed", finishTest);
> +  let butten = gWebConsole.ui.rootElement.querySelector("#instruments-pane-toggle");

butten? Nice name. :)

::: browser/devtools/webconsole/webconsole.js
@@ +490,5 @@
>        this.jsterm.clearOutput(true);
>      }.bind(this));
>  
> +    let collapseButton = doc.getElementById("instruments-pane-toggle");
> +    collapseButton.addEventListener("mousedown", () => { this.jsterm._sidebarDestroy(); }, false);

I think we prefer "click" here. Most UIs react to clicks - not when you first press any mouse button.

Comment 5

[Rob Campbell [:rc] (:robcee)]

(In reply to Mihai Sucan [:msucan] from comment #4)
> Comment on attachment 748984 [details] [diff] [review]
...

+    let collapseButton = doc.getElementById("instruments-pane-toggle");
> > +    collapseButton.addEventListener("mousedown", () => { this.jsterm._sidebarDestroy(); }, false);
> 
> I think we prefer "click" here. Most UIs react to clicks - not when you
> first press any mouse button.

the other instances of this button use "mousedown" for some reason. Victor?

Comment 6

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #5)
> 
> the other instances of this button use "mousedown" for some reason. Victor?

Debugger is (used to be?) a special case because certain actions used to take longer to finish (for various reasons, like source editor being slow etc.) and mousedown made everything feel snappier in certain places. This was especially the case with the sidebar at some point, however I don't think it's still mandatory.

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

We don't have the variable view anymore on the console (there is a sidebar, and it has a close button).