Last Comment Bug 676253 - Make the Highlighter Toolbar look like faaborg's design on OS X
: Make the Highlighter Toolbar look like faaborg's design on OS X
Status: VERIFIED FIXED
[minotaur][fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: x86 Mac OS X
: P2 normal (vote)
: Firefox 9
Assigned To: Paul Rouget [:paul]
:
Mentors:
Depends on:
Blocks: 672006
  Show dependency treegraph
 
Reported: 2011-08-03 07:42 PDT by Rob Campbell [:rc] (:robcee)
Modified: 2011-10-21 06:15 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Toolbar Design Specs - i01 (648.77 KB, image/jpeg)
2011-08-29 10:23 PDT, Stephen Horlander [:shorlander]
no flags Details
Toolbar Design Specs - i02 (648.08 KB, image/jpeg)
2011-08-30 13:13 PDT, Stephen Horlander [:shorlander]
no flags Details
patch v1 (2.29 KB, patch)
2011-08-31 08:10 PDT, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Review
Screenshot (v1) (162.46 KB, image/png)
2011-08-31 08:16 PDT, Paul Rouget [:paul]
no flags Details
patch v1.1 (2.16 KB, patch)
2011-09-01 06:35 PDT, Paul Rouget [:paul]
dao+bmo: review+
Details | Diff | Review
[checked-in] patch v1.2 (2.16 KB, patch)
2011-09-01 06:54 PDT, Paul Rouget [:paul]
no flags Details | Diff | Review

Description Rob Campbell [:rc] (:robcee) 2011-08-03 07:42:34 PDT
The highlighter toolbar should have the white on-black styling designed by Alex Faaborg on OS X.

see: http://people.mozilla.com/~faaborg/files/projects/webDeveloper/web%20developer%20toolbar%20i1.png
Comment 1 Stephen Horlander [:shorlander] 2011-08-29 10:23:27 PDT
Created attachment 556597 [details]
Toolbar Design Specs - i01
Comment 2 Paul Rouget [:paul] 2011-08-30 03:12:38 PDT
Questions:
- the border-top of the toolbars is transparent (.65), what is supposed to be behind this border? The content page? The toolbar? 
- What if the breadcrumbs bar is hidden? Should we keep the dark border-top?
- Is it ok to use an image for the ">" shape betweens the buttons? Or it is better to use a 45deg-rotated block element?
Comment 3 Paul Rouget [:paul] 2011-08-30 07:54:21 PDT
Replying to my own questions,

(In reply to Paul Rouget [:paul] from comment #2)
> Questions:
> - the border-top of the toolbars is transparent (.65), what is supposed to
> be behind this border? The content page? The toolbar?

The toolbar of course :)

> - What if the breadcrumbs bar is hidden? Should we keep the dark border-top?

Yes. But this will only happen when we will have the developer toolbar, so not a problem here.

> - Is it ok to use an image for the ">" shape betweens the buttons? Or it is
> better to use a 45deg-rotated block element?

Using a 45deg-rotated block element works, but the CSS code is very ugly.
Stephen, do you think you can provide an image where the right part is transparent?
I would need 2 version of it, when the element is selected, and when it's not selected.
Comment 4 Stephen Horlander [:shorlander] 2011-08-30 13:13:15 PDT
Created attachment 556968 [details]
Toolbar Design Specs - i02
Comment 5 Paul Rouget [:paul] 2011-08-31 08:10:32 PDT
Created attachment 557177 [details] [diff] [review]
patch v1
Comment 6 Paul Rouget [:paul] 2011-08-31 08:12:11 PDT
Attachment #557177 [details] [diff] is only for the toolbar and the buttons inside. No breadcrumbs or layout changes included.
Comment 7 Paul Rouget [:paul] 2011-08-31 08:16:33 PDT
Created attachment 557180 [details]
Screenshot (v1)
Comment 8 Dão Gottwald [:dao] 2011-08-31 08:22:21 PDT
Comment on attachment 557177 [details] [diff] [review]
patch v1

>+#inspector-toolbar {

>+  padding-left: 6px;

this doesn't look RTL-ready

>+#inspector-inspect-toolbutton,
>+#inspector-tools > toolbarbutton {

>+  margin: 3px 10px 3px 0;

this doesn't look RTL-ready either

>+  border-radius: 3px;

use @toolbarbuttonCornerRadius@

>+  font-size: 12px;

Where does this number come from? Can you use a relative unit?

>+#inspector-inspect-toolbutton:not([checked="true"]):hover:active,
>+#inspector-tools > toolbarbutton:not([checked="true"]):hover:active {

nit: remove ="true"

>+  border: 1px solid hsla(210,8%,5%,.6);

use border-color

>+#inspector-inspect-toolbutton[checked="true"],
>+#inspector-tools > toolbarbutton[checked="true"] {

nit: remove ="true"

>+  border: 1px solid hsla(210,8%,5%,.6);

border-color again

>+  color: hsl(208,100%,60%)!important;

Why !important?

>+#inspector-inspect-toolbutton[checked="true"]:hover:active,
>+#inspector-tools > toolbarbutton[checked="true"]:hover:active {

nit: remove ="true"
Comment 9 Paul Rouget [:paul] 2011-09-01 06:25:28 PDT
(In reply to Dão Gottwald [:dao] from comment #8)
> >+  color: hsl(208,100%,60%)!important;
> 
> Why !important?

Because of this:
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/pinstripe/global/toolbarbutton.css#79
Comment 10 Paul Rouget [:paul] 2011-09-01 06:35:15 PDT
Created attachment 557482 [details] [diff] [review]
patch v1.1

Addressed Dao's comments. Removed some useless properties.
Comment 11 Dão Gottwald [:dao] 2011-09-01 06:46:52 PDT
Comment on attachment 557482 [details] [diff] [review]
patch v1.1

>+#inspector-inspect-toolbutton[checked],
>+#inspector-tools > toolbarbutton[checked] {
>+  color: hsl(208,100%,60%)!important;

nit: please add a space before !important
Comment 12 Paul Rouget [:paul] 2011-09-01 06:54:15 PDT
Created attachment 557484 [details] [diff] [review]
[checked-in] patch v1.2

Thank you Dao.
Comment 13 Rob Campbell [:rc] (:robcee) 2011-09-01 07:48:00 PDT
Comment on attachment 557484 [details] [diff] [review]
[checked-in] patch v1.2

http://hg.mozilla.org/integration/fx-team/rev/cefb66e24059
Comment 14 Rob Campbell [:rc] (:robcee) 2011-09-02 05:36:04 PDT
Comment on attachment 557484 [details] [diff] [review]
[checked-in] patch v1.2

http://hg.mozilla.org/mozilla-central/rev/cefb66e24059
Comment 15 Teodosia Pop 2011-10-21 06:15:31 PDT
Verified Fixed using Build identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:10.0a1) Gecko/20111020 Firefox/10.0a1.

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