679753 - Remove Scratchpad's status bar, the Environment menu and the Tools menu

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Cedric Vivier [:cedricv]]

The current status bar in Scratchpad looks unnecessary/weird :

1) it just shows "Content" and for most people it is not actionable as they do not do 'chrome' development pref enabled and thus cannot switch context.

2) the presence of a "Chrome" context is not discoverable.

3) since 'chrome' context has serious safety implications, the switch from "Content" to "Chrome" in the status bar is perhaps too quiet


Maybe it would make sense to:

1) remove the statusbar, increase usable screen estate 

2) have "Chrome" visible but disabled in the menu, so that it is discoverable to add-on developers that they can use Scratchpad too - a quick google "how to enable chrome menu in scratchpad" to get the pref, otherwise they might not even think about it

3) to render the chrome switch more visible (and 'permanently' visible at a glance when switching between multiple scratchpads), use a notificationbox over the editor with text like "This Scratchpad interacts on the Chrome context. Beware."

Comment 1

[Clochix]

Attachment: Disable browserContext menuitem instead of hidding it

Here's a patch proposal that:
 - disable browserContext menuitem instead of hidding it when chrome is not enabled. A tooltip text explains how to enable it;
 - hide the statusbar  when chrome is not enabled;

I have not updated the tests, as I don't know if this ticket will be accepted.

Comment 2

[Cedric Vivier [:cedricv]]

(In reply to Clochix from comment #1)
> Created attachment 554127 [details] [diff] [review]
> Disable browserContext menuitem instead of hidding it

Nice! Great idea to use a tooltip so that how to enable the browser context is even more discoverable :)

Comment 3

[Cedric Vivier [:cedricv]]

Attachment: Improved patch, with test

Improved on your patch. Added notificationbox and updated tests.

Comment 4

[Cedric Vivier [:cedricv]]

Attachment: Bug 679753 - Remove status bar in Scratchpad. Make Browser context more discoverable and notify user more visibly. Based on patch by Clochix. r=?

https://bugzilla.mozilla.org/show_bug.cgi?id=680544

Comment 5

[Cedric Vivier [:cedricv]]

Comment on attachment 554543 [details] [diff] [review]
Bug 679753 - Remove status bar in Scratchpad. Make Browser context more discoverable and notify user more visibly. Based on patch by Clochix. r=?

Sorry for the duplicate patch. This one is the same as the previous one.
(but this time formatted and attached to the bug automatically right from the console: mozilla [bug-679753]$ git bz attach HEAD)

Comment 6

[Cedric Vivier [:cedricv]]

Attachment: Bug 679753 - Remove status bar in Scratchpad. Make Browser context more discoverable and notify user more visibly. Based on patch by Clochix. r=?

Comment 7

[Cedric Vivier [:cedricv]]

Comment on attachment 554543 [details] [diff] [review]
Bug 679753 - Remove status bar in Scratchpad. Make Browser context more discoverable and notify user more visibly. Based on patch by Clochix. r=?

Obsolete. New patch also remove the tooltiptext on Browser context menu item when it is already enabled.

Comment 8

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

Comment on attachment 554567 [details] [diff] [review]
Bug 679753 - Remove status bar in Scratchpad. Make Browser context more discoverable and notify user more visibly. Based on patch by Clochix. r=?

This looks like it could be a nice improvement.

