Closed Bug 449442 Opened 12 years ago Closed 11 years ago

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

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: stefanh, Assigned: mstange)

References

Details

(Keywords: fixed1.9.1)

Attachments

(4 files, 2 obsolete files)

 
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.
(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.
Attached patch render part, v1 (obsolete) — Splinter Review
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.
Assignee: joshmoz → mstange
Status: NEW → ASSIGNED
Attachment #353431 - Flags: superreview?(roc)
Attachment #353431 - Flags: review?(joshmoz)
Attached patch CSS part, v1Splinter Review
Attachment #353438 - Flags: review?(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?
That was a deliberate change; in native Mac apps, find bars don't fade.
Attachment #353438 - Flags: review?(dao) → review+
Blocks: 470045
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?
Attached patch render part, v2 (obsolete) — Splinter Review
Attachment #353431 - Attachment is obsolete: true
Attachment #353532 - Flags: superreview?(roc)
Attachment #353532 - Flags: review?(joshmoz)
Attachment #353431 - Flags: superreview?(roc)
Attachment #353431 - Flags: review?(joshmoz)
(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.
Attachment #353535 - Flags: ui-review?(faaborg)
Attachment #353535 - Attachment is patch: false
Attachment #353535 - Attachment mime type: text/plain → image/png
Alex, which one do you prefer?
Attachment #353537 - Flags: ui-review?(faaborg)
Comment on attachment 353532 [details] [diff] [review]
render part, v2

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

I think < 2.0f would be better.
Attachment #353532 - Flags: superreview?(roc) → superreview+
fwiw, Camino uses a "gradient-style" statusbar.
(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.
> 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.
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).
Attachment #353535 - Flags: ui-review?(faaborg) → ui-review+
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 on attachment 353537 [details]
Screenshot: Safari-style statusbar

Sorry, forgot to set the review flags back on 12/18, see comment #16 for
rationale.
Attachment #353537 - Flags: ui-review?(faaborg) → ui-review-
Attachment #353532 - Flags: review?(joshmoz) → review+
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
Attachment #353532 - Attachment is obsolete: true
Checked in:
http://hg.mozilla.org/mozilla-central/rev/e666dabedee5
http://hg.mozilla.org/mozilla-central/rev/c7538401b1c9
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Attachment #353532 - Flags: approval1.9.1?
Attachment #355572 - Flags: approval1.9.1?
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
Attachment #353532 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 355572 [details] [diff] [review]
render part, v2.1, for checkin

a191=beltzner
Attachment #355572 - Flags: approval1.9.1? → approval1.9.1+
Blocks: 462376
You need to log in before you can comment on or make changes to this bug.