546874 - New style for the bookmarks bar

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Markus Stange [:mstange]]

Attachment: v1

- Remove the top border and the gradient
- Use the Cocoa recessed scope bar button style for the buttons

like in the mockup in the URL field.

Tryserver build: https://build.mozilla.org/tryserver-builds/[e-mail]-try-ee1d1d645d59/try-ee1d1d645d59-macosx.dmg

Comment 1

[Dão Gottwald [:dao]]

Comment on attachment 427545 [details] [diff] [review]
v1

>+toolbarbutton.bookmark-item:hover,
>+toolbarbutton.bookmark-item[open="true"],
>+#navigator-toolbox toolbarbutton:-moz-lwtheme:hover,
>+#navigator-toolbox toolbarbutton:-moz-lwtheme[open="true"] {
>+  color: #FFF !important;
>+  text-shadow: 0 1px rgba(0, 0, 0, .4) !important;
>+}

Why is #navigator-toolbox toolbarbutton:-moz-lwtheme part of this patch? Why under the /* ----- BOOKMARK BUTTONS ----- */ heading?

>+toolbarbutton.bookmark-item-microsummarized {
>+  max-width: 20em;
>+}

this appears to be a dead class

>+.bookmark-item > .toolbarbutton-icon {
>+  margin: 0px;
>+  display: none !important;
> }

the margin seems superfluous

Comment 2

[Dão Gottwald [:dao]]

Btw, why isn't this using -moz-appearance:toolbar?

Comment 3

[Markus Stange [:mstange]]

(In reply to comment #1)
> (From update of attachment 427545 [details] [diff] [review])
> >+toolbarbutton.bookmark-item:hover,
> >+toolbarbutton.bookmark-item[open="true"],
> >+#navigator-toolbox toolbarbutton:-moz-lwtheme:hover,
> >+#navigator-toolbox toolbarbutton:-moz-lwtheme[open="true"] {
> >+  color: #FFF !important;
> >+  text-shadow: 0 1px rgba(0, 0, 0, .4) !important;
> >+}
> 
> Why is #navigator-toolbox toolbarbutton:-moz-lwtheme part of this patch?

This was supposed to be
#navigator-toolbox toolbarbutton.bookmark-item:-moz-lwtheme
so that I have a selector with enough specificity to override the color: inherit !important from #navigator-toolbox toolbarbutton:-moz-lwtheme. But I can just remove the !important from that rule.

(In reply to comment #2)
> Btw, why isn't this using -moz-appearance:toolbar?

-moz-appearance: toolbar has a top border that we don't want.

Comment 4

[Markus Stange [:mstange]]

Attachment: v2

Comment 5

[Dão Gottwald [:dao]]

(In reply to comment #3)
> -moz-appearance: toolbar has a top border that we don't want.

Do we normally want a top border? If not, why does it have a top border?

Comment 6

[Markus Stange [:mstange]]

Attachment: toolbars in DOM inspector

(In reply to comment #5)
> (In reply to comment #3)
> > -moz-appearance: toolbar has a top border that we don't want.
> 
> Do we normally want a top border?

Yes, I think we do. It's a little hard to say because we don't have any non-unified toolbars in Firefox (apart from the bookmarks bar and a scope bar in the Places library window which has a different styling) and because I don't know of any native Cocoa app that we could use for inspiration - they all have either only one toolbar (which is unified) or the lower toolbar is a scope bar (which has a lighter gradient). The only exception is Safari: Safari 3 had a dark bookmarks bar which was separated with a "groove" border and Safari 4 got rid of that separator border completely (which is what we're doing with this patch, too).

So the question is whether extension windows that stack more than one toolbar on top of each other look better with the lighter top border. I think they do. See for example the DOM Inspector screenshot.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 427565 [details] [diff] [review]
v2

> #PersonalToolbar {
>   -moz-appearance: none;
>-  background: url("chrome://browser/skin/bookmark_toolbar_background.png") repeat-x center center -moz-mac-chrome-active;
>-  border-top: 1px solid rgba(255, 255, 255, 0.4);
>+  margin-top: -1px;
>+  background-color: -moz-mac-chrome-active;
>   border-bottom: 1px solid rgba(0, 0, 0, 0.57);
>+  min-height: 22px;
>+  padding: 0 4px 3px;
>+  -moz-box-align: center;
> }

Add a comment for the negative margin? I guess it overlays a toolbar border.

>-toolbarbutton.bookmark-item-microsummarized {
>-  max-width: 20em;
>-}

Please remove this from winstripe while you're at it.

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 427565 [details] [diff] [review]
v2

https://build.mozilla.org/tryserver-builds/[e-mail]-try-7193e6f05f2d/try-7193e6f05f2d-macosx.dmg

Comment 9

[Markus Stange [:mstange]]

http://hg.mozilla.org/mozilla-central/rev/4dc4f0609c23

Comment 10

[d]

I think this might have had an unwanted effect, where the bookmarks will stretch as far as the title of the page, as in, they are not shortened and the end replace with three dots (...). Was this intended, or is it something we don't want to do?

Comment 11

[Markus Stange [:mstange]]

Good catch, this was not intentional. Filed bug 548017.