Closed Bug 970079 Opened 6 years ago Closed 6 years ago

Native theming for MacOS X help buttons

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: stefanh, Assigned: stefanh)

References

Details

Attachments

(3 files, 8 obsolete files)

The help button still relies on css for its icon (the look of the icon is alos outdated). We could use native styling to render the help button instead.
Attached patch Almost there (obsolete) — Splinter Review
I need to look over the css a bit and I'm in-between with the naming of the widget type. Also, I'm a bit unsure of the scaling.
Comment on attachment 8373042 [details] [diff] [review]
Almost there

Whoops, this version doesn't work...
Attachment #8373042 - Attachment is obsolete: true
Attached patch Patch that actually works (obsolete) — Splinter Review
Putting up a new version that actually works. Btw, Apple seems to be using the regular icon for Arabic, so I don't think we shall use IsFrameRTL(aFrame) here.
Attached patch New version (obsolete) — Splinter Review
New wip version with a mHelpButtonCell. Interestingly, it seems to be enough to set the bezelstyle.
Attached patch Widget part (obsolete) — Splinter Review
I think this is ready for review now.
We don't scale it down, and we don't scale up the icon - just the rect. I went with a new mHelpButtonCell, mostly because of the problems with the button height sometimes not being accurate. As you mentioned, this is probably caused by setControlSize not being called in DrawCellWithScaling. I could probably do something about that, but I'm not sure how much we gain by that.
Attachment #8376869 - Attachment is obsolete: true
Attachment #8377244 - Attachment is obsolete: true
Attachment #8380348 - Flags: review?(mstange)
Attached patch CSS part (obsolete) — Splinter Review
This is the CSS part - Dao, can you please take a look at it?
Attachment #8380350 - Flags: review?(dao)
Comment on attachment 8380348 [details] [diff] [review]
Widget part

I forgot to mention that try builds (widget+css) are available at https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/stefanh@inbox.com-15c5cefc3ce8/
Attached image Screenshots before/after (obsolete) —
Screenshots comparing before/after for a couple of prefpanes. I didn't find any reason for the margin difference in preferences.css and I think it makes more sense to change the bottom margin to be the same as the side margins. Since the old help button was 24x24, I've added some top margin to make it look better in the Applications pane.
Comment on attachment 8380348 [details] [diff] [review]
Widget part

>diff --git a/widget/cocoa/nsNativeThemeCocoa.mm b/widget/cocoa/nsNativeThemeCocoa.mm

