Add an "Option" menu in the Inspector Toolbar

RESOLVED FIXED in Firefox 15

Status

()

Firefox
Developer Tools: Inspector
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: paul, Assigned: paul)

Tracking

Trunk
Firefox 15
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 attachment, 8 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

5 years ago
Blocks: 724507
(Assignee)

Comment 1

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

Comment 2

5 years ago
Comment on attachment 594677 [details] [diff] [review]
patch v1

tests to come
Attachment #594677 - Flags: review?(dcamp)

Comment 3

5 years ago
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.
Attachment #594677 - Flags: review?(dcamp) → review+
(Assignee)

Updated

5 years ago
Blocks: 733741
(Assignee)

Comment 4

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

5 years ago
(In reply to Paul Rouget [:paul] from comment #4)
> 
> Sorry?

Grumble, splinter ate that.

setAttribute()'s second argument is a string, not a boolean.
(Assignee)

Comment 6

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

Updated

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

Updated

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

Comment 7

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

rebased
(Assignee)

Updated

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

Updated

5 years ago
Depends on: 717923
(Assignee)

Updated

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

Updated

5 years ago
Duplicate of this bug: 722404
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.
(Assignee)

Comment 10

5 years ago
triage, filter on centaur
Priority: -- → P2
Whiteboard: [firefox14-wanted]
(Assignee)

Updated

5 years ago
Assignee: nobody → paul
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Depends on: 735214
(Assignee)

Comment 11

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

Updated

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

Comment 12

5 years ago
Comment on attachment 606545 [details] [diff] [review]
patch v1.3

The menu is accessible with the tab key.
Attachment #606545 - Flags: review?(dao)
> 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 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.
Attachment #606545 - Flags: review?(dao) → review-
(Assignee)

Comment 15

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

Updated

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

Comment 16

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

Updated

5 years ago
Duplicate of this bug: 694956
(Assignee)

Comment 19

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

rebased
(Assignee)

Updated

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

Comment 20

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

Comment 21

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

Updated

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

Comment 23

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

Addressed Dao's comments (and rebased)
(Assignee)

Updated

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

Updated

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

Comment 25

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

s/menu/option. Thank you Dao
(Assignee)

Updated

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

Comment 26

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/ecf8b9849a50
Whiteboard: [firefox14-wanted] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/ecf8b9849a50
Target Milestone: --- → Firefox 15
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 754653
(Assignee)

Updated

5 years ago
No longer depends on: 735214

Updated

5 years ago
Depends on: 735214
You need to log in before you can comment on or make changes to this bug.