1085134 - Native theming for Mac OS X disclosure buttons

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Stefan [:stefanh]]

We currently use icons for OS X disclosure buttons in the places window and the Clear Recent History dialog. These buttons doesn't really match the OS style to 100% and we also don't have any 2x versions.

Comment 1

[Stefan [:stefanh]]

Attachment: Widget part

I took the same approach as with the help button and I figured I could then re-use the existing NSButtonCell. I'll let Mano look at the css once this is done. The only possible bad thing here is that the native disclosure button is aqua blue on 10.6 (similar to what the current active icon looks like). Otherwise, I think this is an improvement.

Comment 2

[Stefan [:stefanh]]

Attachment: css part

This is the css part.

Comment 3

[Stefan [:stefanh]]

Attachment: Screenshots (10.9.5), Places window

Here are some before-after screenshots (from top to bottom: before-after & before-after).

Comment 4

[Stefan [:stefanh]]

And there will be try builds (just pushed) available here: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-76148149c301

Comment 5

[Stefan [:stefanh]]

Comment on attachment 8507534 [details] [diff] [review]
Widget part

Roc, could you please take a quick look at the gfx/src and layout/style parts (Markus have already reviewed the other parts)?

Comment 6

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

Comment on attachment 8507534 [details] [diff] [review]
Widget part

Review of attachment 8507534 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine modulo the above comment.

::: widget/cocoa/nsNativeThemeCocoa.mm
@@ +478,5 @@
>    // before the main event-loop pool is in place
>    nsAutoreleasePool pool;
>  
> +  mButtonCell = [[NSButtonCell alloc] initTextCell:nil];
> +  [mButtonCell setHighlightsBy:NSPushInCellMask];

Wouldn't it make more sense, or at least be more consistent with the existing code, to add a separate mDisclosureButtonCell?

Comment 7

[Stefan [:stefanh]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> Wouldn't it make more sense, or at least be more consistent with the
> existing code, to add a separate mDisclosureButtonCell?

Yes. I'll add one, thanks.

Comment 8

[Stefan [:stefanh]]

Comment on attachment 8507535 [details] [diff] [review]
css part

Mano, can you look at this? It looks like the twisty-open.gif and twisty-closed.gif are not used, so I removed them.

I'll post some screenshots of the clear history dialog.

Comment 9

[Stefan [:stefanh]]

Attachment: Screenshots, Clear Recent History dialog

Top-bottom: 10.6.8, 10.9.5 and 10.10.

Comment 10

[Stefan [:stefanh]]

Thanks, I'll land everything when I'm back from vacation.

Comment 11

[Stefan [:stefanh]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9179aa942d3b

Comment 12

[Stefan [:stefanh]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d56bb31bab5
https://hg.mozilla.org/integration/mozilla-inbound/rev/98e71f1f533d

Comment 13

[Stefan [:stefanh]]

I noticed an error in attachment 8507534 [details] [diff] [review]:
     case NS_THEME_MOZ_MAC_HELP_BUTTON:
     {
-      aResult->SizeTo(kHelpButtonSize.width, kHelpButtonSize.height);
+      aResult->SizeTo(kDisclosureButtonSize.width, kDisclosureButtonSize.height);
       *aIsOverridable = false;
       break;
     }

I reverted this change in https://hg.mozilla.org/integration/mozilla-inbound/rev/c43dc30b39b4

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2d56bb31bab5
https://hg.mozilla.org/mozilla-central/rev/98e71f1f533d
https://hg.mozilla.org/mozilla-central/rev/c43dc30b39b4