As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 717923 - Use an icon for the inspect button
: Use an icon for the inspect button
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 13
Assigned To: Paul Rouget [:paul]
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on:
Blocks: 717916 717922 721593 724507 724509
  Show dependency treegraph
 
Reported: 2012-01-13 07:07 PST by Paul Rouget [:paul]
Modified: 2012-03-12 12:59 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (17.82 KB, patch)
2012-01-20 17:05 PST, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Splinter Review
screenshot (animated) (277.03 KB, image/gif)
2012-01-20 17:12 PST, Paul Rouget [:paul]
no flags Details
patch v1.1 (19.25 KB, patch)
2012-02-03 06:33 PST, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Splinter Review
patch v1.2 (19.28 KB, patch)
2012-02-06 04:13 PST, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Splinter Review
patch 1.3 (19.52 KB, patch)
2012-02-25 09:09 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch 1.4 (19.52 KB, patch)
2012-02-25 09:19 PST, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Splinter Review
patch 1.4 (rebased) (19.52 KB, patch)
2012-02-28 04:37 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.5 (19.51 KB, patch)
2012-03-02 05:57 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.6 (21.01 KB, patch)
2012-03-06 02:41 PST, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Splinter Review
patch v1.7 (21.42 KB, patch)
2012-03-07 06:56 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.8 (21.42 KB, patch)
2012-03-07 07:46 PST, Paul Rouget [:paul]
l10n: feedback+
Details | Diff | Splinter Review
patch v1.9 (21.33 KB, patch)
2012-03-08 06:32 PST, Paul Rouget [:paul]
dao+bmo: review+
Details | Diff | Splinter Review
patch v1.9.1 (21.35 KB, patch)
2012-03-08 07:02 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review

Description User image Paul Rouget [:paul] 2012-01-13 07:07:27 PST
We should use Firebug's icon.
Comment 1 User image Paul Rouget [:paul] 2012-01-19 13:06:22 PST
Stephen, can I get an icon for that?
Comment 2 User image Paul Rouget [:paul] 2012-01-20 17:05:00 PST
Created attachment 590394 [details] [diff] [review]
patch v1
Comment 3 User image Paul Rouget [:paul] 2012-01-20 17:12:38 PST
Created attachment 590397 [details]
screenshot (animated)
Comment 4 User image Dão Gottwald [:dao] 2012-01-25 08:10:52 PST
Comment on attachment 590394 [details] [diff] [review]
patch v1

>           <toolbarbutton id="inspector-inspect-toolbutton"
>                          class="devtools-toolbarbutton"
>-                         label="&inspectButton.label;"
>                          accesskey="&inspectButton.accesskey;"
>                          command="Inspector:Inspect"/>

This makes inspectButton.label unused. You need to remove it from the dtd.
At the same time you need to add a tooltip for discoverability and accessibility.
Comment 5 User image Paul Rouget [:paul] 2012-02-03 06:33:12 PST
Created attachment 594163 [details] [diff] [review]
patch v1.1

