[10.7] Update Firefox’s Toolbar Button Style for OS X Lion

RESOLVED FIXED in Firefox 8

Status

()

Firefox
Theme
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: Dominic Spitaler, Assigned: mstange)

Tracking

(Depends on: 1 bug, Blocks: 3 bugs, {addon-compat})

Trunk
Firefox 8
x86_64
Mac OS X
addon-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox6 affected, firefox7 affected)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Blocks: 667456
(Reporter)

Updated

6 years ago
No longer blocks: 667456
(Reporter)

Updated

6 years ago
Blocks: 667456

Updated

6 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 1

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

Comment 2

6 years ago
Created attachment 542863 [details]
Split button states from Finder

The one in Safari is similar although I think narrower.
(Assignee)

Updated

6 years ago
Depends on: 672050

Updated

6 years ago
Blocks: 636455
(Assignee)

Comment 3

6 years ago
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?
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Attachment #549348 - Flags: review?(dao)
(Assignee)

Comment 4

6 years ago
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.
Attachment #549348 - Flags: review?(dao) → review-
(Assignee)

Comment 6

6 years ago
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.
Attachment #549348 - Attachment is obsolete: true
Attachment #549681 - Flags: review?(dao)
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?
(Assignee)

Comment 8

6 years ago
(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.
(Assignee)

Comment 10

6 years ago
Created attachment 549767 [details] [diff] [review]
v3
Attachment #549681 - Attachment is obsolete: true
Attachment #549681 - Flags: review?(dao)
Attachment #549767 - Flags: review?(dao)

Comment 11

6 years ago
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
(Assignee)

Comment 14

6 years ago
(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.
Attachment #549767 - Flags: review?(dao) → review+
(Assignee)

Comment 16

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/764a4259b9f7
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/c878992c72dc
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → Firefox 8
(Assignee)

Updated

6 years ago
Depends on: 678333
(Reporter)

Comment 18

6 years ago
This looks amazing, only the rounded back-button seems a little bit to light for me somehow!
(Assignee)

Comment 19

6 years ago
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 :)
Depends on: 679708
(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.
(Assignee)

Comment 21

6 years ago
Strange. Sure that it's not due to transparency?
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.
(Assignee)

Comment 23

6 years ago
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.
(Assignee)

Comment 25

6 years ago
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.
Depends on: 679771
(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.

Updated

6 years ago
status-firefox6: --- → affected
status-firefox7: --- → affected
Blocks: 683414

Updated

6 years ago
Keywords: addon-compat
caused regression Bug 702558?

Updated

6 years ago
Depends on: 702558

Updated

3 years ago
Depends on: 1074946
You need to log in before you can comment on or make changes to this bug.