724509 - Add an "Option" menu in the Inspector Toolbar

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Comment 1

[Paul Rouget [:paul]]

Attachment: patch v1

Comment 2

[Paul Rouget [:paul]]

Comment on attachment 594677 [details] [diff] [review]
patch v1

tests to come

Comment 3

[Dave Camp (: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.

Comment 4

[Paul Rouget [:paul]]

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

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

Attachment: patch v1.1

Comment 7

[Paul Rouget [:paul]]

Attachment: patch v1.2

rebased

Comment 9

[Dão Gottwald [:dao]]

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

triage, filter on centaur

Comment 11

[Paul Rouget [:paul]]

Attachment: patch v1.3

Comment 12

[Paul Rouget [:paul]]

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

The menu is accessible with the tab key.

Comment 13

[Dão Gottwald [: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 14

[Dão Gottwald [:dao]]

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

Attachment: patch v1.4

Comment 16

[Paul Rouget [:paul]]

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

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 19

[Paul Rouget [:paul]]

Attachment: patch v1.5

rebased

Comment 20

[Paul Rouget [:paul]]

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

Attachment: patch v1.6

Comment 22

[Dão Gottwald [: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?

Comment 23

[Paul Rouget [:paul]]

Attachment: patch v1.7

Addressed Dao's comments (and rebased)

Comment 24

[Dão Gottwald [: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.

Comment 25

[Paul Rouget [:paul]]

Attachment: patch v1.8

s/menu/option. Thank you Dao

Comment 26

[Paul Rouget [:paul]]

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

Comment 27

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/ecf8b9849a50