Use an icon for the inspect button

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Developer Tools
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: paul, Assigned: paul)

Tracking

Trunk
Firefox 13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 11 obsolete attachments)

(Assignee)

Description

5 years ago
We should use Firebug's icon.
(Assignee)

Updated

5 years ago
Assignee: nobody → paul
(Assignee)

Comment 1

5 years ago
Stephen, can I get an icon for that?
(Assignee)

Comment 2

5 years ago
Created attachment 590394 [details] [diff] [review]
patch v1
(Assignee)

Updated

5 years ago
Attachment #590394 - Flags: review?(dao)
(Assignee)

Comment 3

5 years ago
Created attachment 590397 [details]
screenshot (animated)
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.
Attachment #590394 - Flags: review?(dao) → review-
(Assignee)

Comment 5

5 years ago
Created attachment 594163 [details] [diff] [review]
patch v1.1

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

Updated

5 years ago
Attachment #594163 - Flags: review?(dao)
(Assignee)

Updated

5 years ago
Blocks: 717922
Hm, I guess this is going to hide the access key?
(Assignee)

Comment 7

5 years ago
(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
Attachment #594163 - Flags: review?(dao) → review-
I'd include the accesskey in the tooltiptext entity, like

<!ENTITY inspectButton.accesskey        "I">
<!ENTITY inspectButton.tooltiptext      "Inspect (&inspectButton.accesskey;)">
(Assignee)

Updated

5 years ago
Blocks: 724507
(Assignee)

Comment 12

5 years ago
Created attachment 594672 [details] [diff] [review]
patch v1.2
(Assignee)

Updated

5 years ago
Attachment #594163 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #594672 - Flags: review?(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 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
Attachment #594672 - Flags: review?(dao) → review-
(Assignee)

Comment 15

5 years ago
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)
(Assignee)

Updated

5 years ago
Attachment #594672 - Attachment is obsolete: true
(Assignee)

Comment 16

5 years ago
Hmmm, there was no tooltip in the first place. No need to rename it.
(Assignee)

Comment 17

5 years ago
Created attachment 600679 [details] [diff] [review]
patch 1.4

s/tooltiptext2/tooltiptext/
(Assignee)

Updated

5 years ago
Attachment #600678 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #600679 - Flags: review?(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>?
Attachment #600679 - Flags: review?(dao) → review-
(Assignee)

Comment 19

5 years ago
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.
(Assignee)

Comment 21

5 years ago
Created attachment 601239 [details] [diff] [review]
patch 1.4 (rebased)
(Assignee)

Updated

5 years ago
Attachment #600679 - Attachment is obsolete: true

Comment 22

5 years ago
(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.
(Assignee)

Comment 23

5 years ago
(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

5 years ago
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.
(Assignee)

Comment 25

5 years ago
Created attachment 602329 [details] [diff] [review]
patch v1.5

no accesskey
(Assignee)

Updated

5 years ago
Attachment #601239 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #602329 - Flags: review?(dao)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
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?
(Assignee)

Comment 27

5 years ago
(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?
(Assignee)

Comment 29

5 years ago
(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?
(Assignee)

Comment 31

5 years ago
(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?
(Assignee)

Comment 33

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 721593
(Assignee)

Comment 35

5 years ago
Created attachment 603211 [details] [diff] [review]
patch v1.6

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

Updated

5 years ago
Attachment #602329 - Attachment is obsolete: true
Attachment #602329 - Flags: review?(dao)
(Assignee)

Updated

5 years ago
Attachment #603211 - Flags: review?(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
Attachment #603211 - Flags: review?(dao) → review-

Updated

5 years ago
Attachment #590394 - Attachment is obsolete: true
(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.
(Assignee)

Comment 38

5 years ago
Created attachment 603704 [details] [diff] [review]
patch v1.7

use formatStringFromName
(Assignee)

Updated

5 years ago
Attachment #603211 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #603704 - Flags: review?(dao)
Attachment #603704 - Flags: feedback?(l10n)
(Assignee)

Comment 39

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

sorry, wrong version of the patch
Attachment #603704 - Attachment is obsolete: true
Attachment #603704 - Flags: review?(dao)
Attachment #603704 - Flags: feedback?(l10n)
(Assignee)

Comment 40

5 years ago
Created attachment 603724 [details] [diff] [review]
patch v1.8
(Assignee)

Updated

5 years ago
Attachment #603724 - Flags: feedback?(l10n)
(Assignee)

Updated

5 years ago
Attachment #603724 - Flags: review?(dao)
> "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
(Assignee)

Updated

5 years ago
Blocks: 717916
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.
Attachment #603724 - Flags: feedback?(l10n) → feedback+
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.
Attachment #603724 - Flags: review?(dao)
(Assignee)

Comment 44

5 years ago
Created attachment 604035 [details] [diff] [review]
patch v1.9
(Assignee)

Updated

5 years ago
Attachment #603724 - Attachment is obsolete: true
(Assignee)

Comment 45

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

Addressed Dao's and Pike's comments.
Attachment #604035 - Flags: review?(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"
Attachment #604035 - Flags: review?(dao) → review+
(Assignee)

Comment 47

5 years ago
Created attachment 604045 [details] [diff] [review]
patch v1.9.1
(Assignee)

Updated

5 years ago
Attachment #604035 - Attachment is obsolete: true
(Assignee)

Comment 48

5 years ago
Thank you Dao.
Whiteboard: [land-in-fx-team]
(Assignee)

Updated

5 years ago
Blocks: 724509
https://hg.mozilla.org/integration/fx-team/rev/0fe720174e5d
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/0fe720174e5d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Hardware: x86 → All
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 13
You need to log in before you can comment on or make changes to this bug.