Closed Bug 676253 Opened 8 years ago Closed 8 years ago

Make the Highlighter Toolbar look like faaborg's design on OS X

Categories

(DevTools :: General, defect, P2)

x86
macOS
defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 9

People

(Reporter: rcampbell, Assigned: paul)

References

Details

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

Attachments

(3 files, 3 obsolete files)

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
Whiteboard: [minotaur][best: 1d; likely: 3d: worst: 1w]
Priority: -- → P2
Assignee: nobody → paul
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?
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.
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #557177 - Flags: review?(rcampbell)
Attachment #557177 [details] [diff] is only for the toolbar and the buttons inside. No breadcrumbs or layout changes included.
Attached image Screenshot (v1) (obsolete) —
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"
Attachment #557177 - Flags: review?(rcampbell) → review-
Blocks: 672006
Status: NEW → ASSIGNED
(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
Attached patch patch v1.1Splinter Review
Addressed Dao's comments. Removed some useless properties.
Attachment #557180 - Attachment is obsolete: true
Attachment #557482 - Flags: review?(dao)
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
Attachment #557482 - Flags: review?(dao) → review+
Attachment #557177 - Attachment is obsolete: true
Thank you Dao.
Whiteboard: [minotaur][best: 1d; likely: 3d: worst: 1w] → [minotaur][land-in-fx-team]
Whiteboard: [minotaur][land-in-fx-team] → [minotaur][fixed-in-fx-team]
Comment on attachment 557484 [details] [diff] [review]
[checked-in] patch v1.2

http://hg.mozilla.org/integration/fx-team/rev/cefb66e24059
Attachment #557484 - Attachment description: patch v1.2 → [in-fx-team] patch v1.2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Comment on attachment 557484 [details] [diff] [review]
[checked-in] patch v1.2

http://hg.mozilla.org/mozilla-central/rev/cefb66e24059
Attachment #557484 - Attachment description: [in-fx-team] patch v1.2 → [checked-in] patch v1.2
Verified Fixed using Build identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:10.0a1) Gecko/20111020 Firefox/10.0a1.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.