818428 - Don't use the word "Toolbox" in the UI

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Paul Rouget [:paul]]

It's used in the developer toolbar and in the 2 menus.

Comment 1

[Girish Sharma [:Optimizer]]

I think after bug 818447 lands, the app menu entry would be gone. So only the Tools > Web Developer > Toggle Toolbox and the one in Developer Toolbar would remain.


What string should be used in place of "Toggle Toolbox" ?

Comment 2

[Paul Rouget [:paul]]

In the menu: just "Toggle Tools", followed by a separator.
In the toolbar, we just remove the text and just keep the icon, but we'll need a tooltip, that should be "Toggle Tools".

Comment 3

[Girish Sharma [:Optimizer]]

Attachment: Remove "Toolbox" word from UI

(Please change the reviewer if required)

This fixes everything except that now when any error occurs in Web Console, the Developer Toolbar's Button on the right is left with only a square red box with a number inside (Since the label is gone).

Comment 4

[Girish Sharma [:Optimizer]]

Comment on attachment 692507 [details] [diff] [review]
Remove "Toolbox" word from UI

># HG changeset patch
># User Girish Sharma <[e-mail]>
># Date 1355524229 -19800
># Node ID d2b9eecb6d8b741bc576f63000dbd29237148b93
># Parent  034a28d4d68b8c7e5834f1d5db562b3da0d6ee21
>Bug 818447 - [toolbox] Make the Web Developer appmenu entry a combo button (Linux & Windows), r=paul
>
>diff --git a/browser/base/content/browser-appmenu.inc b/browser/base/content/browser-appmenu.inc
>--- a/browser/base/content/browser-appmenu.inc
>+++ b/browser/base/content/browser-appmenu.inc
>@@ -142,22 +142,20 @@
>                       label="&printPreviewCmd.label;"
>                       command="cmd_printPreview"/>
>             <menuitem id="appmenu_printSetup"
>                       label="&printSetupCmd.label;"
>                       command="cmd_pageSetup"/>
>           </menupopup>
>       </splitmenu>
>       <menuseparator class="appmenu-menuseparator"/>
>-      <menu id="appmenu_webDeveloper"
>-            label="&appMenuWebDeveloper.label;">
>+      <splitmenu id="appmenu_webDeveloper"
>+                 observes="devtoolsMenuBroadcaster_WebDeveloper"
>+                 label="&appMenuWebDeveloper.label;">
>         <menupopup id="appmenu_webDeveloper_popup">
>-          <menuitem id="appmenu_devToolbox"
>-                    observes="devtoolsMenuBroadcaster_DevToolbox"/>
>-          <menuseparator id="appmenu_devtools_separator"/>
>           <menuitem id="appmenu_devToolbar"
>                     observes="devtoolsMenuBroadcaster_DevToolbar"/>
>           <menuitem id="appmenu_chromeDebugger"
>                     observes="devtoolsMenuBroadcaster_ChromeDebugger"/>
>           <menuitem id="appmenu_responsiveUI"
>                     observes="devtoolsMenuBroadcaster_ResponsiveUI"/>
>           <menuitem id="appmenu_scratchpad"
>                     observes="devtoolsMenuBroadcaster_Scratchpad"/>
>@@ -177,17 +175,17 @@
> #include browser-charsetmenu.inc
> #undef ID_PREFIX
> #undef OMIT_ACCESSKEYS
>           <menuitem label="&goOfflineCmd.label;"
>                     type="checkbox"
>                     observes="workOfflineMenuitemState"
>                     oncommand="BrowserOffline.toggleOfflineStatus();"/>
>         </menupopup>
>-      </menu>
>+      </splitmenu>
>       <menuseparator class="appmenu-menuseparator"/>
> #define ID_PREFIX appmenu_
> #define OMIT_ACCESSKEYS
> #include browser-charsetmenu.inc
> #undef ID_PREFIX
> #undef OMIT_ACCESSKEYS
>       <menuitem id="appmenu_fullScreen"
>                 class="menuitem-tooltip"
>diff --git a/browser/base/content/browser-sets.inc b/browser/base/content/browser-sets.inc
>--- a/browser/base/content/browser-sets.inc
>+++ b/browser/base/content/browser-sets.inc
>@@ -172,16 +172,20 @@
>     <broadcaster id="socialSidebarBroadcaster" hidden="true"/>
>     <broadcaster id="socialActiveBroadcaster" hidden="true"/>
> 
>     <!-- DevTools broadcasters -->
>     <broadcaster id="devtoolsMenuBroadcaster_DevToolbox"
>                  label="&devToolbarToolsButton.label;"
>                  type="checkbox" autocheck="false"
>                  command="Tools:DevToolbox"/>
>+#ifndef XP_MACOSX
>+    <broadcaster id="devtoolsMenuBroadcaster_WebDeveloper"
>+                 command="Tools:DevToolbox"/>
>+#endif
>     <broadcaster id="devtoolsMenuBroadcaster_DevToolbar"
>                  label="&devToolbarMenu.label;"
>                  type="checkbox" autocheck="false"
>                  command="Tools:DevToolbar"
>                  key="key_devToolbar"/>
>     <broadcaster id="devtoolsMenuBroadcaster_ChromeDebugger"
>                  label="&chromeDebuggerMenu.label;"
>                  command="Tools:ChromeDebugger"/>

