Closed
Bug 724509
Opened 12 years ago
Closed 12 years ago
Add an "Option" menu in the Inspector Toolbar
Categories
(DevTools :: Inspector, defect, P2)
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)
30.11 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•12 years ago
|
||
Assignee | ||
Comment 2•12 years ago
|
||
Comment on attachment 594677 [details] [diff] [review] patch v1 tests to come
Attachment #594677 -
Flags: review?(dcamp)
Comment 3•12 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 | ||
Comment 4•12 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•12 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•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #594677 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #603808 -
Flags: review?(dao)
Assignee | ||
Comment 7•12 years ago
|
||
rebased
Assignee | ||
Updated•12 years ago
|
Attachment #603808 -
Attachment is obsolete: true
Attachment #603808 -
Flags: review?(dao)
Assignee | ||
Updated•12 years ago
|
Attachment #604359 -
Flags: review?(dao)
Comment 9•12 years ago
|
||
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•12 years ago
|
||
triage, filter on centaur
Priority: -- → P2
Whiteboard: [firefox14-wanted]
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → paul
Status: NEW → ASSIGNED
Assignee | ||
Comment 11•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #604359 -
Attachment is obsolete: true
Attachment #604359 -
Flags: review?(dao)
Assignee | ||
Comment 12•12 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)
Comment 13•12 years ago
|
||
> 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•12 years ago
|
||
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•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #606545 -
Attachment is obsolete: true
Assignee | ||
Comment 16•12 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 17•12 years ago
|
||
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)
Assignee | ||
Comment 19•12 years ago
|
||
rebased
Assignee | ||
Updated•12 years ago
|
Attachment #616163 -
Attachment is obsolete: true
Assignee | ||
Comment 20•12 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•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #622323 -
Flags: review?(dao)
Comment 22•12 years ago
|
||
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•12 years ago
|
||
Addressed Dao's comments (and rebased)
Assignee | ||
Updated•12 years ago
|
Attachment #622323 -
Attachment is obsolete: true
Attachment #622323 -
Flags: review?(dao)
Assignee | ||
Updated•12 years ago
|
Attachment #623106 -
Flags: review?(dao)
Comment 24•12 years ago
|
||
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•12 years ago
|
||
s/menu/option. Thank you Dao
Assignee | ||
Updated•12 years ago
|
Attachment #623106 -
Attachment is obsolete: true
Assignee | ||
Comment 26•12 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/ecf8b9849a50
Whiteboard: [firefox14-wanted] → [fixed-in-fx-team]
Comment 27•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ecf8b9849a50
Target Milestone: --- → Firefox 15
Updated•12 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•