Closed Bug 818428 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: General, defect, P1)

x86
All
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 20

People

(Reporter: paul, Assigned: Optimizer)

References

Details

Attachments

(1 file, 5 obsolete files)

It's used in the developer toolbar and in the 2 menus.
Priority: -- → P2
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" ?
Assignee: nobody → scrapmachines
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".
Depends on: 818447
Attached patch Remove "Toolbox" word from UI (obsolete) — Splinter Review
(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).
Attachment #692507 - Flags: review?(paul)
Comment on attachment 692507 [details] [diff] [review]
Remove "Toolbox" word from UI

># HG changeset patch
># User Girish Sharma <scrapmachines@gmail.com>
># 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"/>
Attachment #692507 - Attachment description: Remove Toolbox → Remove "Toolbox" word from UI
Attached patch Aactual patch (obsolete) — Splinter Review
Sorry for the spam. This is the real patch.
Attachment #692507 - Attachment is obsolete: true
Attachment #692507 - Flags: review?(paul)
Attachment #692510 - Flags: review?(paul)
Attached patch Fixed pinstripe radius (obsolete) — Splinter Review
Changed pinstripe's button's border radius by mistake ...
Attachment #692510 - Attachment is obsolete: true
Attachment #692510 - Flags: review?(paul)
Attachment #692512 - Flags: review?(paul)
Whiteboard: [has-patch]
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?
Attachment #692512 - Flags: review?(paul) → review-
(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.
(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?
Attached patch patch v2.0 (obsolete) — Splinter Review
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 :)
Attachment #692512 - Attachment is obsolete: true
Attachment #693021 - Flags: review?(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?
Attachment #693021 - Flags: review?(paul)
(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.
I'd like to avoid polluting browser.css.
(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.
(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 on attachment 693021 [details] [diff] [review]
patch v2.0

Asking Dao for review.
Attachment #693021 - Flags: review?(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.
Attachment #693021 - Flags: review?(dao) → review-
Attached patch patch v2.1 (obsolete) — Splinter Review
comments addressed.
Attachment #693021 - Attachment is obsolete: true
Attachment #696688 - Flags: review?(dao)
Priority: P2 → P1
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".
Attachment #696688 - Flags: review?(dao) → review+
Attached patch patch v2.2Splinter Review
Carry forward r+ from :dao

Please land it wherever it is supposed to land first (fx-team or m-i)
Attachment #696688 - Attachment is obsolete: true
Attachment #697002 - Flags: review+
Whiteboard: [has-patch] → [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/c7d1fb2a2248
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
Depends on: 826695
https://hg.mozilla.org/mozilla-central/rev/c7d1fb2a2248
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 20
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.