Comment 5

[Girish Sharma [:Optimizer]]

Attachment: Aactual patch

Sorry for the spam. This is the real patch.

Comment 6

[Girish Sharma [:Optimizer]]

Attachment: Fixed pinstripe radius

Changed pinstripe's button's border radius by mistake ...

Comment 7

[Paul Rouget [:paul]]

Comment on attachment 692512 [details] [diff] [review]
Fixed pinstripe radius

>        <splitmenu id="appmenu_webDeveloper"
> -                 observes="devtoolsMenuBroadcaster_WebDeveloper"
> +                 observes="devtoolsMenuBroadcaster_DevToolboxLabelLess"

See comment in bug 818447.

> diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
> --- a/browser/base/content/browser.xul
> +++ b/browser/base/content/browser.xul
> @@ -1104,17 +1104,18 @@
>              <hbox class="gclitoolbar-prompt">
>                <label class="gclitoolbar-prompt-label">&#187;</label>
>              </hbox>
>              <hbox class="gclitoolbar-complete-node"/>
>              <textbox class="gclitoolbar-input-node" rows="1"/>
>            </stack>
>            <toolbarbutton id="developer-toolbar-toolbox-button"
>                           class="developer-toolbar-button"
> -                         observes="devtoolsMenuBroadcaster_DevToolbox"/>
> +                         observes="devtoolsMenuBroadcaster_DevToolboxLabelLess"
> +                         tooltiptext="&devToolbarToolsButton.tooltip;"/>

So the label-less feature should not be implemented in the observer,
but directly in the <toolbarbutton> markup. What happens if you force
the label to be empty? (label="")? If it doesn't work, you might want
to just display:none the label in CSS.

>        let broadcaster = win.document.getElementById("devtoolsMenuBroadcaster_DevToolbox");
> +      let broadcaster2 = win.document.getElementById("devtoolsMenuBroadcaster_DevToolboxLabelLess");

We really want only one broadcaster.
Broadcasters are here to "share" common features. It's better to have only one broadcaster that holds
the common attributes, and add the specific attributes in the markup.

>  <!ENTITY devToolbarCloseButton.tooltiptext "Close Developer Toolbar">
>  <!ENTITY devToolbarMenu.label              "Developer Toolbar">
>  <!ENTITY devToolbarMenu.accesskey          "v">
>  <!ENTITY devToolbar.keycode                "VK_F2">
>  <!ENTITY devToolbar.keytext                "F2">
> -<!ENTITY devToolbarToolsButton.label       "Toggle Toolbox">
> +<!ENTITY devToolbarToolsButton.label2      "Toggle Tools">
> +<!ENTITY devToolbarToolsButton.tooltip     "Toggle Tools">

1) Maybe we could use devToolbarToolsButton.label2 in the tooltip (no need to duplicate).
2) Maybe we want to use "Toggle Developer Tools", not "Toggle Tools". But that might be
too much for the menu. So we'd need to strings. So 1) would be irrelevant.

What do you think?

> -<!ENTITY devToolbox.accesskey              "B">
> +<!ENTITY devToolbox.accesskey              "T">

Ok.

>  .developer-toolbar-button {
>    -moz-appearance: none;
> -  min-width: 78px;
> +  -moz-box-align: center;

That looks weird on my Linux.

> +.developer-toolbar-button[label] {
> +  min-width: 78px;
> +}
> +
> +.developer-toolbar-button :not([label]) {
> +  min-width: 18px;
> +}

Is the space before ":not" in purpose?

Comment 8

[Girish Sharma [:Optimizer]]

(In reply to Paul Rouget [:paul] from comment #7)
> Comment on attachment 692512 [details] [diff] [review]
> Fixed pinstripe radius
> 
> >        <splitmenu id="appmenu_webDeveloper"
> > -                 observes="devtoolsMenuBroadcaster_WebDeveloper"
> > +                 observes="devtoolsMenuBroadcaster_DevToolboxLabelLess"
> 
> See comment in bug 818447.

Got it.

> > -                         observes="devtoolsMenuBroadcaster_DevToolbox"/>
> > +                         observes="devtoolsMenuBroadcaster_DevToolboxLabelLess"
> > +                         tooltiptext="&devToolbarToolsButton.tooltip;"/>
> 
> So the label-less feature should not be implemented in the observer,
> but directly in the <toolbarbutton> markup. What happens if you force
> the label to be empty? (label="")? If it doesn't work, you might want
> to just display:none the label in CSS.

It is not possible to overwrite the broadcaster's label. I will go with display none.

> >        let broadcaster = win.document.getElementById("devtoolsMenuBroadcaster_DevToolbox");
> > +      let broadcaster2 = win.document.getElementById("devtoolsMenuBroadcaster_DevToolboxLabelLess");
> 
> We really want only one broadcaster.
> Broadcasters are here to "share" common features. It's better to have only
> one broadcaster that holds
> the common attributes, and add the specific attributes in the markup.

noted. will switch to single

> >  <!ENTITY devToolbarCloseButton.tooltiptext "Close Developer Toolbar">
> >  <!ENTITY devToolbarMenu.label              "Developer Toolbar">
> >  <!ENTITY devToolbarMenu.accesskey          "v">
> >  <!ENTITY devToolbar.keycode                "VK_F2">
> >  <!ENTITY devToolbar.keytext                "F2">
> > -<!ENTITY devToolbarToolsButton.label       "Toggle Toolbox">
> > +<!ENTITY devToolbarToolsButton.label2      "Toggle Tools">
> > +<!ENTITY devToolbarToolsButton.tooltip     "Toggle Tools">
> 
> 1) Maybe we could use devToolbarToolsButton.label2 in the tooltip (no need
> to duplicate).
> 2) Maybe we want to use "Toggle Developer Tools", not "Toggle Tools". But
> that might be
> too much for the menu. So we'd need to strings. So 1) would be irrelevant.
> 
> What do you think?

