Last Comment Bug 683873 - [highlighter] move the close button to the toolbar
: [highlighter] move the close button to the toolbar
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: x86 All
: P2 normal (vote)
: Firefox 10
Assigned To: Paul Rouget [:paul]
:
Mentors:
Depends on: 672006
Blocks: 663830
  Show dependency treegraph
 
Reported: 2011-09-01 05:48 PDT by Paul Rouget [:paul]
Modified: 2011-11-25 05:23 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
DevTools Toolbar Close - Pinstripe (1.01 KB, image/png)
2011-09-02 08:33 PDT, Stephen Horlander [:shorlander]
no flags Details
DevTools Toolbar Close - Winstripe (898 bytes, image/png)
2011-09-02 08:33 PDT, Stephen Horlander [:shorlander]
no flags Details
patch v1 (17.95 KB, patch)
2011-09-13 15:05 PDT, Paul Rouget [:paul]
bugzilla: review+
dao+bmo: review-
Details | Diff | Splinter Review
patch v1.1 (28.68 KB, patch)
2011-10-21 05:34 PDT, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Splinter Review
patch v1.2 (25.55 KB, patch)
2011-10-21 08:56 PDT, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Splinter Review
patch v1.3 (24.85 KB, patch)
2011-11-01 02:31 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.4 (25.19 KB, patch)
2011-11-01 02:38 PDT, Paul Rouget [:paul]
dao+bmo: review+
Details | Diff | Splinter Review

Description Paul Rouget [:paul] 2011-09-01 05:48:26 PDT
because it takes too much space on the content.
Comment 1 Stephen Horlander [:shorlander] 2011-09-02 08:33:08 PDT
Created attachment 557842 [details]
DevTools Toolbar Close - Pinstripe
Comment 2 Stephen Horlander [:shorlander] 2011-09-02 08:33:54 PDT
Created attachment 557843 [details]
DevTools Toolbar Close - Winstripe

States are from left to right: Normal, Hover, Pressed
Comment 3 Paul Rouget [:paul] 2011-09-02 10:01:05 PDT
Thank you Stephen.
Comment 4 Paul Rouget [:paul] 2011-09-06 02:12:07 PDT
I think this button should be at the right-end of the tool bar (in LTR, and for all the OSes). I don't think it is a good idea to keep it next to the "Inspect" button (which is the most important on in this toolbar).
Comment 5 Paul Rouget [:paul] 2011-09-13 15:05:25 PDT
Created attachment 560046 [details] [diff] [review]
patch v1
Comment 6 Dão Gottwald [:dao] 2011-09-13 16:36:53 PDT
(In reply to Stephen Horlander from comment #2)
> Created attachment 557843 [details]
> DevTools Toolbar Close - Winstripe
> 
> States are from left to right: Normal, Hover, Pressed

Is it just me or is that image blurry?

(In reply to Paul Rouget [:paul] from comment #4)
> I think this button should be at the right-end of the tool bar (in LTR, and
> for all the OSes). I don't think it is a good idea to keep it next to the
> "Inspect" button (which is the most important on in this toolbar).

This makes it inconsistent with the add-on bar and the find bar.
Comment 7 Dão Gottwald [:dao] 2011-09-13 16:38:57 PDT
Comment on attachment 560046 [details] [diff] [review]
patch v1

>+      <spacer flex="1" />
>+      <toolbarbutton id="highlighter-close-button" oncommand="InspectorUI.closeInspectorUI(false);" tooltiptext="&inspectCloseButton.tooltiptext;" />

Superfluous spaces before /> and the second line is overly long.

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

> #highlighter-close-button {
>-  list-style-image: url("chrome://browser/skin/KUI-close.png");
>-  top: 12px;
>-  right: 12px;
>-  cursor: pointer;
>+  list-style-image: url("chrome://browser/skin/devtools/toolbarbutton-close.png");
>+  -moz-image-region: rect(0, 16px, 16px, 0);
>+  min-width: 16px;
>+  width: 16px;
>+}

For consistency with all other close buttons in gnomestrip, this should probably use the stock icon.
Comment 8 David Dahl :ddahl 2011-09-13 17:02:45 PDT
Comment on attachment 560046 [details] [diff] [review]
patch v1

Looks good. passing to dao.
Comment 9 Dão Gottwald [:dao] 2011-09-15 11:04:20 PDT
Comment on attachment 560046 [details] [diff] [review]
patch v1

see comment 6 and comment 7
Comment 10 Paul Rouget [:paul] 2011-10-21 05:34:42 PDT
Created attachment 568636 [details] [diff] [review]
patch v1.1

Dao, I use `margin:-4px` for the gnomestrip button:
> /* XXX Buttons have padding in widget/ that we don't want here but can't override with good CSS, so we must
>   use evil CSS to give the impression of smaller content */

The same thing is used for `.tab-close-button > .toolbarbutton-icon`
(see http://mxr.mozilla.org/mozilla-central/source/browser/themes/gnomestripe/browser/browser.css#1597)

Let me know if you think this could be a problem.
Comment 11 Dão Gottwald [:dao] 2011-10-21 08:18:54 PDT
Comment on attachment 568636 [details] [diff] [review]
patch v1.1

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

>+#inspector-toolbar[treepanel-open] {
>+  padding: 0 0 4px 3px;
>+}

Does this need to take RTL into account?

>+#inspector-toolbar[treepanel-open] .inspector-resizer {
>+  display: -moz-box;
> }

Can the descendant selector be avoided here?
This looks like it belongs in content CSS.