Addressed Dao's comments. Also changed the min-width of the button per Shorlander request
Comment 6 User image Dão Gottwald [:dao] 2012-02-03 08:21:39 PST
Hm, I guess this is going to hide the access key?
Comment 7 User image Paul Rouget [:paul] 2012-02-03 08:28:35 PST
(In reply to Dão Gottwald [:dao] from comment #6)
> Hm, I guess this is going to hide the access key?

Yes. It's hidden but we can use it. Is that a problem?
Comment 8 User image Dão Gottwald [:dao] 2012-02-03 08:32:25 PST
(In reply to Paul Rouget [:paul] from comment #7)
> (In reply to Dão Gottwald [:dao] from comment #6)
> > Hm, I guess this is going to hide the access key?
> 
> Yes. It's hidden but we can use it. Is that a problem?

The problem is that users aren't going to be aware of its existence. So at this point you could remove the access key, but this isn't desirable either, since the key is actually useful.

I think Windows appends the access key to the tooltip in such cases, but as far as I know we have no built-in platform support for this.
Comment 9 User image Dão Gottwald [:dao] 2012-02-03 08:34:16 PST
So presumably you could write:
  tooltiptext="&inspectButton.tooltiptext; (&inspectButton.accesskey;)"
but I don't know if this is ok for all locales.
Comment 10 User image Dão Gottwald [:dao] 2012-02-03 08:37:06 PST
Comment on attachment 594163 [details] [diff] [review]
patch v1.1

either way we need a solution for this
Comment 11 User image Axel Hecht [:Pike] 2012-02-03 08:54:23 PST
I'd include the accesskey in the tooltiptext entity, like

<!ENTITY inspectButton.accesskey        "I">
<!ENTITY inspectButton.tooltiptext      "Inspect (&inspectButton.accesskey;)">
Comment 12 User image Paul Rouget [:paul] 2012-02-06 04:13:41 PST
Created attachment 594672 [details] [diff] [review]
patch v1.2
Comment 13 User image Dão Gottwald [:dao] 2012-02-06 08:09:42 PST
(In reply to Dão Gottwald [:dao] from comment #8)
> I think Windows appends the access key to the tooltip in such cases

Well, not quite. This was about shortcut keys rather than access keys:
http://msdn.microsoft.com/en-us/library/windows/desktop/bb545460.aspx#shortcutkeys
So in this case I think we'd want (Alt+I), not (I).
Comment 14 User image Dão Gottwald [:dao] 2012-02-07 00:58:33 PST
Comment on attachment 594672 [details] [diff] [review]
patch v1.2

>+.devtools-toolbarbutton:not([label]) > .toolbarbutton-text {
>+  display: none;
>+}

Can you move this to the content stylesheet?

other than that, see the previous comment
Comment 15 User image Paul Rouget [:paul] 2012-02-25 09:09:37 PST
Created attachment 600678 [details] [diff] [review]
patch 1.3

(Alt+I) and entity renamed:
> <!ENTITY inspectButton.tooltiptext2     "Inspect  (Alt+&inspectButton.accesskey;)">

Moved
>+.devtools-toolbarbutton:not([label]) > .toolbarbutton-text {
>+  display: none;
>+}
to highlighter.css (content)
Comment 16 User image Paul Rouget [:paul] 2012-02-25 09:11:08 PST
Hmmm, there was no tooltip in the first place. No need to rename it.
Comment 17 User image Paul Rouget [:paul] 2012-02-25 09:19:14 PST
Created attachment 600679 [details] [diff] [review]
patch 1.4

s/tooltiptext2/tooltiptext/
Comment 18 User image Dão Gottwald [:dao] 2012-02-26 07:18:42 PST
Comment on attachment 600679 [details] [diff] [review]
patch 1.4

>+<!ENTITY inspectButton.tooltiptext      "Inspect  (Alt+&inspectButton.accesskey;)">

This needs to be platform specific, see <http://mxr.mozilla.org/mozilla-central/search?string=VK_ALT&find=locales>.
Can we build the tooltip dynamically, using <chrome://global-platform/locale/platformKeys.properties>?
Comment 19 User image Paul Rouget [:paul] 2012-02-28 00:33:42 PST
If I do something like that:

var tooltiptext = button.getAttribute("tooltiptext");
var accesskey = button.getAttribute("accesskey");
var alt = bundle.getString("VK_ALT");

tooltiptext = tooltiptext + " (" + alt + "+" + accesskey + ")";

button.setAttribute("tooltiptext", tooltiptext);

Would that be ok? What about RTL languages? Would it work?

What about creating a dtd with the Alt strings?
Comment 20 User image Dão Gottwald [:dao] 2012-02-28 02:08:00 PST
(In reply to Paul Rouget [:paul] from comment #19)
> If I do something like that:
> 
> var tooltiptext = button.getAttribute("tooltiptext");
> var accesskey = button.getAttribute("accesskey");
> var alt = bundle.getString("VK_ALT");
> 
> tooltiptext = tooltiptext + " (" + alt + "+" + accesskey + ")";
> 
> button.setAttribute("tooltiptext", tooltiptext);

the MODIFIER_SEPARATOR string should be used instead of "+"

> Would that be ok? What about RTL languages? Would it work?

Axel or Ehsan would probably know.

> What about creating a dtd with the Alt strings?

That's extra work for every locale, so we should avoid it if we can without too much effort.
Comment 21 User image Paul Rouget [:paul] 2012-02-28 04:37:14 PST
Created attachment 601239 [details] [diff] [review]
patch 1.4 (rebased)
Comment 22 User image Neil Deakin 2012-02-28 06:16:56 PST
(In reply to Paul Rouget [:paul] from comment #19)
> If I do something like that:
> 
> var tooltiptext = button.getAttribute("tooltiptext");
> var accesskey = button.getAttribute("accesskey");
> var alt = bundle.getString("VK_ALT");
> 
> tooltiptext = tooltiptext + " (" + alt + "+" + accesskey + ")";
> 

Note though that the access key may not be the Alt key. On Mac, it is nothing, and on all platforms it can be changed with the ui.key.menuAccessKey preference.
Comment 23 User image Paul Rouget [:paul] 2012-02-28 07:05:33 PST
(In reply to Neil Deakin from comment #22)
> Note though that the access key may not be the Alt key. On Mac, it is
> nothing, and on all platforms it can be changed with the
> ui.key.menuAccessKey preference.

So what would you recommend? No tooltip on Mac?  Is it possible to get the name of the accesskey from the keycode in menuAccessKey?
Comment 24 User image Neil Deakin 2012-02-28 07:43:02 PST
I wouldn't display the accesskey in the tooltip at all, as we don't elsewhere. If we wanted to support this, we shouldn't do something specifically for this button.
Comment 25 User image Paul Rouget [:paul] 2012-03-02 05:57:25 PST
Created attachment 602329 [details] [diff] [review]
patch v1.5

no accesskey
Comment 26 User image Dão Gottwald [:dao] 2012-03-02 08:01:15 PST
Comment on attachment 602329 [details] [diff] [review]
patch v1.5

I'd normally r- this for missing keyboard-accessibility. On second thought, this button only controls what happens when moving the mouse over the page, doesn't? Does it have any other implications that would be of interest to keyboard users?
Comment 27 User image Paul Rouget [:paul] 2012-03-02 08:21:29 PST
(In reply to Dão Gottwald [:dao] from comment #26)
> Comment on attachment 602329 [details] [diff] [review]
> patch v1.5
> 
> I'd normally r- this for missing keyboard-accessibility. On second thought,
> this button only controls what happens when moving the mouse over the page,
> doesn't? Does it have any other implications that would be of interest to
> keyboard users?

So now I think about it, we can select nodes with the keyboard (up,down,left,right), so it affects keyboard users. But at the same time, we also use the Enter key to toggle the inspection (lock/unlock a node). No need to use Alt or Cmd (not an accesskey). So, in the tooltip, we could use:

> "Inspect (Enter)"

Would that work?

This will need to be created dynamically: http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/global/keys.properties#56
Comment 28 User image Dão Gottwald [:dao] 2012-03-02 08:23:44 PST
> So now I think about it, we can select nodes with the keyboard
> (up,down,left,right),

This seems to work regardless of the button state. Is this unexpected?
Comment 29 User image Paul Rouget [:paul] 2012-03-02 08:48:19 PST
(In reply to Dão Gottwald [:dao] from comment #28)
> > So now I think about it, we can select nodes with the keyboard
> > (up,down,left,right),
> 
> This seems to work regardless of the button state.

Yes, you're right.

> Is this unexpected?

Yes.

So, indeed, it only affects people using the mouse.
Comment 30 User image Dão Gottwald [:dao] 2012-03-02 08:54:41 PST
(In reply to Paul Rouget [:paul] from comment #29)
> (In reply to Dão Gottwald [:dao] from comment #28)
> > Is this unexpected?
> 
> Yes.

Should it be changed?
Comment 31 User image Paul Rouget [:paul] 2012-03-02 09:14:38 PST
(In reply to Dão Gottwald [:dao] from comment #30)
> (In reply to Paul Rouget [:paul] from comment #29)
> > (In reply to Dão Gottwald [:dao] from comment #28)
> > > Is this unexpected?
> > 
> > Yes.
> 
> Should it be changed?

Sorry, I read "expected".
The current behavior, being able to change the node with the keyboard, even if locked, is expected.
Comment 32 User image Dão Gottwald [:dao] 2012-03-02 09:20:04 PST
So should the Enter key behavior be removed or is this still useful?
Comment 33 User image Paul Rouget [:paul] 2012-03-02 09:54:33 PST
It is still useful. People want to be able to lock/unlock a node with the keyboard, while selecting the node with the mouse. Because they can click to lock the node, but they can't unlock the node with the mouse.
Comment 34 User image Dão Gottwald [:dao] 2012-03-02 11:10:02 PST
Then the keyboard shortcut should be exposed. While you're at it, can you also find a better tooltip than "Inspect"? I find this rather meaningless since this whole UI is about inspecting.
Comment 35 User image Paul Rouget [:paul] 2012-03-06 02:41:13 PST
Created attachment 603211 [details] [diff] [review]
patch v1.6

Better tooltip. Append ' (Return)' to the tooltip text
Comment 36 User image Dão Gottwald [:dao] 2012-03-06 05:57:59 PST
Comment on attachment 603211 [details] [diff] [review]
patch v1.6

>+  button.setAttribute("tooltiptext", tooltip + " (" + returnString + ")");

I think putting the parentheses in the string like this might be wrong for RTL locales. Please check with Ehsan.

>+<!-- LOCALIZATION NOTE (inspectButton.tooltiptext): This button appears
>+  - in the Inspector Toolbar. Pressing the "Return" key toggle its state.
>+  - The string ' (Return)' is automatically appended to the tooltip,
>+  - using the VK_RETURN value from:
>+  - toolkit/locales/en-US/chrome/global/keys.properties)"

This should be toolkit/chrome/global/keys.properties or just keys.properties. /en-US/ is certainly wrong for locales other than en-US.

>+<!ENTITY inspectButton.tooltiptext          "Select an element in the page">

"Select element by mouse"?

>--- a/browser/themes/pinstripe/devtools/common.css
>+++ b/browser/themes/pinstripe/devtools/common.css

>+.devtools-toolbarbutton:not([label]) > .toolbarbutton-text {
>+  display: none;
>+}

This is duplicated in browser/base/content/highlighter.css
Comment 37 User image Axel Hecht [:Pike] 2012-03-06 06:20:09 PST
(In reply to Dão Gottwald [:dao] from comment #36)
> Comment on attachment 603211 [details] [diff] [review]
> patch v1.6
> 
> >+  button.setAttribute("tooltiptext", tooltip + " (" + returnString + ")");
> 
> I think putting the parentheses in the string like this might be wrong for
> RTL locales. Please check with Ehsan.
> 

... and for language which don't use ' ' to separate words. Just get the value, and pass it in as a param to format.
Comment 38 User image Paul Rouget [:paul] 2012-03-07 06:56:30 PST
Created attachment 603704 [details] [diff] [review]
patch v1.7

use formatStringFromName
Comment 39 User image Paul Rouget [:paul] 2012-03-07 07:44:49 PST
Comment on attachment 603704 [details] [diff] [review]
patch v1.7

sorry, wrong version of the patch
Comment 40 User image Paul Rouget [:paul] 2012-03-07 07:46:52 PST
Created attachment 603724 [details] [diff] [review]
patch v1.8
Comment 41 User image Cedric Vivier [:cedricv] 2012-03-07 07:57:24 PST
> "Select element with mouse"

Perhaps, we should use a more generic "Select element below pointer" ?
I've not been using a mouse for years :p
Comment 42 User image Axel Hecht [:Pike] 2012-03-07 15:14:30 PST
Comment on attachment 603724 [details] [diff] [review]
patch v1.8

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

Technically yes, though I think we can do better on the Localization Note.

::: browser/locales/en-US/chrome/browser/devtools/inspector.properties
@@ +28,5 @@
> +
> +# LOCALIZATION NOTE (inspectButton.tooltiptext): This button appears
> +# in the Inspector Toolbar. Pressing the "Return" key toggle its state.
> +# $1 is the keyboard shortcut (VK_RETURN in chrome://global/locale/keys.properties)
> +inspectButton.tooltiptext=Select element with mouse (%S)

The Localization Note is referencing the wrong variable, should be $S, too.

I'm not too fond of the comment itself. buttons rarely have state, so that itself is probably worth explaining. I don't have a good idea ad-hoc, sadly. Maybe something along the lines of "The inspectButton is stateful, if it's pressed users can select an element. Pressing the "Return" key changes that state".

"pressed" there seems to be jargon, too, don't have a better idea.
Comment 43 User image Dão Gottwald [:dao] 2012-03-08 04:20:58 PST
Comment on attachment 603724 [details] [diff] [review]
patch v1.8

>--- a/browser/themes/pinstripe/devtools/common.css
>+++ b/browser/themes/pinstripe/devtools/common.css

>+.devtools-toolbarbutton:not([label]) > .toolbarbutton-text {
>+  display: none;
>+}

Again, this is duplicated in browser/base/content/highlighter.css.
Comment 44 User image Paul Rouget [:paul] 2012-03-08 06:32:38 PST
Created attachment 604035 [details] [diff] [review]
patch v1.9
Comment 45 User image Paul Rouget [:paul] 2012-03-08 06:35:54 PST
Comment on attachment 604035 [details] [diff] [review]
patch v1.9

Addressed Dao's and Pike's comments.
Comment 46 User image Dão Gottwald [:dao] 2012-03-08 06:52:31 PST
Comment on attachment 604035 [details] [diff] [review]
patch v1.9

>+# LOCALIZATION NOTE (inspectButton.tooltiptext):
>+# This button appears in the Inspector Toolbar. inspectButton is stateful,
>+# if it's pressed users can select an element. Pressing the "Return" key
>+# changes that state. %S is the keyboard shortcut (VK_RETURN in
>+# chrome://global/locale/keys.properties).

"select an element" => "select an element with the mouse"
Comment 47 User image Paul Rouget [:paul] 2012-03-08 07:02:48 PST
Created attachment 604045 [details] [diff] [review]
patch v1.9.1
Comment 48 User image Paul Rouget [:paul] 2012-03-08 07:04:08 PST
Thank you Dao.
Comment 49 User image Panos Astithas [:past] 2012-03-12 04:15:13 PDT
https://hg.mozilla.org/integration/fx-team/rev/0fe720174e5d
Comment 50 User image Tim Taubert [:ttaubert] 2012-03-12 12:59:03 PDT
https://hg.mozilla.org/mozilla-central/rev/0fe720174e5d

Note You need to log in before you can comment on or make changes to this bug.