Question: in,

   setBrowserContext: function SP_setBrowserContext()
   {
     let browser = document.getElementById("sp-menu-browser");
     document.getElementById("sp-menu-content").removeAttribute("checked");
     browser.setAttribute("checked", true);
     this.executionContext = SCRATCHPAD_CONTEXT_BROWSER;
-    this.statusbarStatus.label = browser.getAttribute("label");
+    this.notificationBox.appendNotification(
+      this.strings.GetStringFromName("browserContext.notification"),
+      SCRATCHPAD_CONTEXT_BROWSER,
+      null,
+      this.notificationBox.PRIORITY_WARNING_HIGH,
+      null);

If you repeatedly click the Browser context menu item, will this stack up multiple notifications? It may not matter as you're calling removeAllNotifications when switching back to content, but it's a little weird.

Comment 9

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

also, thanks for the patch, Clochix!

Comment 10

[Cedric Vivier [:cedricv]]

Attachment: [backed-out] Bug 679753 - Remove status bar in Scratchpad. Make Browser context more discoverable and notify user more visibly. Based on patch by Clochix. r=robcee

Comment 11

[Cedric Vivier [:cedricv]]

(In reply to Rob Campbell [:rc] (robcee) from comment #8)
> Comment on attachment 554567 [details] [diff] [review]
> If you repeatedly click the Browser context menu item, will this stack up
> multiple notifications? It may not matter as you're calling
> removeAllNotifications when switching back to content, but it's a little
> weird.

Good catch. Resetting context was also an unexpected behavior in current Scratchpad if the user repeatedly/inadvertently click the same context again (there's "Reset context" for that).

New patch with added guards so that attempting to switch to the same context is a no-op.

Comment 12

[Cedric Vivier [:cedricv]]

(In reply to Rob Campbell [:rc] (robcee) from comment #8)
> If you repeatedly click the Browser context menu item, will this stack up
> multiple notifications?

It does not as the notificationbox display one notification at a time (with the highest priority).

Comment 13

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

yes, but if you press the close button on the notification bar, what happens with multiple appended notifications?

Comment 14

[Cedric Vivier [:cedricv]]

(In reply to Rob Campbell [:rc] (robcee) from comment #13)
> yes, but if you press the close button on the notification bar, what happens
> with multiple appended notifications?

They would be shown in sequence. They would not "stack up". Anyways neither can happen with latest patch.

Comment 15

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

Comment on attachment 555048 [details] [diff] [review]
[backed-out] Bug 679753 - Remove status bar in Scratchpad. Make Browser context more discoverable and notify user more visibly. Based on patch by Clochix. r=robcee

Yep. Works well. Thanks!

Comment 16

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

Comment on attachment 555048 [details] [diff] [review]
[backed-out] Bug 679753 - Remove status bar in Scratchpad. Make Browser context more discoverable and notify user more visibly. Based on patch by Clochix. r=robcee

https://bugzilla.mozilla.org/show_bug.cgi?id=679753

Comment 17

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

Comment on attachment 555048 [details] [diff] [review]
[backed-out] Bug 679753 - Remove status bar in Scratchpad. Make Browser context more discoverable and notify user more visibly. Based on patch by Clochix. r=robcee

http://hg.mozilla.org/integration/fx-team/rev/20c17cdc5cb3

Test failures. See log for details:

http://tinderbox.mozilla.org/showlog.cgi?log=Fx-Team/1314372189.1314373411.24547.gz&fulltext=1

Please fix and run the update through the try server.

Comment 18

[Cedric Vivier [:cedricv]]

Attachment: Bug 679753 - Remove status bar in Scratchpad. Make Browser context more discoverable and notify user more visibly. Based on patch by Clochix. r=robcee

Comment 19

[Cedric Vivier [:cedricv]]

Comment on attachment 558769 [details] [diff] [review]
Bug 679753 - Remove status bar in Scratchpad. Make Browser context more discoverable and notify user more visibly. Based on patch by Clochix. r=robcee

I still do not have access to try server, I'll post it here anyways for now.
At least, this time I correctly checked it passes locally.

Comment 20

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

Comment on attachment 558769 [details] [diff] [review]
Bug 679753 - Remove status bar in Scratchpad. Make Browser context more discoverable and notify user more visibly. Based on patch by Clochix. r=robcee

looks like this should fix up the tests.

Comment 21

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

let's use this to test out your new try credentials. Let me know when the results are in. :)

Comment 22

[Dão Gottwald [:dao]]

Comment on attachment 558769 [details] [diff] [review]
Bug 679753 - Remove status bar in Scratchpad. Make Browser context more discoverable and notify user more visibly. Based on patch by Clochix. r=robcee

>--- a/browser/devtools/scratchpad/scratchpad.xul
>+++ b/browser/devtools/scratchpad/scratchpad.xul
>@@ -269,19 +269,20 @@
>         accesskey="&environmentMenu.accesskey;">
>     <menupopup id="sp-menu-environment">
>       <menuitem id="sp-menu-content"
>                 label="&contentContext.label;"
>                 accesskey="&contentContext.accesskey;"
>                 command="sp-cmd-contentContext"
>                 checked="true"
>                 type="radio"/>
>-      <menuitem id="sp-menu-browser" hidden="true"
>+      <menuitem id="sp-menu-browser" disabled="true"
>                 command="sp-cmd-browserContext"
>                 label="&browserContext.label;"
>+                tooltiptext="&browserContext.tooltiptext;"
>                 accesskey="&browserContext.accesskey;"
>                 type="radio"/>
>       <menuseparator/>
>       <menuitem id="sp-menu-resetContext"
>                 command="sp-cmd-resetContext"
>                 label="&resetContext.label;"
>                 accesskey="&resetContext.accesskey;"/>
>     </menupopup>

> <!ENTITY browserContext.label         "Browser">
>+<!ENTITY browserContext.tooltiptext   "To enable this, set devtools.chrome.enabled preference to true in about:config">
> <!ENTITY browserContext.accesskey     "B">

I'd go the other way and hide all of the Environment menu for devtools.chrome.enabled=false. It seems entirely uninteresting for web developers...

Comment 23

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

Maybe I'm biased because I like hacking on the browser, but I think it could be a good thing to expose more webdevs to browser chrome. Exposing the Browser Environment option in a more discoverable way might lead to more people hacking on the browser. I see that as a good thing. :)

Comment 24

[Dão Gottwald [:dao]]

(In reply to Rob Campbell [:rc] (robcee) from comment #23)
> Maybe I'm biased because I like hacking on the browser,

Very much so!

> but I think it could
> be a good thing to expose more webdevs to browser chrome. Exposing the
> Browser Environment option in a more discoverable way might lead to more
> people hacking on the browser. I see that as a good thing. :)

I think I started hacking Firefox in the DOM Inspector when it was bundled, so I know what you mean. The scratchpad however doesn't seem like a good entry point, as it doesn't provide any insight into browser innards.

Comment 25

[Cedric Vivier [:cedricv]]

(In reply to Dão Gottwald [:dao] from comment #24)
> I think I started hacking Firefox in the DOM Inspector when it was bundled,
> so I know what you mean. The scratchpad however doesn't seem like a good
> entry point, as it doesn't provide any insight into browser innards.

Otoh Scratchpad has an Inspect capability that is a first step towards that, and if/when we have auto-completion it could be a possible way to experiment with this use case in mind.

Also we cannot remove the whole Environment menu unless we find a place to move "Reset" to afaik.

Comment 26

[Dão Gottwald [:dao]]

(In reply to Cedric Vivier [cedricv] from comment #25)
> (In reply to Dão Gottwald [:dao] from comment #24)
> > I think I started hacking Firefox in the DOM Inspector when it was bundled,
> > so I know what you mean. The scratchpad however doesn't seem like a good
> > entry point, as it doesn't provide any insight into browser innards.
> 
> Otoh Scratchpad has an Inspect capability that is a first step towards that,
> and if/when we have auto-completion it could be a possible way to experiment
> with this use case in mind.

Again, you need to type something before you can inspect the result. Still no great entry point.

> Also we cannot remove the whole Environment menu unless we find a place to
> move "Reset" to afaik.

What does Reset do anyway? It doesn't seem to do anything over here.

Comment 27

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

(In reply to Dão Gottwald [:dao] from comment #24)
> (In reply to Rob Campbell [:rc] (robcee) from comment #23)
> > Maybe I'm biased because I like hacking on the browser,
> 
> Very much so!

:)

...

(In reply to Dão Gottwald [:dao] from comment #26)
> (In reply to Cedric Vivier [cedricv] from comment #25)
> > (In reply to Dão Gottwald [:dao] from comment #24)
> > > I think I started hacking Firefox in the DOM Inspector when it was bundled,
> > > so I know what you mean. The scratchpad however doesn't seem like a good
> > > entry point, as it doesn't provide any insight into browser innards.
> > 
> > Otoh Scratchpad has an Inspect capability that is a first step towards that,
> > and if/when we have auto-completion it could be a possible way to experiment
> > with this use case in mind.
> 
> Again, you need to type something before you can inspect the result. Still
> no great entry point.

Not sure about that either.

Typing "window" into a Scratchpad running in Browser and you can immediately see all of the globals associated with it and start digging into them. With a bit of experience with the Scratchpad, you learn some of the entry-points to start poking around in various contexts. That experience translates directly to browser exploration.

Coupled with MDN, I think a JS programmer can start figuring out the browser and even XPCOM programming (e.g., what's Components?)

> > Also we cannot remove the whole Environment menu unless we find a place to
> > move "Reset" to afaik.
> 
> What does Reset do anyway? It doesn't seem to do anything over here.

When you start evaluating code against content or browser, declared variables are attached to the sandbox you're running in. These can build up. Reset throws the old sandbox away and creates a new one, erasing any saved variables.

Comment 28

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

I meant typing "window" and choosing "inspect" from the menu. Just typing something you don't see a whole lot in the scratchpad right now. :)

Comment 29

[Dão Gottwald [:dao]]

(In reply to Rob Campbell [:rc] (robcee) from comment #27)
> Typing "window" into a Scratchpad running in Browser [...]

Sure, if you know that the browser chrome has a DOM comparable to a web page...
DOM Inspector exposed this, the Scratchpad doesn't really.

> Coupled with MDN, I think a JS programmer can start figuring out the browser
> and even XPCOM programming (e.g., what's Components?)

The coupling with MDN or something similar seems key to me. But then you can learn about devtools.chrome.enabled there.

> When you start evaluating code against content or browser, declared
> variables are attached to the sandbox you're running in. These can build up.
> Reset throws the old sandbox away and creates a new one, erasing any saved
> variables.

Any reason why this isn't under Execute, maybe even as "Reset & Run", since a reset alone doesn't really seem useful?

Comment 30

[Cedric Vivier [:cedricv]]

(In reply to Dão Gottwald [:dao] from comment #29)
> (In reply to Rob Campbell [:rc] (robcee) from comment #27)
> > Typing "window" into a Scratchpad running in Browser [...]
> 
> Sure, if you know that the browser chrome has a DOM comparable to a web
> page...
> DOM Inspector exposed this, the Scratchpad doesn't really.

This brings the idea that Inspect should work with the context's global object when nothing is selected :)
Rob?

In this case, Scratchpad would expose this information the same in any context.


> Any reason why this isn't under Execute, maybe even as "Reset & Run", since
> a reset alone doesn't really seem useful?

Reset alone is still useful when used with inspection.

Comment 31

[Dão Gottwald [:dao]]

(In reply to Cedric Vivier [cedricv] from comment #30)
> (In reply to Dão Gottwald [:dao] from comment #29)
> > (In reply to Rob Campbell [:rc] (robcee) from comment #27)
> > > Typing "window" into a Scratchpad running in Browser [...]
> > 
> > Sure, if you know that the browser chrome has a DOM comparable to a web
> > page...
> > DOM Inspector exposed this, the Scratchpad doesn't really.
> 
> This brings the idea that Inspect should work with the context's global
> object when nothing is selected :)
> Rob?
> 
> In this case, Scratchpad would expose this information the same in any
> context.

This still doesn't strike me as discoverable.

Anyway, if you're serious about this, why let the user jump through hoops with the disabled menu item, a tooltip and a hidden pref rather than just offering the browser context from the start?

Comment 32

[Cedric Vivier [:cedricv]]

(In reply to Rob Campbell [:rc] (robcee) from comment #21)
> let's use this to test out your new try credentials. Let me know when the
> results are in. :)

Try results: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=5d3caf1d321d

Comment 33

[Cedric Vivier [:cedricv]]

(In reply to Dão Gottwald [:dao] from comment #31)
> > In this case, Scratchpad would expose this information the same in any
> > context.
> 
> This still doesn't strike me as discoverable.

It is directly accessibly from the context menu.
I agree it would probably be more obvious if the contextual Inspect menu changed title according to the selection (eg. "Inspect selection" vs "Inspect global")

> 
> Anyway, if you're serious about this, why let the user jump through hoops
> with the disabled menu item, a tooltip and a hidden pref rather than just
> offering the browser context from the start?

That's a really good question. I do not know as well the history behind that decision.

Comment 34

[Kevin Dangoor]

My thoughts on this:

- most people are web developers, not browser developers
- web developers don't need the status bar (as it stands today) or the Environment menu (so they can go away if the pref isn't set)
- move "Reset" to "Execute" and rename it "Reset Variables" to be a little clearer about what it's used for

Our tools are, first and foremost, for web developers. But, we also want to support browser/extension developers and we can do that either via this pref or via an add-on that flips that pref and possibly installs some additional features (no need to worry about that add-on now though).

Comment 35

[Cedric Vivier [:cedricv]]

Attachment: Bug 679753 - Remove status bar and Environment menu in Scratchpad. Notify user more visibly when switching to Browser context. Based on patch by Clochix. r=robcee

Comment 36

[Cedric Vivier [:cedricv]]

Comment on attachment 559174 [details] [diff] [review]
Bug 679753 - Remove status bar and Environment menu in Scratchpad. Notify user more visibly when switching to Browser context. Based on patch by Clochix. r=robcee

New patch with the Environment menu removed entirely when pref devtools.chrome.enabled is not true.
Reset renamed to "Reset variables" and moved into Execute menu.

Comment 37

[Dão Gottwald [:dao]]

Comment on attachment 559174 [details] [diff] [review]
Bug 679753 - Remove status bar and Environment menu in Scratchpad. Notify user more visibly when switching to Browser context. Based on patch by Clochix. r=robcee

>-      <menuitem id="sp-menu-browser" hidden="true"
>+      <menuitem id="sp-menu-browser"
>                 command="sp-cmd-browserContext"
>                 label="&browserContext.label;"
>+                tooltiptext="&browserContext.tooltiptext;"

This is pointless now.

Comment 38

[Cedric Vivier [:cedricv]]

Attachment: Bug 679753 - Remove status bar and Environment menu in Scratchpad. Notify user more visibly when switching to Browser context. Based on patch by Clochix. r=robcee

Comment 39

[Cedric Vivier [:cedricv]]

(In reply to Dão Gottwald [:dao] from comment #37)
> Comment on attachment 559174 [details] [diff] [review]
> This is pointless now.

Wow, you're fast ;)
Updated patch above. Passes testsuite locally, try on the way  https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=de4af106a9d5

Comment 40

[Cedric Vivier [:cedricv]]

Try successful.

Comment 41

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

 <!ENTITY errorConsoleCmd.label        "Error Console">
-<!ENTITY errorConsoleCmd.accesskey    "C">
 <!ENTITY errorConsoleCmd.commandkey   "j">
 
 <!ENTITY webConsoleCmd.label          "Web Console">
-<!ENTITY webConsoleCmd.accesskey      "W">

Why remove the accessKeys?

Comment 42

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

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

Comment 43

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

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

Comment 44

[Francesco Lodolo [:flod]]

You can't do this (changing a label without changing its name)

-<!ENTITY resetContext.label           "Reset">
-<!ENTITY resetContext.accesskey       "R">
+<!ENTITY resetContext.label           "Reset Variables">
+<!ENTITY resetContext.accesskey       "T">

Comment 45

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

(In reply to flod (Francesco Lodolo) from comment #44)
> You can't do this (changing a label without changing its name)
> 
> -<!ENTITY resetContext.label           "Reset">
> -<!ENTITY resetContext.accesskey       "R">
> +<!ENTITY resetContext.label           "Reset Variables">
> +<!ENTITY resetContext.accesskey       "T">

My mistake. Sorry.

I'll backout while waiting for an updated patch.

Comment 46

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

backout: https://hg.mozilla.org/mozilla-central/rev/68799855e853

Comment 47

[Cedric Vivier [:cedricv]]

Attachment: Updated patch with incremented locale property id.

Comment 48

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

Cedric, you still haven't said why you're removing the accesskeys which I asked in Comment 41. Do they conflict with others or is it some other reason? Waiting on an answer this time before checking in.

Comment 49

[Cedric Vivier [:cedricv]]

Attachment: Remove leftover labels

Right. Because the Tools menu is removed as discussed earlier, they are not used anywhere.
What's misleading is that the labels should have been removed as well :)

Note that the command keys to launch ErrorConsole/WebConsole as if the browser was focused are still here.

Comment 50

[Dão Gottwald [:dao]]

(In reply to Cedric Vivier [cedricv] from comment #49)
> Right. Because the Tools menu is removed as discussed earlier, they are not
> used anywhere.

It hasn't been discussed in this bug, has it? I don't seem to remember nor find it.
It seems unrelated to the other changes and should probably have its own bug anyway.

Comment 51

[Cedric Vivier [:cedricv]]

(In reply to Dão Gottwald [:dao] from comment #50)
> (In reply to Cedric Vivier [cedricv] from comment #49)
> It hasn't been discussed in this bug, has it? I don't seem to remember nor
> find it.
> It seems unrelated to the other changes and should probably have its own bug
> anyway.

Renaming "Reset" to "Reset Variables" isn't strictly related to this bug's title as well.

Let's create two bugs for those or just rename this bug "Polish Scratchpad UI" ?

Comment 52

[Dão Gottwald [:dao]]

(In reply to Cedric Vivier [cedricv] from comment #51)
> (In reply to Dão Gottwald [:dao] from comment #50)
> > (In reply to Cedric Vivier [cedricv] from comment #49)
> > It hasn't been discussed in this bug, has it? I don't seem to remember nor
> > find it.
> > It seems unrelated to the other changes and should probably have its own bug
> > anyway.
> 
> Renaming "Reset" to "Reset Variables" isn't strictly related to this bug's
> title as well.

The bug tells how we got there. I see no relation to the Tools menu whatsoever.

Comment 53

[Cedric Vivier [:cedricv]]

(In reply to Dão Gottwald [:dao] from comment #52)
> The bug tells how we got there. I see no relation to the Tools menu
> whatsoever.

The reasoning is the same as for the removal of the Environment menu, there is only one menu item left in Tools for most users (ie. not browser/add-on developers).

This one-item menu looks like a weird/noisy/unnecessary way to introduce one more way to open a tool accessible through other ways (keyboard shortcut included).

Ideally we should perhaps open the console should open automatically when an error occurred with one scratchpad.

Comment 54

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

Comment on attachment 569671 [details] [diff] [review]
Remove leftover labels

+      errorConsoleCommand.removeAttribute("disabled");

I'm worried that the keys won't be discoverable without the menus. It feels a little strange to me.

It might be time to revisit and reopen bug 656701, Scratchpad missing a menu overlay.

alright, let's try this again!

Comment 55

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

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

Comment 56

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

https://hg.mozilla.org/mozilla-central/rev/0072f76a7b8b