>diff --git a/browser/themes/gnomestripe/browser/devtools/toolbarbutton-close.png b/browser/themes/gnomestripe/browser/devtools/toolbarbutton-close.png

Gnomestripe doesn't need this image anymore.
Comment 12 Paul Rouget [:paul] 2011-10-21 08:56:07 PDT
Created attachment 568661 [details] [diff] [review]
patch v1.2

(In reply to Dão Gottwald [:dao] from comment #11)
> Comment on attachment 568636 [details] [diff] [review] [diff] [details] [review]
> patch v1.1
> 
> >--- a/browser/themes/gnomestripe/browser/browser.css
> >+++ b/browser/themes/gnomestripe/browser/browser.css
> 
> >+#inspector-toolbar[treepanel-open] {
> >+  padding: 0 0 4px 3px;
> >+}
> 
> Does this need to take RTL into account?

Fixed.

> >+#inspector-toolbar[treepanel-open] .inspector-resizer {
> >+  display: -moz-box;
> > }
> 
> Can the descendant selector be avoided here?

I could avoid this, but it would need more CSS (to match the 2 resizers),
or more JavaScript.

> This looks like it belongs in content CSS.

Fixed.

> >diff --git a/browser/themes/gnomestripe/browser/devtools/toolbarbutton-close.png b/browser/themes/gnomestripe/browser/devtools/toolbarbutton-close.png
> 
> Gnomestripe doesn't need this image anymore.

Good catch. And I forgot to remove them from jar.mn as well.
Fixed.
Comment 13 Paul Rouget [:paul] 2011-10-21 09:00:11 PDT
(sorry for the UTF8 bug in the previous comment. Sounds like bzexport doesn't like the "ã")
Comment 14 Dave Camp (:dcamp) 2011-10-27 09:13:27 PDT
We're doing developer tool prioritization, filter on 'brontozaur'
to ignore the spam.
Comment 15 Dão Gottwald [:dao] 2011-10-28 03:25:44 PDT
Comment on attachment 568661 [details] [diff] [review]
patch v1.2

(In reply to Paul Rouget [:paul] from comment #12)
> > >+#inspector-toolbar[treepanel-open] .inspector-resizer {
> > >+  display: -moz-box;
> > > }
> > 
> > Can the descendant selector be avoided here?
> 
> I could avoid this, but it would need more CSS (to match the 2 resizers),
> or more JavaScript.

I wouldn't mind if this was a separate document that got created on demand, but since this is in browser.xul, the extra code seems like a price worth paying.
Comment 16 Paul Rouget [:paul] 2011-11-01 02:02:24 PDT
(In reply to Dão Gottwald [:dao] from comment #15)
> Comment on attachment 568661 [details] [diff] [review] [diff] [details] [review]
> patch v1.2
> 
> (In reply to Paul Rouget [:paul] from comment #12)
> > > >+#inspector-toolbar[treepanel-open] .inspector-resizer {
> > > >+  display: -moz-box;
> > > > }
> > > 
> > > Can the descendant selector be avoided here?
> > 
> > I could avoid this, but it would need more CSS (to match the 2 resizers),
> > or more JavaScript.
> 
> I wouldn't mind if this was a separate document that got created on demand,
> but since this is in browser.xul, the extra code seems like a price worth
> paying.

What if I use:
#inspector-toolbar[treepanel-open] > vbox > .inspector-resizer,
#inspector-toolbar[treepanel-open] > vbox > hbox > .inspector-resizer {
 display: -moz-box;
}
Comment 17 Paul Rouget [:paul] 2011-11-01 02:04:48 PDT
(???, how did I manage to change all these fields?)
Comment 18 Dão Gottwald [:dao] 2011-11-01 02:17:24 PDT
> What if I use:
> #inspector-toolbar[treepanel-open] > vbox > .inspector-resizer,
> #inspector-toolbar[treepanel-open] > vbox > hbox > .inspector-resizer {
>  display: -moz-box;
> }

Why would you use .inspector-resizer instead of the ids there?
Comment 19 Paul Rouget [:paul] 2011-11-01 02:31:48 PDT
Created attachment 570951 [details] [diff] [review]
patch v1.3

(In reply to Dao Gottwald [:dao] from comment #18)
> Why would you use .inspector-resizer instead of the ids there?
No good reason.
Comment 20 Paul Rouget [:paul] 2011-11-01 02:38:04 PDT
Created attachment 570952 [details] [diff] [review]
patch v1.4

Rebase mistake.
Comment 21 Paul Rouget [:paul] 2011-11-01 09:07:28 PDT
Thank you for the r+ Dao.
Comment 22 Rob Campbell [:rc] (:robcee) 2011-11-01 10:39:06 PDT
https://hg.mozilla.org/integration/fx-team/rev/5b1c3d9c342b
Comment 23 Rob Campbell [:rc] (:robcee) 2011-11-01 13:04:13 PDT
added a fix for failing test:

https://hg.mozilla.org/integration/fx-team/rev/9f51745bdd6d
Comment 24 Tim Taubert [:ttaubert] 2011-11-02 03:02:20 PDT
https://hg.mozilla.org/mozilla-central/rev/9f51745bdd6d
Comment 25 Tim Taubert [:ttaubert] 2011-11-02 03:02:42 PDT
https://hg.mozilla.org/mozilla-central/rev/5b1c3d9c342b
Comment 26 Teodosia Pop 2011-11-25 05:23:10 PST
Verified fixed using
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:10.0a2) Gecko/20111124 Firefox/10.0a2
and
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a2) Gecko/20111123 Firefox/10.0a2

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