449442 - Use native-styled statusbar with gradient instead of css background-image

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Stefan [:stefanh]]

Comment 1

[Markus Stange [:mstange]]

Firefox's statusbar is actually a little strange.

On OS X, there's a difference between status bars and bottom bars. Bottom bars have a gradient and rounded bottom corners (see e.g. a Finder window); status bars have square corners and no gradient (see Safari).

I'd like to suggest drawing the native-styled statusbar (which this bug is about) with a solid color, the dark chrome grey. Later on we could add another -moz-appearance for bottom bars with gradients and rounded corners which would be used e.g. in the Download Manager.

Comment 2

[Markus Stange [:mstange]]

(In reply to comment #1)
> I'd like to suggest drawing the native-styled statusbar (which this bug is
> about) with a solid color, the dark chrome grey.

I've changed my mind. Let's keep the gradient.

Comment 3

[Markus Stange [:mstange]]

Attachment: render part, v1

This hunk might need explanation:

@@ -1837,18 +1904,18 @@ nsNativeThemeCocoa::GetWidgetBorder(nsID

-    case NS_THEME_MOZ_MAC_UNIFIED_TOOLBAR:
-      aResult->SizeTo(0, 0, 1, 0);
+    case NS_THEME_STATUSBAR:
+      aResult->SizeTo(0, 1, 0, 0);
       break;
   }

I'm removing the bogus widget border for the unified toolbar I added in bug 439354; a border on the right makes no sense. And a border on the bottom creates problems (e.g. the hidden "menubar" at the top of the window appears).

The statusbar gets a 1px top border, although a 2px top border is rendered. That's because the second top border shouldn't take space away from the content.

Comment 4

[Markus Stange [:mstange]]

Attachment: CSS part, v1

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 353438 [details] [diff] [review]
CSS part, v1

> #main-window:not([active="true"]) > #navigator-toolbox > toolbar > toolbaritem,
>-#main-window:not([active="true"]) > #navigator-toolbox > toolbar > toolbarbutton,
>-#main-window:not([active="true"]) > #browser-bottombox {
>+#main-window:not([active="true"]) > #navigator-toolbox > toolbar > toolbarbutton {
>   opacity: 0.75;
> }

What about the findbar as part of #browser-bottombox?

Comment 6

[Markus Stange [:mstange]]

That was a deliberate change; in native Mac apps, find bars don't fade.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think DrawStatusBar should handle the case where the status bar is 0 or 1px high by bailing out instead of doing something weird.

Also, can you explain why you changed your mind in comment #2?

Comment 8

[Markus Stange [:mstange]]

Attachment: render part, v2

Comment 9

[Markus Stange [:mstange]]

(In reply to comment #7)
> Also, can you explain why you changed your mind in comment #2?

This way I don't have to add another -moz-appearance for the bottom bar in the Downloads window :)
And I think the gradient looks better... but let's have Faaborg make that decision.

Comment 10

[Markus Stange [:mstange]]

Attachment: Screenshot: gradient-style statusbar

Comment 11

[Markus Stange [:mstange]]

Attachment: Screenshot: Safari-style statusbar

Alex, which one do you prefer?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 353532 [details] [diff] [review]
render part, v2

+  if (inBoxRect.size.height <= 1.0f)
+    return;

I think < 2.0f would be better.

Comment 13

[Stefan [:stefanh]]

fwiw, Camino uses a "gradient-style" statusbar.

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #13)
> fwiw, Camino uses a "gradient-style" statusbar.

At some point (I can't find it now) we had a discussion that we weren't using the "right" look for a status bar, but apparently we decided we didn't care.

Given that the "status bars" in Gecko apps often contain some form of control (via extensions and so forth), the bottom bar probably makes more sense.

Comment 15

[Stefan [:stefanh]]

> Given that the "status bars" in Gecko apps often contain some form of control
> (via extensions and so forth), the bottom bar probably makes more sense.

Yeah, the Safari-styled status bar would only make sense in one window (browser.xul). And a bottom bar actually looks better. Also, going for the Safari-styled status bar would probably mean that the css style rules should go in browser/themes/ rather than toolkit/themes/global.

Comment 16

[Alex Faaborg [:faaborg] (Firefox UX)]

I think users probably won't be able to catch the subtle difference between if something should be a bottom bar (gradient, rounded corners) and a native status bar (no gradient, square corners).  And since bottom bars seem to appear in the OS considerably more often then status bars (is Safari the only example?) this might be a case where doing the wrong thing ultimately looks more right than doing the right thing.  So, by that logic we should use a bottom bar style on a bar that is only as tall as a status bar.

Overall though, I really would like for us to eventually remove the status bar (if no extensions have modified it), since that seems more respectful to the content area, and is visually cleaner (bug 425584).

Comment 17

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 353535 [details]
Screenshot: gradient-style statusbar

Sorry, forgot to set the review flags back on 12/18, see the comment above for rationale.

Comment 18

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 353537 [details]
Screenshot: Safari-style statusbar

Sorry, forgot to set the review flags back on 12/18, see comment #16 for
rationale.

Comment 19

[Josh Aas]

Comment on attachment 353532 [details] [diff] [review]
render part, v2

>+  GreyGradientInfo* info = (GreyGradientInfo*)aInfo;

new-style static_cast please

>+  CGShadingRef shading = CGShadingCreateAxial(colorSpace,
>+                                              CGPointMake(0, CGRectGetMinY(rect)),
>+                                              CGPointMake(0, CGRectGetMaxY(rect)),
>+                                              function, NO, NO);

Please use "false" instead of "NO" because this is C not Obj-C (bool vs. BOOL).

r+ but please post a new patch for whatever you check in

Comment 20

[Markus Stange [:mstange]]

Attachment: render part, v2.1, for checkin

Comment 21

[Markus Stange [:mstange]]

Checked in:
http://hg.mozilla.org/mozilla-central/rev/e666dabedee5
http://hg.mozilla.org/mozilla-central/rev/c7538401b1c9

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 353532 [details] [diff] [review]
render part, v2

a191=beltzner; doesn't look like there was a perf hit from doing this on trunk, but please watch for it carefully when you land on branch

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 355572 [details] [diff] [review]
render part, v2.1, for checkin

a191=beltzner

Comment 24

[Markus Stange [:mstange]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d76f278e4c6c
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ee511803c7d6

Comment 25

[Markus Stange [:mstange]]

(In reply to comment #22)
> (From update of attachment 353532 [details] [diff] [review])
> a191=beltzner; doesn't look like there was a perf hit from doing this on trunk,
> but please watch for it carefully when you land on branch

It looks like the landing even resulted in a slight perf improvement, or at least the numbers are more stable now:

>http://graphs-new.mozilla.org/graph.html#tests=[{%22test%22:%2217%22,%22branch%22:%223%22,%22machine%22:%2236%22},{%22test%22:%2217%22,%22branch%22:%223%22,%22machine%22:%2237%22},{%22test%22:%2217%22,%22branch%22:%223%22,%22machine%22:%2238%22},{%22test%22:%2217%22,%22branch%22:%223%22,%22machine%22:%2239%22},{%22test%22:%2217%22,%22branch%22:%223%22,%22machine%22:%2249%22}]&sel=1238206294,1238442360