I think you are right, tooltip should say "Toggle Developer Tools"

> >  .developer-toolbar-button {
> >    -moz-appearance: none;
> > -  min-width: 78px;
> > +  -moz-box-align: center;
> 
> That looks weird on my Linux.

The box align center ?

> > +.developer-toolbar-button[label] {
> > +  min-width: 78px;
> > +}
> > +
> > +.developer-toolbar-button :not([label]) {
> > +  min-width: 18px;
> > +}
> 
> Is the space before ":not" in purpose?

No, mistake.

Comment 9

[Paul Rouget [:paul]]

(In reply to Girish Sharma [:Optimizer] from comment #3)
> This fixes everything except that now when any error occurs in Web Console,
> the Developer Toolbar's Button on the right is left with only a square red
> box with a number inside (Since the label is gone).

What do we do about that?

Comment 10

[Girish Sharma [:Optimizer]]

Attachment: patch v2.0

Update the patch addressing comments.

Lets fix the Toolbar button issue of only having the error count visible in a followup bug where we can get some UX love too :)

Comment 11

[Paul Rouget [:paul]]

Comment on attachment 693021 [details] [diff] [review]
patch v2.0

You're going to hate me, but I changed my mind:
- don't display:none the label
- don't include label=&devToolbarToolsButton.label2; in the broadcaster, but directly in the menuitem

Does it make sense?

Comment 12

[Girish Sharma [:Optimizer]]

(In reply to Paul Rouget [:paul] from comment #11)
> You're going to hate me, but I changed my mind:
> - don't display:none the label

Why, display:none approach was much better, as the styling of the button was perfect without the need of -mox-boxalign:center. Otherwise the icon was not appearing in the center of the button. It required many workarounds.

> - don't include label=&devToolbarToolsButton.label2; in the broadcaster, but
> directly in the menuitem

Then that would mean manually adding label to 2 places. Is it a win to add 2 labels instead of removing one ?

> Does it make sense?

Not sure.

Comment 13

[Paul Rouget [:paul]]

I'd like to avoid polluting browser.css.

Comment 14

[Girish Sharma [:Optimizer]]

(In reply to Paul Rouget [:paul] from comment #13)
> I'd like to avoid polluting browser.css.

Well, then you should probably go with this patch only.

The previous one added 3 extra css rules. This one adds only 2.

Comment 15

[Paul Rouget [:paul]]

(In reply to Girish Sharma [:Optimizer] from comment #14)
> Well, then you should probably go with this patch only.

Ok. Ask :dao to review this patch.

Comment 16

[Girish Sharma [:Optimizer]]

Comment on attachment 693021 [details] [diff] [review]
patch v2.0

Asking Dao for review.

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 693021 [details] [diff] [review]
patch v2.0

>     <broadcaster id="devtoolsMenuBroadcaster_DevToolbox"
>-                 label="&devToolbarToolsButton.label;"
>+                 label="&devToolbarToolsButton.label2;"

I'd prefer an entity name referring to the menu while the button is using that string as well, rather than the other way around.

>-<!ENTITY devToolbox.accesskey              "B">
>+<!ENTITY devToolbox.accesskey              "T">

Needs a new entity name for localizers to pick up the change.

Comment 18

[Girish Sharma [:Optimizer]]

Attachment: patch v2.1

comments addressed.

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 696688 [details] [diff] [review]
patch v2.1

>+<!ENTITY devToolbarToolsButton.tooltip     "Toggle Developer Tools">

Tooltips don't use title capitalization, so unless you consider "Developer Tools" a proper name, this should be "Toggle developer tools".

Comment 20

[Girish Sharma [:Optimizer]]

Attachment: patch v2.2

Carry forward r+ from :dao

Please land it wherever it is supposed to land first (fx-team or m-i)

Comment 21

[Paul Rouget [:paul]]

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

Comment 22

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

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