Closed Bug 1085134 Opened 6 years ago Closed 6 years ago

Native theming for Mac OS X disclosure buttons

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: stefanh, Assigned: stefanh)

References

Details

Attachments

(4 files)

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.
Attached patch Widget partSplinter Review
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.
Assignee: nobody → stefanh
Status: NEW → ASSIGNED
Attachment #8507534 - Flags: review?(mstange)
Attached patch css partSplinter Review
This is the css part.
Here are some before-after screenshots (from top to bottom: before-after & before-after).
And there will be try builds (just pushed) available here: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/stefanh@inbox.com-76148149c301
Attachment #8507534 - Flags: review?(mstange) → review+
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)?
Attachment #8507534 - Flags: review?(roc)
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?
Attachment #8507534 - Flags: review?(roc) → review+
(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 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.
Attachment #8507535 - Flags: review?(mano)
Top-bottom: 10.6.8, 10.9.5 and 10.10.
Thanks, I'll land everything when I'm back from vacation.
Blocks: 1092584
Blocks: 1092585
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
You need to log in before you can comment on or make changes to this bug.