717923 - Use an icon for the inspect button

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Paul Rouget [:paul]]

We should use Firebug's icon.

Comment 1

[Paul Rouget [:paul]]

Stephen, can I get an icon for that?

Comment 2

[Paul Rouget [:paul]]

Attachment: patch v1

Comment 3

[Paul Rouget [:paul]]

Attachment: screenshot (animated)

Comment 4

[Dão Gottwald [:dao]]

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

Attachment: patch v1.1

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

Comment 6

[Dão Gottwald [:dao]]

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

Comment 7

[Paul Rouget [:paul]]

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

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

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

Comment on attachment 594163 [details] [diff] [review]
patch v1.1

either way we need a solution for this

Comment 11

[Axel Hecht [:Pike]]

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

Attachment: patch v1.2

Comment 13

[Dão Gottwald [:dao]]

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

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

Attachment: 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]]

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

Comment 17

[Paul Rouget [:paul]]

Attachment: patch 1.4

s/tooltiptext2/tooltiptext/

Comment 18

[Dão Gottwald [:dao]]

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

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

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

Attachment: patch 1.4 (rebased)

Comment 22

[Neil Deakin]

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

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

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

Attachment: patch v1.5

no accesskey

Comment 26

[Dão Gottwald [:dao]]

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

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

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

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

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

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

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

Comment 33

[Paul Rouget [:paul]]

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

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

Attachment: patch v1.6

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

Comment 36

[Dão Gottwald [:dao]]

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

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

Attachment: patch v1.7

use formatStringFromName

Comment 39

[Paul Rouget [:paul]]

Comment on attachment 603704 [details] [diff] [review]
patch v1.7

sorry, wrong version of the patch

Comment 40

[Paul Rouget [:paul]]

Attachment: patch v1.8

Comment 41

[Cedric Vivier [:cedricv]]

> "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]]

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

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

Attachment: patch v1.9

Comment 45

[Paul Rouget [:paul]]

Comment on attachment 604035 [details] [diff] [review]
patch v1.9

Addressed Dao's and Pike's comments.

Comment 46

[Dão Gottwald [:dao]]

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

Attachment: patch v1.9.1

Comment 48

[Paul Rouget [:paul]]

Thank you Dao.

Comment 49

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/0fe720174e5d

Comment 50

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/0fe720174e5d