862558 - (split-console) Web Console should always be available / visible

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

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

Attachment: patch WIP

The console is the go-to place for messages and errors and to enter and evaluate quick bits of JS. As such, it's often destructive to have to switch panels to go to the Web Console from the Inspector, the Debugger, or wherever.

We should make the console a special entity in the Toolbox.

Comment 1

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

Attachment: screenshot OS X

A very rough first draft. I think the console would look better on the bottom or possibly in a left-side-bar when the toolbox is on the bottom. Button needs to have a checked state when toggled on. Opening the console via the keyboard shortcut breaks things, etc. Needs work!

Comment 2

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

woooot! \o/

Comment 3

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

Mmmmmmm!
Maybe also move the web console icon to the right, along with the other togglable tool icons?

Comment 4

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

(In reply to Panos Astithas [:past] from comment #3)
> Mmmmmmm!
> Maybe also move the web console icon to the right, along with the other
> togglable tool icons?

I think the console's more important than that position implies. It's not just a "bonus feature".

Comment 5

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #4)
> 
> I think the console's more important than that position implies. It's not
> just a "bonus feature".

I also think it should be on the left, but it look like an ordinary tab (with borders and everything) to make it more obvious that "this is a tool, not a toggle". Just adding toolbox-tab devtools-tab classes should probably do the trick.

Comment 6

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

(In reply to Victor Porof [:vp] from comment #5)
s/but it look/but look/

Comment 7

[Mihai Sucan [:msucan]]

This looks good. I like how nice and simple the patch is.

Suggestions:

- why not let the web console tool button visible as it was? Just add the new icon alongside Scratchpad.

  - or add a new more-obvious icon in the toolbox tab. I think the JS console needs to be more obvious than Scratchpad and friends. (due to importance)

I like that Web Inspector keeps the Console as a first-class tool even if it has a toggle for showing the console while you are in any tool.

- make the web console 'float' above the others? Currently the webconsole iframe is placed above the toolbox... a lot of space is being used.

  - or split the toolbox height between the current tool and the web console itself, such that the overall toolbox height doesn't change.

This is what Chrome's web inspector does. Dragonfly shows a floating console above the tools.

- add some logic in webconsole.js to tell it is "floating" mode. In floating mode you can hide the web console filters toolbar and perhaps we can turn of net logging temporarily. We can show net logs only when you switch back to the normal web console.

Thanks for your work on this! Please let me know if I can help with anything.

Comment 8

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

Attachment: WIP 2

Moved the panel to the bottom of the toolbox. Added better button handling.

Comment 9

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

(In reply to Mihai Sucan [:msucan] from comment #7)
> Suggestions:
> 
> - why not let the web console tool button visible as it was? Just add the
> new icon alongside Scratchpad.

nope. Gotta stay on the left. See comment #4.

>   - or add a new more-obvious icon in the toolbox tab. I think the JS
> console needs to be more obvious than Scratchpad and friends. (due to
> importance)
> 
> I like that Web Inspector keeps the Console as a first-class tool even if it
> has a toggle for showing the console while you are in any tool.

Yes. It is a first-class tool.

> - make the web console 'float' above the others? Currently the webconsole
> iframe is placed above the toolbox... a lot of space is being used.

Ugh. No more extra positioning options! Do you remember the panels?

>   - or split the toolbox height between the current tool and the web console
> itself, such that the overall toolbox height doesn't change.

overall toolbox height doesn't change. This just splits the available space in two.

> This is what Chrome's web inspector does. Dragonfly shows a floating console
> above the tools.
> 
> - add some logic in webconsole.js to tell it is "floating" mode. In floating
> mode you can hide the web console filters toolbar and perhaps we can turn of
> net logging temporarily. We can show net logs only when you switch back to
> the normal web console.

I think this is a separate thing. I want to create some bugs to provide a more focused JS entry mode than we have now. We'll have to do some work to figure out what that means still.

Comment 10

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

Attachment: WIP 3

more button fiddling.

Comment 12

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

copying this here for reference:

http://people.mozilla.com/~shorlander/files/devtools/creation/DevTools-BuildTab-01-%28Projects%29.png

Comment 13

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

Attachment: Web Console should always be available / visible

Comment 15

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

dropping this since we're exploring different REPL ideas.

Comment 18

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

reassigning myself to this. Going to try it again.

Comment 19

[Brian Grinstead [:bgrins]]

Attachment: split-console-WIP1.patch

I've come up with an approach that I think will work.  We attach the webconsole to a special box in toolbox.xul, then basically show/hide the splitter and the above deck depending on the state.

Still need to clean up the code a little bit, add a button, and add some limits to the UI on the splitter, but if you open it up you can toggle display by pressing escape.

Comment 20

[Brian Grinstead [:bgrins]]

Attachment: splitconsole-wip1.png

A screenshot of the console when it is split.  There is a slider to change the height, and if you are on the console tab it looks like it usually does.

Comment 21

[Brian Grinstead [:bgrins]]

Attachment: split-console-WIP2.patch

Updated the code and added comments, also waits to load the console until requested instead of loading it right away.

Still need to do some analysis of if there are places where the escape key has another meaning - need to make sure that the key event's propagation is still being stopped.  Checked the inplace editor on the inspector, and it seems fine.

Comment 22

[Brian Grinstead [:bgrins]]

Attachment: split-console-WIP3.patch

Changes from WIP2: Hides split console and splitter from start in XUL, fixes typo, adds initial test.

Rob or Mihai, I'd like some feedback about this general approach I'm taking here before I spend too much more time on it.

Comment 23

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

that's pretty much the approach I was going towards, with the difference in my second attempt I was going to use a horizontal layout when docked at the bottom (and then switch directions when docked on the side).

Should be easy to flip that. I find the vertical split takes up too much space and is kind of wasteful, though neither approach is ideal really. E.g., when using the split console in the debugger, would be nice to only have the one variables view.

Comment 24

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

Comment on attachment 828908 [details] [diff] [review]
split-console-WIP3.patch

This looks good.

I can see the splitter when the Console's the selected tool at the top (an extra black pixel border and the mouse cursor changes on hover).

Might be worth trying as victor suggested and hiding the toolbar with just JS and Logging options enabled when in split mode.

Comment 25

[Brian Grinstead [:bgrins]]

Attachment: split-console-WIP4.patch

Hides splitter when console is open, adds min-height to split console and deck, fixes weirdness with debugger watch expressions by allowing stopPropagation() on escape key to cancel the event.

Comment 26

[Brian Grinstead [:bgrins]]

Attachment: split-console.patch

This patch implements the split console behavior.  It can be triggered on/off with the 'escape' key.

There are two things with the UI that this patch does *not* do:
1) It does not tell the console when it is split, meaning that the toolbar is never removed.  There is a question of too much vertical space being taken, but I think removing the toolbar adds UI complications (what if you don't have certain filters applied so you aren't seeing log statements while paused at a breakpoint, for instance).  Additionally, we have some new designs that limit the height of the toolbars (like this: https://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/[e-mail]).
2) It doesn't add any new buttons to the UI.  I think we will eventually want to show a button, but the escape key is pretty well-known for this functionality.

We can discuss either of these things further, but here is the patch without them.

Tests are passing locally, here is a try push: https://tbpl.mozilla.org/?tree=Try&rev=0b2836f01ec0

Comment 27

[Mihai Sucan [:msucan]]

Comment on attachment 830420 [details] [diff] [review]
split-console.patch

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

Patch looks good. r+ on the following grounds:

- this is really nicely working: it puts the console at the bottom of the toolbox and it splits the existing toolbox height, without pushing the page / lowering the height of the page.

- the console toolbar is far from ideal in this UI, but it's not really as bad as I expected. This is really usable.

- the benefit of having the console always visible outweighs the problems we have now for which we are undecided on solutions yet.

- the implementation you have is sufficiently clean.

I would like:

- a button for this new split console.
- a smaller console toolbar when the console is displayed at the bottom.

A button should be easy to add, to the left of the Console tab, or add the button in the group of small buttons on the right, where scratchpad is.

A smaller console toolbar only needs a toolbar class, based on which you could lower paddings for the buttons and toolbar and filter input. I think we should hide the icons on the buttons of categories (net/js/css/etc...).

If you prefer to do these in separate patches/bugs, you can do that. Please open the bugs. We should also have those patches landed in the same milestone.

Thank you!

Comment 28

[Paul Rouget [:paul]]

Comment on attachment 830420 [details] [diff] [review]
split-console.patch

For what it's worth…

Comment 29

[Paul Rouget [:paul]]

(In reply to Mihai Sucan [:msucan] from comment #27)
> - a button for this new split console.

I think we can add that in a followup (because we need an icon, and icons takes time).

> - a smaller console toolbar when the console is displayed at the bottom.

To avoid too many CSS code… I'd prefer if we don't do that. At some point, we will redesign these toolbars, and they will be smaller (bug 915414).

> A button should be easy to add, to the left of the Console tab, or add the
> button in the group of small buttons on the right, where scratchpad is.

You mean "right"?

Comment 30

[Paul Rouget [:paul]]

Does this play nicely if the console is disabled in the option panel? (I didn't check)

Comment 31

[Brian Grinstead [:bgrins]]

(In reply to Paul Rouget [:paul] from comment #30)
> Does this play nicely if the console is disabled in the option panel? (I
> didn't check)

No, it ends up throwing errors and breaking lots of things.  Good catch, that's something I didn't test for - I'll make sure that is covered.

On a related note, who would disable the console anyway?  Can we *not* allow disabling of the main set of tools?  I'm guessing there may be some code that interacts between panels that will be tricky to test for in this case.  For instance, if right clicking on a DOM node in the console opens the element in the inspector, we need to first check if the inspector exists.  If it doesn't then we need to remove the context menu item.  If the inspector is then re-added, we need to show it in the context menu again.  IMO, the standard set of tools should be always-available.  Then again, I may be missing something.

Comment 32

[Paul Rouget [:paul]]

I want to be able to hide the console tab and just use the split console.

And I agree that disabling some tools in the console might break some other tools.

Comment 33

[Brian Grinstead [:bgrins]]

(In reply to Paul Rouget [:paul] from comment #32)
> I want to be able to hide the console tab and just use the split console.
> 
> And I agree that disabling some tools in the console might break some other
> tools.

So I have a small update for the patch that will cause it to not break when the console panel is removed / added: https://hg.mozilla.org/try/rev/4a9ab5a77408.

But I don't think we should try to support the split console while the "console" is is disabled from the option panel.  Being able to keep the split console around even when the console panel has been removed complicated things quite a bit with how the toolbox and gDevTools works, and would require some bigger rewriting.  Here are a couple of the issues I see after looking into this:

* On initial page load, the console will not be part of the set because of its visibilityswitch will be false.  So when you split console, it will pull open an empty panel.  This can't be worked around by setting visibilityswitch to true on console, since then the panel will be added to the tab bar as well.
* When toolUnregistered is called it detroys the instance and removes the panel from the DOM: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.js#797.  We could keep the webconsole panel / frame around *if* the request is coming from an option change, but we should still be removing the elements if it is requested for cleanup.
* Right now the nice part about this feature is that the webconsole is a little special, but isn't *that* special compared to other tools.  Allowing it to open even when removed would require quite a few specific workarounds.

I still believe that giving people options to disable these core tools is option-overload.  It presents users with options that are unlikely to ever be used intentionally.  There are a core set of tools that we should be providing to all users, and the console is probably at the top of this list.

Comment 34

[Brian Grinstead [:bgrins]]

Instead up uploading a new patch to fix compatibility with disabled console, I am working on Bug 938172.

Comment 35

[Brian Grinstead [:bgrins]]

(In reply to Mihai Sucan [:msucan] from comment #27)
> Comment on attachment 830420 [details] [diff] [review]
> split-console.patch
> 
> Review of attachment 830420 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Patch looks good. r+ on the following grounds:
> 
> - this is really nicely working: it puts the console at the bottom of the
> toolbox and it splits the existing toolbox height, without pushing the page
> / lowering the height of the page.
> 
> - the console toolbar is far from ideal in this UI, but it's not really as
> bad as I expected. This is really usable.
> 
> - the benefit of having the console always visible outweighs the problems we
> have now for which we are undecided on solutions yet.
> 
> - the implementation you have is sufficiently clean.
> 
> I would like:
> 
> - a button for this new split console.
> - a smaller console toolbar when the console is displayed at the bottom.
> 
> A button should be easy to add, to the left of the Console tab, or add the
> button in the group of small buttons on the right, where scratchpad is.
> 
> A smaller console toolbar only needs a toolbar class, based on which you
> could lower paddings for the buttons and toolbar and filter input. I think
> we should hide the icons on the buttons of categories (net/js/css/etc...).
> 
> If you prefer to do these in separate patches/bugs, you can do that. Please
> open the bugs. We should also have those patches landed in the same
> milestone.
> 
> Thank you!

I've opened Bug 938616 to land in the same release, and will work with Darrin on these updates.

Comment 36

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/2887e3a300ed
https://tbpl.mozilla.org/?tree=Fx-Team&rev=2887e3a300ed

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in  https://hg.mozilla.org/integration/fx-team/rev/4cca85b2241d for breaking a browser-chrome test: https://tbpl.mozilla.org/php/getParsedLog.php?id=30721837&tree=Fx-Team

Comment 38

[Brian Grinstead [:bgrins]]

(In reply to Wes Kocher (:KWierso) from comment #37)
> Backed out in  https://hg.mozilla.org/integration/fx-team/rev/4cca85b2241d
> for breaking a browser-chrome test:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=30721837&tree=Fx-Team

Oops, looks like a test from Bug 889638 was failing.  Fixed and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=d1b317eb105f.

Comment 39

[Brian Grinstead [:bgrins]]

Attachment: split-console.patch

Patrick,
Can you have a look at the changes I made to Tooltip.js?  I needed to stopPropagation on the escape key, but only when there was an active swatch.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=4c49238099b2

Comment 40

[Patrick Brosset <:pbro>]

I haven't seen anything problematic in the code, but since this is mostly a UI change, I'm currently building with this patch and will test before R+ing.

Comment 41

[Patrick Brosset <:pbro>]

Comment on attachment 8334475 [details] [diff] [review]
split-console.patch

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

Looks good to me. As said above, I'll run a few manual tests.

::: browser/devtools/shared/widgets/Tooltip.js
@@ +513,5 @@
> +  this._onTooltipKeypress = (eventName, code, event) => {
> +    if (this.activeSwatch) {
> +      if (code === ESCAPE_KEYCODE) {
> +        this.revert();
> +        event.stopPropagation();

So this is for the case when you press escape to close the colorpicker, to avoid opening the webconsole.
That change is fine with me.
In fact, I added the attribute ignorekeys="true" on the XUL panel in tooltip.js in order to be able to control the behavior when keys are pressed, but that means that the XUL panel itself isn't blocking keypresses anymore so we should stop their propagation ourselves (in `this._onKeyPress`).

::: browser/devtools/webconsole/test/browser_webconsole_split.js
@@ +20,5 @@
> +  {
> +    info("About to check console loads even when non-webconsole panel is open");
> +
> +    openPanel("inspector").then(() => {
> +      toolbox.on("webconsole-ready", () => {

Out of curiosity, how is the webconsole loaded even when the panel isn't open? I haven't seen anything in your patch that loads it. Was this already the case?

Comment 42

[Brian Grinstead [:bgrins]]

Attachment: split-console.patch

(In reply to Patrick Brosset from comment #41)
> Comment on attachment 8334475 [details] [diff] [review]
> split-console.patch
> 
> Review of attachment 8334475 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me. As said above, I'll run a few manual tests.
> 
> ::: browser/devtools/shared/widgets/Tooltip.js
> @@ +513,5 @@
> > +  this._onTooltipKeypress = (eventName, code, event) => {
> > +    if (this.activeSwatch) {
> > +      if (code === ESCAPE_KEYCODE) {
> > +        this.revert();
> > +        event.stopPropagation();
> 
> So this is for the case when you press escape to close the colorpicker, to
> avoid opening the webconsole.
> That change is fine with me.
> In fact, I added the attribute ignorekeys="true" on the XUL panel in
> tooltip.js in order to be able to control the behavior when keys are
> pressed, but that means that the XUL panel itself isn't blocking keypresses
> anymore so we should stop their propagation ourselves (in
> `this._onKeyPress`).

I've updated the patch to stop propagation in _onKeyPress - which I agree will be better so we don't bump into this with new tooltips.

> 
> ::: browser/devtools/webconsole/test/browser_webconsole_split.js
> @@ +20,5 @@
> > +  {
> > +    info("About to check console loads even when non-webconsole panel is open");
> > +
> > +    openPanel("inspector").then(() => {
> > +      toolbox.on("webconsole-ready", () => {
> 
> Out of curiosity, how is the webconsole loaded even when the panel isn't
> open? I haven't seen anything in your patch that loads it. Was this already
> the case?

It loads when loadTool is called (this is usually initiated by selectTool, but in the case of the split console it is initiated by toggleSplitConsole).

Comment 43

[Patrick Brosset <:pbro>]

Comment on attachment 8334496 [details] [diff] [review]
split-console.patch

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

Looking good!

Comment 44

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset from comment #43)
> Comment on attachment 8334496 [details] [diff] [review]
> split-console.patch
> 
> Review of attachment 8334496 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good!

Try push: https://tbpl.mozilla.org/?tree=Try&rev=60b04f9edf9b

Comment 45

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/7eaa1a0d3fe1
https://tbpl.mozilla.org/?tree=Fx-Team&rev=7eaa1a0d3fe1

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7eaa1a0d3fe1

Comment 47

[Mihaela Velimiroviciu (:mihaelav)]

Is there any other automation needed for this?

Comment 48

[YF (Yang)]

Hi anyone, I want to know the pressing Esc key will toggle "split console" on other panels is a feature? or a bug? I feel this is annoying and anomaly.

Comment 49

[Patrick Brosset <:pbro>]

(In reply to YF (Yang) from comment #48)
> Hi anyone, I want to know the pressing Esc key will toggle "split console"
> on other panels is a feature? or a bug? I feel this is annoying and anomaly.
This is a feature, it's pretty useful to be able to get access to the console from any panel. Some usecases are: 
- in the inspector, you might need the console to get properties about the node currently selector (typing $0)
- in the debugger, you might want to evaluate some code when paused at a breakpoint
I guess using the console while on a different panel is less common, but very useful nonetheless.
If I'm not mistaken, other devtools do this too.

This feature was added in FF28 and announced here: https://hacks.mozilla.org/2013/12/split-console-pretty-print-minified-js-and-more-firefox-developer-tools-episode-28/

Comment 50

[YF (Yang)]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #49)
Okay, thanks for the reply.

But if I open the Developer Tools - Network panel, trying to load a large or slow pages, and try press Esc key to abort the loading, this feature would be triggered.
Perhaps this should not be happened?

Comment 51

[Patrick Brosset <:pbro>]

I see your usecase, but I don't see how we could detect that you want to stop the loading rather than opening the split console. And since we want to have the split console consistently on all panels, we have to have this listener there too.
The only way I see is for you to make sure that the content window is focused, rather than the devtools frame, this way, when pressing ESC, the devtools won't be getting the event at all.

Comment 52

[YF (Yang)]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #51)
Well, although it is not perfect (the focus initial in devtools, and this behavior is normal in the previous version).
Thanks again.