Closed Bug 724509 Opened 9 years ago Closed 9 years ago

Add an "Option" menu in the Inspector Toolbar

Categories

(DevTools :: Inspector, defect, P2)

x86
All
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 15

People

(Reporter: paul, Assigned: paul)

References

Details

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

Attachments

(1 file, 8 obsolete files)

No description provided.
Blocks: 724507
Attached patch patch v1 (obsolete) — Splinter Review
Comment on attachment 594677 [details] [diff] [review]
patch v1

tests to come
Attachment #594677 - Flags: review?(dcamp)
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+
Blocks: 733741
(In reply to Dave Camp (:dcamp) from comment #3)
> ::: browser/devtools/highlighter/highlighter.jsm
> @@ -351,0 +358,13 @@
> > > +
> > > +  /**
> > > +   * Hide the veil
> > > +   */
> NaN more ...

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

Grumble, splinter ate that.

setAttribute()'s second argument is a string, not a boolean.
Attached patch patch v1.1 (obsolete) — Splinter Review
Attachment #594677 - Attachment is obsolete: true
Attachment #603808 - Flags: review?(dao)
Attached patch patch v1.2 (obsolete) — Splinter Review
rebased
Attachment #603808 - Attachment is obsolete: true
Attachment #603808 - Flags: review?(dao)
Depends on: 717923
Attachment #604359 - Flags: review?(dao)
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.
triage, filter on centaur
Priority: -- → P2
Whiteboard: [firefox14-wanted]
Assignee: nobody → paul
Status: NEW → ASSIGNED
Depends on: 735214
Attached patch patch v1.3 (obsolete) — Splinter Review
Attachment #604359 - Attachment is obsolete: true
Attachment #604359 - Flags: review?(dao)
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-
Attached patch patch v1.4 (obsolete) — Splinter Review
Attachment #606545 - Attachment is obsolete: true
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)
Duplicate of this bug: 694956
Attached patch patch v1.5 (obsolete) — Splinter Review
rebased
Attachment #616163 - Attachment is obsolete: true
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
Attached patch patch v1.6 (obsolete) — Splinter Review
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?
Attached patch patch v1.7 (obsolete) — Splinter Review
Addressed Dao's comments (and rebased)
Attachment #622323 - Attachment is obsolete: true
Attachment #622323 - Flags: review?(dao)
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+
Attached patch patch v1.8Splinter Review
s/menu/option. Thank you Dao
Attachment #623106 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/ecf8b9849a50
Whiteboard: [firefox14-wanted] → [fixed-in-fx-team]
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
No longer depends on: 735214
Depends on: 735214
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.