Last Comment Bug 724509 - Add an "Option" menu in the Inspector Toolbar
: Add an "Option" menu in the Inspector Toolbar
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: Trunk
: x86 All
: P2 normal (vote)
: Firefox 15
Assigned To: Paul Rouget [:paul]
:
Mentors:
: 694956 722404 (view as bug list)
Depends on: 717923 735214 754653
Blocks: 724507 733741
  Show dependency treegraph
 
Reported: 2012-02-06 04:07 PST by Paul Rouget [:paul]
Modified: 2012-05-21 01:23 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (29.45 KB, patch)
2012-02-06 04:36 PST, Paul Rouget [:paul]
dcamp: review+
Details | Diff | Review
patch v1.1 (28.32 KB, patch)
2012-03-07 11:47 PST, Paul Rouget [:paul]
no flags Details | Diff | Review
patch v1.2 (28.34 KB, patch)
2012-03-09 03:47 PST, Paul Rouget [:paul]
no flags Details | Diff | Review
patch v1.3 (31.05 KB, patch)
2012-03-16 06:04 PDT, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Review
patch v1.4 (29.38 KB, patch)
2012-04-18 09:07 PDT, Paul Rouget [:paul]
no flags Details | Diff | Review
patch v1.5 (30.56 KB, patch)
2012-05-09 02:35 PDT, Paul Rouget [:paul]
no flags Details | Diff | Review
patch v1.6 (29.66 KB, patch)
2012-05-09 02:53 PDT, Paul Rouget [:paul]
no flags Details | Diff | Review
patch v1.7 (30.05 KB, patch)
2012-05-11 05:23 PDT, Paul Rouget [:paul]
dao+bmo: review+
Details | Diff | Review
patch v1.8 (30.11 KB, patch)
2012-05-11 07:55 PDT, Paul Rouget [:paul]
no flags Details | Diff | Review

Description Paul Rouget [:paul] 2012-02-06 04:07:24 PST

    
Comment 1 Paul Rouget [:paul] 2012-02-06 04:36:19 PST
Created attachment 594677 [details] [diff] [review]
patch v1
Comment 2 Paul Rouget [:paul] 2012-02-06 04:37:14 PST
Comment on attachment 594677 [details] [diff] [review]
patch v1

tests to come
Comment 3 Dave Camp (:dcamp) 2012-02-06 15:58:42 PST
Comment on attachment 594677 [details] [diff] [review]
patch v1

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

Devtools changes look fine, someone else should probably check the browser.xul/css stuff.

::: browser/devtools/highlighter/highlighter.jsm
@@ -351,0 +358,13 @@
> > +
> > +  /**
> > +   * Hide the veil
> > +   */
NaN more ...

