OS X Lion introduces a new toolbar button style and a new button state for hovering over buttons in an inactive window. Firefox’s toolbar buttons should be updated to match this new style.

What does this look like on split buttons (i.e. back/forward)?

The one in Safari is similar although I think narrower.

This patch makes us use -moz-appearance: toolbarbutton (from bug 672050) in all the places where we used to recreate this button style with CSS. And because using Lion-style buttons with a Leopard-style keyhole circle looks bad, I've added a Lion-style keyhole circle. Explanation of non-obvious changes: - -moz-appearance: toolbarbutton looks at adjacent buttons in order to decide whether to use rounded corners or separators. It doesn't consider -moz-transform, so we can't flip adjacent buttons with scaleX(-1) in RTL mode. We can only do it when the flipped button stands alone, e.g. the forward button in large icons mode. - The non-lwtheme keyhole forward button gets the keyhole forward mask applied, too, because the back button circle is now transparent. - That's why we also scaleX(-1) it in non-lwtheme RTL mode, because otherwise we'd have to create a flipped mask. - The mask needs to be shifted because the forward button now has more overlap with the back button because we can't turn off its rounded corners. I'm undecided about how to apply Lion-only theme parts. As I see it, we have two choices: We could use jar/manifest-style file-based platform switching, or we could add a -moz-system-metric(mac-lion-theme). This patch uses the former, but I'm thinking the latter might make more sense, even if only to prevent increasing the omni.jar size. The specific differences on Lion are (going to be): (1) different Toolbar.png, lighter glyphs (2) lighter glpyhs for many other toolbar / tabbar icons (3) different keyhole back button circle (4) different background window behavior (not present in this patch): button:not(:hover) > .icon:-moz-window-inactive { opacity: 0.8; } (5) different urlbar / searchbar inner shadow (6) different text color In this patch I'm handling (1) and (3) via jar platform switching. (6) should probably be implemented outside CSS via a platform color. (4) and (5) are the things I'd prefer a system metric for, instead of a separate CSS file. But for (2), file-based jar switching would be more comfortable than specifying everything twice in the CSS. And once we've done the jar.mn doubling, we might as well use it for (1) and (3), too. Dão, what's your opinion on this?

Tryserver build is here (also contains other patches): https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mstange@themasta.com-d728455128c0/try-macosx64/firefox-8.0a1.en-US.mac.dmg

Comment on attachment 549348 [details] [diff] [review] v1 >--- a/browser/themes/pinstripe/browser/browser.css >+++ b/browser/themes/pinstripe/browser/browser.css > .toolbarbutton-1:not([type="menu-button"]), > .toolbarbutton-1 > .toolbarbutton-menubutton-button, > .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker, > #restore-button { >+ -moz-appearance: toolbarbutton; I guess this can't be done in toolbarbutton.css as it shouldn't apply to toolbar buttons with labels? That's kind of sad. >--- a/browser/themes/pinstripe/browser/jar.mn >+++ b/browser/themes/pinstripe/browser/jar.mn >+# NOTE: If you add a new file here, you'll need to add it to the lion >+# section at the bottom of this file Please take the same approach as Mike in bug 659407.

