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]
:
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 | 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 | Review
patch v1.2 (19.28 KB, patch)
2012-02-06 04:13 PST, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Review
patch 1.3 (19.52 KB, patch)
2012-02-25 09:09 PST, Paul Rouget [:paul]
no flags Details | Diff | Review
patch 1.4 (19.52 KB, patch)
2012-02-25 09:19 PST, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Review
patch 1.4 (rebased) (19.52 KB, patch)
2012-02-28 04:37 PST, Paul Rouget [:paul]
no flags Details | Diff | Review
patch v1.5 (19.51 KB, patch)
2012-03-02 05:57 PST, Paul Rouget [:paul]
no flags Details | Diff | Review
patch v1.6 (21.01 KB, patch)
2012-03-06 02:41 PST, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Review
patch v1.7 (21.42 KB, patch)
2012-03-07 06:56 PST, Paul Rouget [:paul]
no flags Details | Diff | Review
patch v1.8 (21.42 KB, patch)
2012-03-07 07:46 PST, Paul Rouget [:paul]
l10n: feedback+
Details | Diff | Review
patch v1.9 (21.33 KB, patch)
2012-03-08 06:32 PST, Paul Rouget [:paul]
dao+bmo: review+
Details | Diff | Review
patch v1.9.1 (21.35 KB, patch)
2012-03-08 07:02 PST, Paul Rouget [:paul]
no flags Details | Diff | Review

Description Paul Rouget [:paul] 2012-01-13 07:07:27 PST
We should use Firebug's icon.
Comment 1 Paul Rouget [:paul] 2012-01-19 13:06:22 PST
Stephen, can I get an icon for that?
Comment 2 Paul Rouget [:paul] 2012-01-20 17:05:00 PST
Created attachment 590394 [details] [diff] [review]
patch v1
Comment 3 Paul Rouget [:paul] 2012-01-20 17:12:38 PST
Created attachment 590397 [details]
screenshot (animated)
Comment 4 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 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 Dão Gottwald [:dao] 2012-02-03 08:21:39 PST
Hm, I guess this is going to hide the access key?
Comment 7 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 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 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 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 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 Paul Rouget [:paul] 2012-02-06 04:13:41 PST
Created attachment 594672 [details] [diff] [review]
patch v1.2
Comment 13 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 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 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 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 Paul Rouget [:paul] 2012-02-25 09:19:14 PST
Created attachment 600679 [details] [diff] [review]
patch 1.4

s/tooltiptext2/tooltiptext/
Comment 18 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 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 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 Paul Rouget [:paul] 2012-02-28 04:37:14 PST
Created attachment 601239 [details] [diff] [review]
patch 1.4 (rebased)
Comment 22 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 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 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 Paul Rouget [:paul] 2012-03-02 05:57:25 PST
Created attachment 602329 [details] [diff] [review]
patch v1.5

no accesskey
Comment 26 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 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 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 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 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 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 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 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 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 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 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 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 Paul Rouget [:paul] 2012-03-07 06:56:30 PST
Created attachment 603704 [details] [diff] [review]
patch v1.7

use formatStringFromName
Comment 39 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 Paul Rouget [:paul] 2012-03-07 07:46:52 PST
Created attachment 603724 [details] [diff] [review]
patch v1.8
Comment 41 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 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 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 Paul Rouget [:paul] 2012-03-08 06:32:38 PST
Created attachment 604035 [details] [diff] [review]
patch v1.9
Comment 45 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 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 Paul Rouget [:paul] 2012-03-08 07:02:48 PST
Created attachment 604045 [details] [diff] [review]
patch v1.9.1
Comment 48 Paul Rouget [:paul] 2012-03-08 07:04:08 PST
Thank you Dao.
Comment 49 Panos Astithas [:past] 2012-03-12 04:15:13 PDT
https://hg.mozilla.org/integration/fx-team/rev/0fe720174e5d
Comment 50 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.