Closed
Bug 818428
Opened 12 years ago
Closed 12 years ago
Don't use the word "Toolbox" in the UI
Categories
(DevTools :: General, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 20
People
(Reporter: paul, Assigned: Optimizer)
References
Details
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
Optimizer
:
review+
|
Details | Diff | Splinter Review |
It's used in the developer toolbar and in the 2 menus.
Reporter | ||
Updated•12 years ago
|
Priority: -- → P2
Assignee | ||
Comment 1•12 years ago
|
||
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
Reporter | ||
Comment 2•12 years ago
|
||
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".
Assignee | ||
Comment 3•12 years ago
|
||
(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)
Assignee | ||
Comment 4•12 years ago
|
||
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
Assignee | ||
Comment 5•12 years ago
|
||
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)
Assignee | ||
Comment 6•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Whiteboard: [has-patch]
Reporter | ||
Comment 7•12 years ago
|
||
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">»</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-
Assignee | ||
Comment 8•12 years ago
|
||
(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.
Reporter | ||
Comment 9•12 years ago
|
||
(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?
Assignee | ||
Comment 10•12 years ago
|
||
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)
Reporter | ||
Comment 11•12 years ago
|
||
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)
Assignee | ||
Comment 12•12 years ago
|
||
(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.
Reporter | ||
Comment 13•12 years ago
|
||
I'd like to avoid polluting browser.css.
Assignee | ||
Comment 14•12 years ago
|
||
(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.
Reporter | ||
Comment 15•12 years ago
|
||
(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.
Assignee | ||
Comment 16•12 years ago
|
||
Comment on attachment 693021 [details] [diff] [review]
patch v2.0
Asking Dao for review.
Attachment #693021 -
Flags: review?(dao)
Comment 17•12 years ago
|
||
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-
Assignee | ||
Comment 18•12 years ago
|
||
comments addressed.
Attachment #693021 -
Attachment is obsolete: true
Attachment #696688 -
Flags: review?(dao)
Reporter | ||
Updated•12 years ago
|
Priority: P2 → P1
Comment 19•12 years ago
|
||
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+
Assignee | ||
Comment 20•12 years ago
|
||
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+
Reporter | ||
Updated•12 years ago
|
Whiteboard: [has-patch] → [land-in-fx-team]
Reporter | ||
Comment 21•12 years ago
|
||
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
Comment 22•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 20
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•