>@@ -982,40 +988,55 @@ static const CellRenderSettings pushButt
> 
> // The height at which we start doing square buttons instead of rounded buttons
> // Rounded buttons look bad if drawn at a height greater than 26, so at that point
> // we switch over to doing square buttons which looks fine at any size.
> #define DO_SQUARE_BUTTON_HEIGHT 26
> 
> void
> nsNativeThemeCocoa::DrawPushButton(CGContextRef cgContext, const HIRect& inBoxRect,
>-                                   nsEventStates inState, nsIFrame* aFrame)
>+                                   nsEventStates inState, uint8_t aWidgetType,
>+                                   nsIFrame* aFrame)
> {
>   NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
> 
>   BOOL isActive = FrameIsInActiveWindow(aFrame);
>   BOOL isDisabled = IsDisabled(aFrame, inState);
> 
>-  [mPushButtonCell setEnabled:!isDisabled];
>-  [mPushButtonCell setHighlighted:isActive &&
>-                                  inState.HasAllStates(NS_EVENT_STATE_ACTIVE | NS_EVENT_STATE_HOVER)];
>-  [mPushButtonCell setShowsFirstResponder:inState.HasState(NS_EVENT_STATE_FOCUS) && !isDisabled && isActive];

You can share these three calls for both button types by adding
NSCell* cell = (aWidgetType == NS_THEME_MOZ_MAC_HELP_BUTTON) ? mHelpButtonCell : mPushButtonCell;
before and then calling them on cell.

>+  if (aWidgetType == NS_THEME_MOZ_MAC_HELP_BUTTON) {
>+    [mHelpButtonCell setEnabled:!isDisabled];
>+    [mHelpButtonCell setShowsFirstResponder:inState.HasState(NS_EVENT_STATE_FOCUS) &&
>+                                            !isDisabled && isActive];
>+    [mHelpButtonCell setHighlighted:isActive &&
>+                                    inState.HasState(NS_EVENT_STATE_ACTIVE)];

Then you also don't have the inconsistency here ;-)
(The pressed state should be used during :active:hover, not during all of :active because then the button also stays pressed when you move the mouse outside the button while keeping it pressed. But releasing the mouse button outside the button won't result in a click on it, so the pressed indication is misleading.)

>+    DrawCellWithScaling(mHelpButtonCell, cgContext, inBoxRect,
>+                        NSRegularControlSize, NSZeroSize, NSMakeSize(20, 20),
>+                        NULL, mCellDrawView, false); // Don't mirror icon in RTL.

Can you put a "static const NSSize kHelpButtonSize = NSMakeSize(20, 20);" somewhere further up, and use that here and in GetMinimumWidgetSize (as aResult->SizeTo(kHelpButtonSize.width, kHelpButtonSize.height);)?
Attached patch New version (obsolete) — Splinter Review
Not sure what I thought of when I skipped the hover state.

Shared the 3 calls and added a constant for the width/height. I also sorted the calls according to your original order.
Attachment #8380348 - Attachment is obsolete: true
Attachment #8380350 - Attachment is obsolete: true
Attachment #8380348 - Flags: review?(mstange)
Attachment #8380350 - Flags: review?(dao)
Attachment #8380776 - Flags: review?(mstange)
Attached patch New css version (obsolete) — Splinter Review
I decided to skip the 7px margin-top rule, it feel like an overkill to special-case the help button and I don't think the 2px decrease matters. I'll attach new screenshots.
Attachment #8380356 - Attachment is obsolete: true
Attachment #8380780 - Flags: review?(dao)
Comment on attachment 8380776 [details] [diff] [review]
New version

Great!
Attachment #8380776 - Flags: review?(mstange) → review+
Looking at the CSS patch, I noticed one thing you can add to the widget patch to make the CSS one simpler: In GetMinimumWidgetSize, add *aIsOverridable = false;, then you can get rid of all the width/height/min-width rules in the CSS.
Comment on attachment 8380780 [details] [diff] [review]
New css version

canceling review because of comment 14
Attachment #8380780 - Flags: review?(dao)
I'll have a look (tonight or tomorrow night) and put up a new patch - thanks Markus.
(In reply to Markus Stange [:mstange] from comment #14)
> Looking at the CSS patch, I noticed one thing you can add to the widget
> patch to make the CSS one simpler: In GetMinimumWidgetSize, add
> *aIsOverridable = false;, then you can get rid of all the
> width/height/min-width rules in the CSS.

WidgetIsContainer returns false, because we don't want the label to be displayed but the label inside the button affects the button width, though. With "*aIsOverridable = false;" I only make it impossible to override the minimum size and with width/height/min-width rules removed, we'll end up with a rect wider that 20px (35). It works fine if I set the width to 20px in the css file, though. I suppose we could either do that or add back the button-text display:none rule. Hmm.
(In reply to Stefan [:stefanh] from comment #17)
> With "*aIsOverridable = false;" I only make it impossible to override the
> minimum size

I don't think that's true. I think doing that completely locks down the size.
Turns out that I was right about GetMinimumWidgetSize - at least its behaviour. So Markus and I came to the conclusion that the best way here is to keep the width in the css.
Attachment #8380780 - Attachment is obsolete: true
Attachment #8382338 - Flags: review?(dao)
Roc, can you please take a glance at the gfx/layout part (Markus have reviewed the other parts)?
Attachment #8382341 - Flags: review?(roc)
Status: NEW → ASSIGNED
(In reply to Stefan [:stefanh] from comment #19)
> Created attachment 8382338 [details] [diff] [review]
> New css version (keep width)
> 
> Turns out that I was right about GetMinimumWidgetSize - at least its
> behaviour. So Markus and I came to the conclusion that the best way here is
> to keep the width in the css.

And I filed bug 977229 in order to sort out the GetMinimumWidgetSize behaviour.
Attachment #8382338 - Flags: review?(dao) → review+
Attachment #8380776 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/c483fb963fe1
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd328470b070

I made some adjustments to the widget part before I pushed (removed 3 duplicated calls that I for some added earlier and switched from locally using NSCell* cell to NSButtonCell* cell to silence a build warning).
https://hg.mozilla.org/mozilla-central/rev/bd328470b070
https://hg.mozilla.org/mozilla-central/rev/c483fb963fe1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.