Closed Bug 511785 Opened 11 years ago Closed 11 years ago

Restyle the Places window toolbar

Categories

(Firefox :: Theme, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 3.7a1
Tracking Status
status1.9.2 --- wontfix

People

(Reporter: mstange, Assigned: mstange)

Details

(Keywords: memory-footprint)

Attachments

(1 file, 3 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
This patch is on top of bug 508940.

This patch does the following:
 - Get rid of the toolbar button background images and replace them with
   borders, gradients and box shadows.
 - Combine the toolbar button glyphs into one file, places/toolbar.png.
 - Share some of the round rect button stuff in shared.inc.
 - Fix some things for the console Clear button (text alignment and wrong
   inactive gradient).

For the pressed state, I had to use a little trick in order to get around bug 466572.
Dão, when you review this, please also review the new image. I don't know how to deal with color management and gAMA chunks, or how to tell whether they're included in that image.
Attached patch v2 (obsolete) — Splinter Review
use -moz-background-origin: border so that the left and right borders are subject to the same color mixing as the top and bottom borders
Attachment #395718 - Attachment is obsolete: true
Attached patch v2.1 (obsolete) — Splinter Review
Attachment #395762 - Attachment is obsolete: true
Attachment #395768 - Flags: review?(dao)
Comment on attachment 395768 [details] [diff] [review]
v2.1

>+#placesToolbar > toolbarbutton {
>+  list-style-image: url("chrome://browser/skin/places/toolbar.png");
>+  margin: 4px 6px 5px;
>+  padding: 1px 5px;
>+  -moz-border-radius: 100%;  
>+  border: 1px solid @roundRectButtonBorderColor@;
>+  -moz-box-shadow: @loweredShadow@;
>+  background: @roundRectButtonBackground@;
>+  -moz-background-origin: border;
> }
> 
>+#placesToolbar > toolbarbutton:not([disabled="true"]):active:hover,
>+#placesToolbar > toolbarbutton[open="true"] {
>+  background: rgba(0, 0, 0, 0.6);
>+  border-color: transparent;
>+  -moz-box-shadow: @roundRectButtonPressedInnerShadow@,
>+                   @loweredShadow@,
>+                   inset 0 0 0 20px @roundRectButtonPressedBackgroundColor@;
> }

Can you rename roundRectButtonPressedBackgroundColor to roundRectButtonPressedBackground and actually use it as background, as elsewhere? rgba(0, 0, 0, 0.6) could be used as an inset shadow if needed, as far as I can see.

Also, for things that are used for generic toolbar buttons as well as "roundRect" buttons, maybe do s/roundRect/toolbarbutton/?
(In reply to comment #3)
> Can you rename roundRectButtonPressedBackgroundColor to
> roundRectButtonPressedBackground and actually use it as background, as
> elsewhere? rgba(0, 0, 0, 0.6) could be used as an inset shadow if needed, as
> far as I can see.

That wouldn't achieve the same thing, would it? rgba(0, 0, 0, 0.6) needs to blend with the toolbar background and must not be influenced by roundRectButtonPressedBackgroundColor.

> Also, for things that are used for generic toolbar buttons as well as
> "roundRect" buttons, maybe do s/roundRect/toolbarbutton/?

toolbarbuttonButton? ;-P
Sure, I can do that.
Attached patch v3Splinter Review
Same patch with s/roundRectButton/toolbarbutton/
Attachment #395768 - Attachment is obsolete: true
Attachment #402226 - Flags: review?(dao)
Attachment #395768 - Flags: review?(dao)
Comment on attachment 402226 [details] [diff] [review]
v3

>+%define toolbarbuttonCornerRadius 3px

I think you actually want something like roundRect here, since the toolbar buttons in the main window have a different radius.
Attachment #402226 - Flags: review?(dao) → review+
I think I'll leave it toolbarbuttonCornerRadius, since it's the radius that toolbarbuttons are supposed to have. We only use the wrong radius in the Places window because we want to be consistent with the main browser window, which uses the wrong radius in order to look less similar to Safari.
http://hg.mozilla.org/mozilla-central/rev/50adf6bd6ed6
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a1
Attachment #402226 - Flags: approval1.9.2?
Comment on attachment 402226 [details] [diff] [review]
v3

Actually, we don't really need this on 1.9.2, and I'd have to merge around bug 508940...
Attachment #402226 - Flags: approval1.9.2?
Comment on attachment 402226 [details] [diff] [review]
v3

a192=beltzner, more last minute theme polish, what could go wrong? :)
Attachment #402226 - Flags: approval1.9.2+
You need to log in before you can comment on or make changes to this bug.