(In reply to comment #5) > Comment on attachment 549348 [details] [diff] [review] [diff] [details] [review] > v1 > > >--- a/browser/themes/pinstripe/browser/browser.css > >+++ b/browser/themes/pinstripe/browser/browser.css > > > .toolbarbutton-1:not([type="menu-button"]), > > .toolbarbutton-1 > .toolbarbutton-menubutton-button, > > .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker, > > #restore-button { > > >+ -moz-appearance: toolbarbutton; > > I guess this can't be done in toolbarbutton.css as it shouldn't apply to > toolbar buttons with labels? That's kind of sad. Right. If the expectation for <toolbarbutton> is that it looks good with arbitrary contents and at arbitrary sizes then using -moz-appearance: toolbarbutton for them won't make anybody happy. > >--- a/browser/themes/pinstripe/browser/jar.mn > >+++ b/browser/themes/pinstripe/browser/jar.mn > > >+# NOTE: If you add a new file here, you'll need to add it to the lion > >+# section at the bottom of this file > > Please take the same approach as Mike in bug 659407. Done.

Comment on attachment 549681 [details] [diff] [review] v2 >--- a/browser/themes/pinstripe/browser/jar.mn >+++ b/browser/themes/pinstripe/browser/jar.mn >+#ifdef XP_MACOSX This seems unnecessary. >--- a/toolkit/themes/pinstripe/global/console/console.css >+++ b/toolkit/themes/pinstripe/global/console/console.css > #Console\:clear { >- text-shadow: @loweredShadow@; >+ text-shadow: @loweredShadow@ !important; Why is this change needed?

(In reply to comment #7) > Comment on attachment 549681 [details] [diff] [review] [diff] [details] [review] > v2 > > >--- a/browser/themes/pinstripe/browser/jar.mn > >+++ b/browser/themes/pinstripe/browser/jar.mn > > >+#ifdef XP_MACOSX > > This seems unnecessary. That's what Mike does in bug 659407 for area Windows files. Is that only done on Windows because the Linux theme is built on top of the Windows theme? > >--- a/toolkit/themes/pinstripe/global/console/console.css > >+++ b/toolkit/themes/pinstripe/global/console/console.css > > > #Console\:clear { > >- text-shadow: @loweredShadow@; > >+ text-shadow: @loweredShadow@ !important; > > Why is this change needed? It's not needed, good catch. I can remove the !important in all the places where the button selector is more specific than toolbarbutton:not([disabled="true"]):active:hover.

> That's what Mike does in bug 659407 for area Windows files. Is that only > done on Windows because the Linux theme is built on top of the Windows theme? Linux only builds upon winstripe in toolkit, but OS/2 always builds winstripe directly.

Comment on attachment 549767 [details] [diff] [review] v3 You can remove toolbarbuttonFocusedBorderColorAqua and toolbarbuttonFocusedBorderColorGraphite in shared.inc as well.

(In reply to Markus Stange from comment #3) > - -moz-appearance: toolbarbutton looks at adjacent buttons in order to > decide > whether to use rounded corners or separators. It doesn't consider > -moz-transform, so we can't flip adjacent buttons with scaleX(-1) in RTL > mode. We can only do it when the flipped button stands alone, e.g. the > forward button in large icons mode. Can you just flip the icon?

Comment on attachment 549767 [details] [diff] [review] v3 >--- a/browser/themes/pinstripe/browser/browser.css >+++ b/browser/themes/pinstripe/browser/browser.css > .toolbarbutton-1:not([type="menu-button"]), > .toolbarbutton-1 > .toolbarbutton-menubutton-button, > .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker, > #restore-button { > -moz-box-orient: vertical; >- padding: 0 3px; >+ -moz-appearance: toolbarbutton; > height: 22px; >- border: 1px solid @toolbarbuttonBorderColor@; >- border-radius: @toolbarbuttonCornerRadius@; >- box-shadow: 0 1px rgba(255, 255, 255, 0.2); >- background: @toolbarbuttonBackground@; >- background-origin: border-box; >+ padding: 0; >+ border: 0; > } border: 0; looks like a no-op here

(In reply to Dão Gottwald [:dao] from comment #12) > (In reply to Markus Stange from comment #3) > > - -moz-appearance: toolbarbutton looks at adjacent buttons in order to > > decide > > whether to use rounded corners or separators. It doesn't consider > > -moz-transform, so we can't flip adjacent buttons with scaleX(-1) in RTL > > mode. We can only do it when the flipped button stands alone, e.g. the > > forward button in large icons mode. > > Can you just flip the icon? No, or I'd have to create a flipped mask. See: (In reply to Markus Stange from comment #3) > - The non-lwtheme keyhole forward button gets the keyhole forward mask applied, > too, because the back button circle is now transparent. > - That's why we also scaleX(-1) it in non-lwtheme RTL mode, because otherwise > we'd have to create a flipped mask. (In reply to Dão Gottwald [:dao] from comment #13) > Comment on attachment 549767 [details] [diff] [review] [diff] [details] [review] > v3 > > >--- a/browser/themes/pinstripe/browser/browser.css > >+++ b/browser/themes/pinstripe/browser/browser.css > > > .toolbarbutton-1:not([type="menu-button"]), > > .toolbarbutton-1 > .toolbarbutton-menubutton-button, > > .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker, > > #restore-button { > > -moz-box-orient: vertical; > >- padding: 0 3px; > >+ -moz-appearance: toolbarbutton; > > height: 22px; > >- border: 1px solid @toolbarbuttonBorderColor@; > >- border-radius: @toolbarbuttonCornerRadius@; > >- box-shadow: 0 1px rgba(255, 255, 255, 0.2); > >- background: @toolbarbuttonBackground@; > >- background-origin: border-box; > >+ padding: 0; > >+ border: 0; > > } > > border: 0; looks like a no-op here It's simpler that way. toolbarbutton.css sets a default transparent border on the left and right side (and I don't want to mess with that now), so I'd definitely need to unset it for the circle back button, but then the specificity for the circle back button selector would turn off the border for it in lwtheme mode, too.

Comment on attachment 549767 [details] [diff] [review] v3 The back/forward button code is really nasty :/ Comment 11 still needs to be addressed.

This looks amazing, only the rounded back-button seems a little bit to light for me somehow!

Yeah, that one still needs tweaking. Feel free to file a bug about it. Maybe you can even create a new browser/themes/pinstripe/browser/keyhole-circle-lion.png that looks correct :)

(In reply to Markus Stange from comment #19) > Yeah, that one still needs tweaking. Feel free to file a bug about it. Maybe > you can even create a new > browser/themes/pinstripe/browser/keyhole-circle-lion.png that looks correct > :) I checked that image in Photoshop but it looks different than what is being displayed in browser.

Strange. Sure that it's not due to transparency?

(In reply to Markus Stange from comment #21) > Strange. Sure that it's not due to transparency? I don't think so. It's like it is being slightly clipped or scaled.

Comment on attachment 553782 [details] Back Button Comparison What is this image comparing? What's the other circle made of?

(In reply to Markus Stange from comment #23) > Comment on attachment 553782 [details] > Back Button Comparison > > What is this image comparing? What's the other circle made of? I pasted the circle from keyhole-circle-lion.png onto a screenshot of the browser window.

Oh, I see. No idea. Maybe it's better to drop the image and recreate the circle with CSS again. Want to do that?

(In reply to Markus Stange from comment #25) > Oh, I see. No idea. > > Maybe it's better to drop the image and recreate the circle with CSS again. > Want to do that? Yes, I will file a new bug.

(In reply to Stephen Horlander from comment #26) > (In reply to Markus Stange from comment #25) > > Oh, I see. No idea. > > > > Maybe it's better to drop the image and recreate the circle with CSS again. > > Want to do that? > > Yes, I will file a new bug. Actually I don't know that we have to use CSS, the image should work. I will try some things.

(In reply to Markus Stange from comment #25) > Oh, I see. No idea. > > Maybe it's better to drop the image and recreate the circle with CSS again. > Want to do that? Filed bug 679771. The border-radius was the cause of the clipping and the color shift was caused by the image color mode being Greyscale.

caused regression Bug 702558?