Closed Bug 546874 Opened 10 years ago Closed 10 years ago

New style for the bookmarks bar

Categories

(Firefox :: Theme, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 3.7a2

People

(Reporter: mstange, Assigned: mstange)

References

()

Details

Attachments

(2 files, 1 obsolete file)

Attached patch v1 (obsolete) — Splinter Review
- 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/mstange@themasta.com-try-ee1d1d645d59/try-ee1d1d645d59-macosx.dmg
Attachment #427545 - Flags: ui-review?(shorlander)
Attachment #427545 - Flags: review?(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
Btw, why isn't this using -moz-appearance:toolbar?
(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.
Attached patch v2Splinter Review
Attachment #427545 - Attachment is obsolete: true
Attachment #427565 - Flags: review?(dao)
Attachment #427545 - Flags: ui-review?(shorlander)
Attachment #427545 - Flags: review?(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?
(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 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.
Attachment #427565 - Flags: review?(dao) → review+
http://hg.mozilla.org/mozilla-central/rev/4dc4f0609c23
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a2
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?
Depends on: 548017
Good catch, this was not intentional. Filed bug 548017.
Depends on: 551582
Attachment #427565 - Flags: ui-review?(shorlander)
You need to log in before you can comment on or make changes to this bug.