We should use Firebug's icon.

Stephen, can I get an icon for that?

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.

Addressed Dao's comments. Also changed the min-width of the button per Shorlander request

Hm, I guess this is going to hide the access key?

(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?

(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.

So presumably you could write: tooltiptext="&inspectButton.tooltiptext; (&inspectButton.accesskey;)" but I don't know if this is ok for all locales.

Comment on attachment 594163 [details] [diff] [review] patch v1.1 either way we need a solution for this

I'd include the accesskey in the tooltiptext entity, like <!ENTITY inspectButton.accesskey "I"> <!ENTITY inspectButton.tooltiptext "Inspect (&inspectButton.accesskey;)">

(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 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

(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)

Hmmm, there was no tooltip in the first place. No need to rename it.

s/tooltiptext2/tooltiptext/

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>?

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?

(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.

(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.

(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?

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.

no accesskey

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?

(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

> 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?

(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.

(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?

(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.

So should the Enter key behavior be removed or is this still useful?

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.

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.

Better tooltip. Append ' (Return)' to the tooltip text

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

(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.

use formatStringFromName

Comment on attachment 603704 [details] [diff] [review] patch v1.7 sorry, wrong version of the patch

> "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 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 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 on attachment 604035 [details] [diff] [review] patch v1.9 Addressed Dao's and Pike's comments.

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"

Thank you Dao.