Last Comment Bug 667480 - [10.7] Update Firefox’s Toolbar Button Style for OS X Lion
: [10.7] Update Firefox’s Toolbar Button Style for OS X Lion
Status: RESOLVED FIXED
: addon-compat
Product: Firefox
Classification: Client Software
Component: Theme (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- normal with 3 votes (vote)
: Firefox 8
Assigned To: Markus Stange [:mstange]
:
Mentors:
Depends on: 1074946 672050 678333 679708 679771 702558
Blocks: lion-compatibility 667456 683414
  Show dependency treegraph
 
Reported: 2011-06-27 09:10 PDT by Dominic Spitaler
Modified: 2014-09-30 09:10 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
affected


Attachments
Toolbar Button States in OS X Lion (10.88 KB, image/png)
2011-06-27 09:10 PDT, Dominic Spitaler
no flags Details
Split button states from Finder (8.00 KB, image/png)
2011-06-29 10:44 PDT, Dominic Spitaler
no flags Details
v1 (51.07 KB, patch)
2011-07-29 05:46 PDT, Markus Stange [:mstange]
dao+bmo: review-
Details | Diff | Splinter Review
v2 (39.28 KB, patch)
2011-07-31 12:52 PDT, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
v3 (39.03 KB, patch)
2011-08-01 06:12 PDT, Markus Stange [:mstange]
dao+bmo: review+
Details | Diff | Splinter Review
Back Button Comparison (24.59 KB, image/png)
2011-08-17 08:27 PDT, Stephen Horlander [:shorlander]
no flags Details

Description Dominic Spitaler 2011-06-27 09:10:08 PDT
Created attachment 542174 [details]
Toolbar Button States in OS X Lion

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.
Comment 1 Markus Stange [:mstange] 2011-06-27 10:34:38 PDT
What does this look like on split buttons (i.e. back/forward)?
Comment 2 Dominic Spitaler 2011-06-29 10:44:21 PDT
Created attachment 542863 [details]
Split button states from Finder

The one in Safari is similar although I think narrower.
Comment 3 Markus Stange [:mstange] 2011-07-29 05:46:21 PDT
Created attachment 549348 [details] [diff] [review]
v1

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?
Comment 4 Markus Stange [:mstange] 2011-07-29 05:47:41 PDT
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 5 Dão Gottwald [:dao] 2011-07-29 09:21:30 PDT
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.
Comment 6 Markus Stange [:mstange] 2011-07-31 12:52:04 PDT
Created attachment 549681 [details] [diff] [review]
v2

(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 7 Dão Gottwald [:dao] 2011-08-01 05:23:23 PDT
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?
Comment 8 Markus Stange [:mstange] 2011-08-01 05:35:22 PDT
(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.
Comment 9 Dão Gottwald [:dao] 2011-08-01 05:38:53 PDT
> 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 10 Markus Stange [:mstange] 2011-08-01 06:12:22 PDT
Created attachment 549767 [details] [diff] [review]
v3
Comment 11 Stefan [:stefanh] 2011-08-04 13:31:21 PDT
Comment on attachment 549767 [details] [diff] [review]
v3

You can remove toolbarbuttonFocusedBorderColorAqua and toolbarbuttonFocusedBorderColorGraphite in shared.inc as well.
Comment 12 Dão Gottwald [:dao] 2011-08-05 01:48:05 PDT
(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 13 Dão Gottwald [:dao] 2011-08-05 01:48:49 PDT
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
Comment 14 Markus Stange [:mstange] 2011-08-05 02:50:48 PDT
(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 15 Dão Gottwald [:dao] 2011-08-06 12:24:21 PDT
Comment on attachment 549767 [details] [diff] [review]
v3

The back/forward button code is really nasty :/

Comment 11 still needs to be addressed.
Comment 18 Dominic Spitaler 2011-08-17 07:42:19 PDT
This looks amazing, only the rounded back-button seems a little bit to light for me somehow!
Comment 19 Markus Stange [:mstange] 2011-08-17 07:48:06 PDT
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 :)
Comment 20 Stephen Horlander [:shorlander] 2011-08-17 08:19:12 PDT
(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.
Comment 21 Markus Stange [:mstange] 2011-08-17 08:22:49 PDT
Strange. Sure that it's not due to transparency?
Comment 22 Stephen Horlander [:shorlander] 2011-08-17 08:27:27 PDT
Created attachment 553782 [details]
Back Button Comparison

(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 23 Markus Stange [:mstange] 2011-08-17 08:31:18 PDT
Comment on attachment 553782 [details]
Back Button Comparison

What is this image comparing? What's the other circle made of?
Comment 24 Stephen Horlander [:shorlander] 2011-08-17 08:33:47 PDT
(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.
Comment 25 Markus Stange [:mstange] 2011-08-17 08:40:53 PDT
Oh, I see. No idea.

Maybe it's better to drop the image and recreate the circle with CSS again.
Want to do that?
Comment 26 Stephen Horlander [:shorlander] 2011-08-17 08:41:37 PDT
(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.
Comment 27 Stephen Horlander [:shorlander] 2011-08-17 08:42:45 PDT
(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.
Comment 28 Stephen Horlander [:shorlander] 2011-08-17 10:27:26 PDT
(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.
Comment 29 Carsten Book [:Tomcat] 2011-11-15 07:07:20 PST
caused regression Bug 702558?

Note You need to log in before you can comment on or make changes to this bug.