setAttribute's second arg is a string, the empty string should work.
Comment 4 Paul Rouget [:paul] 2012-03-07 10:05:17 PST
(In reply to Dave Camp (:dcamp) from comment #3)
> ::: browser/devtools/highlighter/highlighter.jsm
> @@ -351,0 +358,13 @@
> > > +
> > > +  /**
> > > +   * Hide the veil
> > > +   */
> NaN more ...

Sorry?
Comment 5 Dave Camp (:dcamp) 2012-03-07 10:10:21 PST
(In reply to Paul Rouget [:paul] from comment #4)
> 
> Sorry?

Grumble, splinter ate that.

setAttribute()'s second argument is a string, not a boolean.
Comment 6 Paul Rouget [:paul] 2012-03-07 11:47:00 PST
Created attachment 603808 [details] [diff] [review]
patch v1.1
Comment 7 Paul Rouget [:paul] 2012-03-09 03:47:52 PST
Created attachment 604359 [details] [diff] [review]
patch v1.2

rebased
Comment 8 Paul Rouget [:paul] 2012-03-13 03:56:02 PDT
*** Bug 722404 has been marked as a duplicate of this bug. ***
Comment 9 Dão Gottwald [:dao] 2012-03-13 08:05:48 PDT
Comment on attachment 604359 [details] [diff] [review]
patch v1.2

>+#highlighter-veil-container:not([dim]) .highlighter-veil {

child selector

This will need keyboard access too, the two options you're currently adding don't seem critical.
Comment 10 Paul Rouget [:paul] 2012-03-14 09:13:10 PDT
triage, filter on centaur
Comment 11 Paul Rouget [:paul] 2012-03-16 06:04:02 PDT
Created attachment 606545 [details] [diff] [review]
patch v1.3
Comment 12 Paul Rouget [:paul] 2012-03-16 06:05:07 PDT
Comment on attachment 606545 [details] [diff] [review]
patch v1.3

The menu is accessible with the tab key.
Comment 13 Dão Gottwald [:dao] 2012-03-20 06:20:57 PDT
> This will need keyboard access too, the two options you're currently adding
> don't seem critical.

I guess you figured this out already, but I meant to write: "..., _although_ the two options you're currently adding don't seem critical."
Comment 14 Dão Gottwald [:dao] 2012-03-20 06:27:53 PDT
Comment on attachment 606545 [details] [diff] [review]
patch v1.3

The button needs a label for accessibility.

The menu item labels should use title capitalization.

I'm not sure that "Show node details" is a good label (or a good option to have at all) since the info bar doesn't really show any additional details.
Comment 15 Paul Rouget [:paul] 2012-04-18 09:07:53 PDT
Created attachment 616163 [details] [diff] [review]
patch v1.4
Comment 16 Paul Rouget [:paul] 2012-04-18 09:09:58 PDT
Comment on attachment 616163 [details] [diff] [review]
patch v1.4

(In reply to Dão Gottwald [:dao] from comment #14)
> The button needs a label for accessibility.

I used an aria-label.

> The menu item labels should use title capitalization.

Done.

> I'm not sure that "Show node details" is a good label (or a good option to
> have at all) since the info bar doesn't really show any additional details.

I checked with Kevin and Shorlander. We agreed on "Show Node Info".
Comment 17 Dão Gottwald [:dao] 2012-05-07 02:14:41 PDT
Comment on attachment 616163 [details] [diff] [review]
patch v1.4

Could you please update this to apply on current mozilla-central? I'd take care of firefox.js and jar.mn myself, but there are more conflicts in browser.xul and inspector.jsm.
Comment 18 Johan C 2012-05-07 06:13:14 PDT
*** Bug 694956 has been marked as a duplicate of this bug. ***
Comment 19 Paul Rouget [:paul] 2012-05-09 02:35:14 PDT
Created attachment 622320 [details] [diff] [review]
patch v1.5

rebased
Comment 20 Paul Rouget [:paul] 2012-05-09 02:52:20 PDT
Comment on attachment 622320 [details] [diff] [review]
patch v1.5

Hmm, while testing this patch, I found a bug in the focus mechanism in the toolbar. It's a minor fix. I'll add that to this patch.
Comment 21 Paul Rouget [:paul] 2012-05-09 02:53:43 PDT
Created attachment 622323 [details] [diff] [review]
patch v1.6
Comment 22 Dão Gottwald [:dao] 2012-05-10 10:57:12 PDT
Comment on attachment 622323 [details] [diff] [review]
patch v1.6

>--- a/browser/base/content/highlighter.css
>+++ b/browser/base/content/highlighter.css

>+#highlighter-veil-container:not([dim]) > .highlighter-veil,
>+#highlighter-veil-container:not([dim]) > hbox > .highlighter-veil {
>+  background-color: transparent;
>+}

Can you use visibility:hidden here?

>--- a/browser/locales/en-US/chrome/browser/browser.dtd
>+++ b/browser/locales/en-US/chrome/browser/browser.dtd

>+<!-- LOCALIZATION NOTE (inspectMenuButton.*): The menu in the Inspector
>+  -  toolbar. The button doesn't have a label, so we use an aria-label
>+  -  for accessibility -->
>+<!ENTITY inspectMenuButton.tooltiptext      "Inspector Options">
>+<!ENTITY inspectMenuButton.arialabel        "Inspector Options">

I believe the tooltip text is automatically used as a label, so an aria-label with the same value seems redundant.

>--- a/browser/themes/gnomestripe/browser.css
>+++ b/browser/themes/gnomestripe/browser.css

>+#inspector-menu-toolbarbutton:focus {
>+  outline: 1px dotted hsla(210,30%,85%,0.4);
>+  outline-offset: -2px;
>+}

:-moz-focusring

Why is there no similar rule for winstripe and pinstripe?
Comment 23 Paul Rouget [:paul] 2012-05-11 05:23:34 PDT
Created attachment 623106 [details] [diff] [review]
patch v1.7

Addressed Dao's comments (and rebased)
Comment 24 Dão Gottwald [:dao] 2012-05-11 07:35:34 PDT
Comment on attachment 623106 [details] [diff] [review]
patch v1.7

>+<!-- LOCALIZATION NOTE (inspectMenuButton.*): The menu in the Inspector
>+  -  toolbar.  -->
>+<!ENTITY inspectMenuButton.tooltiptext      "Inspector Options">

This doesn't seem like useful localization note.
"inspectMenuButton" should probably be "inspectorOptionsButton" and "inspector-menu" should be replaced with "inspector-options" throughout this patch.
Comment 25 Paul Rouget [:paul] 2012-05-11 07:55:27 PDT
Created attachment 623141 [details] [diff] [review]
patch v1.8

s/menu/option. Thank you Dao
Comment 27 Rob Campbell [:rc] (:robcee) 2012-05-11 11:34:27 PDT
https://hg.mozilla.org/mozilla-central/rev/ecf8b9849a50

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