Last Comment Bug 394892 - Draw checkboxes and radiobuttons with NSButtonCell
: Draw checkboxes and radiobuttons with NSButtonCell
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: All Mac OS X
: P3 normal (vote)
: mozilla1.9.1b2
Assigned To: Markus Stange [:mstange]
:
Mentors:
Depends on: 462233
Blocks: 54488 424151 446647 459708
  Show dependency treegraph
 
Reported: 2007-09-04 11:20 PDT by Colin Barrett [:cbarrett]
Modified: 2008-10-29 14:52 PDT (History)
15 users (show)
jaas: blocking1.9-
jaas: wanted1.9+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix v0.7 (3.12 KB, patch)
2007-09-04 11:20 PDT, Colin Barrett [:cbarrett]
no flags Details | Diff | Review
fix 1.0 (3.34 KB, patch)
2007-09-05 14:56 PDT, Colin Barrett [:cbarrett]
jaas: review-
Details | Diff | Review
fix v1.1 (5.30 KB, patch)
2007-09-06 14:17 PDT, Colin Barrett [:cbarrett]
jaas: review+
mozcbarrett: review-
Details | Diff | Review
fix v1.2 (5.33 KB, patch)
2007-09-10 15:18 PDT, Colin Barrett [:cbarrett]
jaas: review+
Details | Diff | Review
fix v1.3 (5.25 KB, patch)
2007-09-11 09:23 PDT, Colin Barrett [:cbarrett]
jaas: review+
Details | Diff | Review
fix v1.4 (I really hope this is the last one) (5.34 KB, patch)
2007-09-11 10:35 PDT, Colin Barrett [:cbarrett]
jaas: review+
Details | Diff | Review
patch v1.4.1 (5.42 KB, patch)
2007-09-11 16:53 PDT, Colin Barrett [:cbarrett]
jaas: review-
Details | Diff | Review
patch v1.5 (5.41 KB, patch)
2007-09-18 14:14 PDT, Colin Barrett [:cbarrett]
no flags Details | Diff | Review
radio fix v1.0 (14.89 KB, patch)
2008-01-09 18:34 PST, Josh Aas
mozcbarrett: review+
roc: superreview+
Details | Diff | Review
drawWindow testcase, needs signed.applets.codebase_principal_support = true (5.44 KB, text/html)
2008-06-17 15:15 PDT, Markus Stange [:mstange]
no flags Details
WIP fix 0.1: Beautiful radio buttons with NSSmallControlSize (4.17 KB, patch)
2008-06-17 15:27 PDT, Markus Stange [:mstange]
no flags Details | Diff | Review
Fix v0.2: Draw checkboxes using NSButtonCell (16.35 KB, patch)
2008-06-18 09:57 PDT, Markus Stange [:mstange]
no flags Details | Diff | Review
Fix v0.3: Never scale, use NSRegularControlSize for large checkboxes / radiobuttons (16.67 KB, patch)
2008-06-18 11:50 PDT, Markus Stange [:mstange]
no flags Details | Diff | Review
Fix v0.4: Adapt for Leopard (11.65 KB, patch)
2008-07-20 16:00 PDT, Markus Stange [:mstange]
no flags Details | Diff | Review
Fix v0.5: Remove minimum widget size, use scaling and snapping (25.52 KB, patch)
2008-07-24 07:05 PDT, Markus Stange [:mstange]
no flags Details | Diff | Review
fix v0.6 (30.86 KB, patch)
2008-09-29 14:08 PDT, Markus Stange [:mstange]
no flags Details | Diff | Review
v7: only snap when the difference is <= 1px, base the snap direction on vertical align (32.05 KB, patch)
2008-10-01 15:00 PDT, Markus Stange [:mstange]
jaas: review+
dbaron: superreview+
Details | Diff | Review
Testcase for scaled radiobuttons and checkboxes (20.99 KB, text/html)
2008-10-01 15:01 PDT, Markus Stange [:mstange]
no flags Details
Screenshot of testcase with patch v7 (93.17 KB, image/png)
2008-10-01 15:02 PDT, Markus Stange [:mstange]
no flags Details

Description Colin Barrett [:cbarrett] 2007-09-04 11:20:19 PDT
Created attachment 279627 [details] [diff] [review]
fix v0.7

There are a number of reasons why we want to do this. 1) NSButtonCell is a more modern API. It makes our code more readable and approachable. 2) It can draw in states that HITheme cannot, that we want controls to draw in (see bug 54488 and bug 394092).

The attached patch mostly works. There are some issues with clipping that I think stem from the -2 we return in GetMinimumWidgetSize for small radio buttons. I think we want to more that -2 somewhere else, but I'm not sure where.

Also, we might want to keep around an NSButtonCell instance -- but I'd only recommend that if that object alloc is showing up on profiles. I don't think it will, though.
Comment 1 Colin Barrett [:cbarrett] 2007-09-05 14:56:58 PDT
Created attachment 279797 [details] [diff] [review]
fix 1.0

This fixes the drawing issues I was talking about.

I'm wondering if we should change the parameters to DrawCheckboxRadio. Instead of taking HITheme types, we should just pass in NSRects, bools, and NS_THEME_* constants.

Also unclear if I should make the 1 and 2 offets constants.

A note about the weird casting when calling drawFrame:inView: : I got that from Apple's documentation (http://developer.apple.com/releasenotes/Cocoa/AppKitOlderNotes.html search for CGRect. Note that HIRect is a typedef of CGRect. I found all that from this page: http://www.cocoadev.com/index.pl?NSRectToCGRect )
Comment 2 Josh Aas 2007-09-06 13:22:35 PDT
Comment on attachment 279797 [details] [diff] [review]
fix 1.0

+    drawRect.size.height += 1;

Yeah, that should probably be a define, SOMETHING_OFFSET... Use the macro for each in both places in the file.

+  } else if(inKind == kThemeRadioButtonSmall) {

Space after the "if" here

As for changing the arguments to the drawing function, lets leave it for now. Once we have more controls converted to NSCell we can re-evaluate.

The weird cast is fine for now, seems like Apple recommends it from my experience. It'll go away when we fix up the args to that function.
Comment 3 Colin Barrett [:cbarrett] 2007-09-06 14:17:49 PDT
Created attachment 279974 [details] [diff] [review]
fix v1.1
Comment 4 Colin Barrett [:cbarrett] 2007-09-10 11:36:35 PDT
Comment on attachment 279974 [details] [diff] [review]
fix v1.1

>-  if (inDisabled)
>-    bdi.state = kThemeStateUnavailable;

Just realized I didn't check this flag in my new version. Will post an updated patch, and this time I'll run the reftests against it.
Comment 5 Colin Barrett [:cbarrett] 2007-09-10 15:18:17 PDT
Created attachment 280391 [details] [diff] [review]
fix v1.2

Fixes the above issue, and passes all reftests, near as I can tell (it would be nice if reftest outputted a summary at the end? maybe it did and I missed it)
Comment 6 Colin Barrett [:cbarrett] 2007-09-11 09:23:21 PDT
Created attachment 280468 [details] [diff] [review]
fix v1.3

Last one, I swear.

It turns out that disabled and highlighted (being clicked on) need to be exclusive states -- otherwise clicking on an inactive control results in it being highlighted.

I'd like to try to figure out how to write a reftest for that.
Comment 7 Colin Barrett [:cbarrett] 2007-09-11 10:35:28 PDT
Created attachment 280474 [details] [diff] [review]
fix v1.4 (I really hope this is the last one)

Disabled elements should not get focus rings either. Also, don't use NSSetFocusRingStyle.
Comment 8 Colin Barrett [:cbarrett] 2007-09-11 16:53:52 PDT
Created attachment 280528 [details] [diff] [review]
patch v1.4.1

Alright, well, this fixes the fact that small radio buttons would draw outside their frame. Only change is #define SMALL_RADIO_HEIGHT_OFFSET 2  is now   #define SMALL_RADIO_HEIGHT_OFFSET 1. This should be fine, but please make sure.

I also noticed something with checkboxes in bugzilla and the focus ring, but I swear I've seen it in builds without this change, so I don't think it's related. I took a screenshot, can upload if needed (the right and bottom edges of the focus ring don't get cleared -- I can sometimes repro on the "remove selected CC's" checkbox). I guess it's possible this change exposes a bug somewhere else?
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-09-12 20:29:12 PDT
In GTK2 native theme drawing we cache widgets of the necessary types, would that be a good idea here? Can you measure performance painting a button-heavy page with and without this patch?
Comment 10 Colin Barrett [:cbarrett] 2007-09-12 22:13:13 PDT
Sure. Do we have any existing performance tests that cover it?

Here's some preliminary data on the relative speed of various Cocoa operations, including NSButtonCell creation and drawWithFrame:inView:.

http://www.mikeash.com/blog/pivot/entry.php?id=29

It's not too terribly slow (microseconds to init, tens of microseconds to paint), but it would be good to verify things before we land it.
Comment 11 Josh Aas 2007-09-13 15:24:44 PDT
Comment on attachment 280528 [details] [diff] [review]
patch v1.4.1

+  [cell setState:inChecked];

The argument for setState is not a PRBool - please pass the correct state. NSOnState, NSOffState, or NSMixedState.

+    drawRect.size.height += SMALL_CHECKBOX_HEIGHT_OFFSET;

This is incorrect. You don't want to change the size of the control you're drawing, you want to shift it down. Do this instead:

drawRect.origin.y += SMALL_CHECKBOX_HEIGHT_OFFSET;
Comment 12 Colin Barrett [:cbarrett] 2007-09-18 14:14:00 PDT
Created attachment 281367 [details] [diff] [review]
patch v1.5

Addresses josh's review comments. If I make the same change he suggested to make to checkboxes to radio buttons, I get a rendering glitch. I'm a bit confused as to why that's happening (since josh's change to checkboxes was just a correctness fix -- didn't actually change rendering)

I've included that line, commented out, in this patch. I'd remove it on checkin if this turns out to be the right thing to do.
Comment 13 Reed Loden [:reed] (use needinfo?) 2007-12-03 21:23:31 PST
Is this still relevant? I don't see any activity since September...
Comment 14 Josh Aas 2007-12-03 21:29:42 PST
Yes, it is. We've had to work on other stuff, this is still a blocker.
Comment 15 Josh Aas 2008-01-09 18:34:41 PST
Created attachment 296265 [details] [diff] [review]
radio fix v1.0

This is a fix for radio buttons only, I'll post checkboxes once this goes in.

For some reason, as noted in the patch, the button cell won't respect any side we give it other than regular. Because of that, I always draw regular size and scale down if need be. I left in some of the metrics for the other sizes where I could calculate them, maybe in the future we'll figure out how to use small and mini control sizes. Right now it doesn't matter much though and we just need to do this and keep going.

Note that some of the code for drawing radio buttons is similar to what we do for push buttons, but there are enough differences thrown in that sharing more code is messy. For example, CTM adjustment happens in a different place, scaling vs. not scaling is different, 

This also fixes a bug where layout didn't redraw controls when keyboard gave them focus. This is separate from whether we draw with NSCell or not.
Comment 16 Colin Barrett [:cbarrett] 2008-01-10 14:06:19 PST
Comment on attachment 296265 [details] [diff] [review]
radio fix v1.0

This looks fine.

One thing you might be able to share is the code to generate initialBufferSize and bufferRenderRect and finalCopyRect.
Comment 17 Josh Aas 2008-01-10 14:08:09 PST
Comment on attachment 296265 [details] [diff] [review]
radio fix v1.0

If that stuff can be shared I'll do it with the checkbox patch that is coming up.
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-01-13 14:07:02 PST
Comment on attachment 296265 [details] [diff] [review]
radio fix v1.0

+  [mRadioButtonCell setBezelStyle:NSRoundedBezelStyle];

Can't this go in the constructor?
Comment 19 Josh Aas 2008-01-13 20:49:14 PST
radio fix landed on trunk
Comment 20 Josh Aas 2008-02-12 12:58:54 PST
probably not going to make it for 1.9, tough call but we're fast running out of time
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-02-12 13:53:29 PST
So the remaining issue is checkboxes, right? Maybe hwaara can rescue this. I think this is important because canvas.drawWindow (and printing?) of checkboxes won't work without it ... right?
Comment 22 Markus Stange [:mstange] 2008-06-17 15:15:09 PDT
Created attachment 325453 [details]
drawWindow testcase, needs signed.applets.codebase_principal_support = true

(In reply to comment #21)
> I think this is important because canvas.drawWindow (and printing?) of
> checkboxes won't work without it ... right?

canvas.drawWindow of checkboxes seems to work in this testcase.
But I still want to fix this because I need NSClearTint for bug 54488.
Comment 23 Markus Stange [:mstange] 2008-06-17 15:27:35 PDT
Created attachment 325457 [details] [diff] [review]
WIP fix 0.1: Beautiful radio buttons with NSSmallControlSize

This patch doesn't contain changes to checkboxes yet.

(In reply to comment #15)
> For some reason, as noted in the patch, the button cell won't respect any
> size we give it other than regular.

I've found the reason - [cell setHighlightsBy:NSPushInCellMask] was causing it.
When I removed that call, I could render radio buttons at NSSmallControlSize - but they were always transparent (as if using NSClearControlTint).
However, these lines from Colin's patches fixed it:
+// Workaround for NSCell control tint drawing.
+@implementation NSCell (ControlTintWorkaround)
+- (int)_realControlTint { return [self controlTint]; }
+@end


> Because of that, I always draw regular size and
> scale down if need be.

Is there a way of drawing them at smaller sizes? Changing the CSS height doesn't have any effect; no matter what I do, the requested drawRect is always 14x14. That's why I didn't bother with the sizes in this patch.

The patch also fixes bug 424151:
+  [mRadioButtonCell setControlTint:[NSColor currentControlTint]];
Comment 24 Markus Stange [:mstange] 2008-06-18 09:57:10 PDT
Created attachment 325582 [details] [diff] [review]
Fix v0.2: Draw checkboxes using NSButtonCell

This patch also contains the changes for checkboxes. At least on 10.4 the rendering of checkboxes with and without the patch is identical.
I might be able to test it on 10.5 on the weekend.

Checkboxes were vertically offset by 1px, so I extended the marginSets to include offsets.
Comment 25 Markus Stange [:mstange] 2008-06-18 11:50:25 PDT
Created attachment 325601 [details] [diff] [review]
Fix v0.3: Never scale, use NSRegularControlSize for large checkboxes / radiobuttons

Hah. I've found large checkboxes and radio buttons in the preferences.
(But I still haven't found out how to control their size with CSS.)

The marginSets still need to be adapted for Leopard.
Comment 26 Markus Stange [:mstange] 2008-07-20 16:00:02 PDT
Created attachment 330509 [details] [diff] [review]
Fix v0.4: Adapt for Leopard
Comment 27 Markus Stange [:mstange] 2008-07-20 16:04:56 PDT
(In reply to comment #25)
> (But I still haven't found out how to control their size with CSS.)

I've figured out what's going on: You can't change their size with CSS because it's disallowed in GetMinimumWidgetSize. Switching between regular and small size is done using -moz-appearance: radio / radio-small.
Comment 28 Markus Stange [:mstange] 2008-07-23 09:46:14 PDT
Comment on attachment 330509 [details] [diff] [review]
Fix v0.4: Adapt for Leopard

I'm going to make some changes to this patch for bug 446647.
Comment 29 Markus Stange [:mstange] 2008-07-24 07:05:19 PDT
Created attachment 331093 [details] [diff] [review]
Fix v0.5: Remove minimum widget size, use scaling and snapping

This patch makes radiobuttons and checkboxes resizable and still tries to look pretty. If the requested size is smaller than the natural mini size or larger than the natural regular size, scaling is applied. In between, the sizes snap to the next smaller natural size.
Comment 30 Markus Stange [:mstange] 2008-07-26 14:32:07 PDT
Comment on attachment 331093 [details] [diff] [review]
Fix v0.5: Remove minimum widget size, use scaling and snapping

More changes coming...
Comment 31 Markus Stange [:mstange] 2008-09-29 14:08:23 PDT
Created attachment 340990 [details] [diff] [review]
fix v0.6
Comment 32 Markus Stange [:mstange] 2008-10-01 15:00:12 PDT
Created attachment 341350 [details] [diff] [review]
v7: only snap when the difference is <= 1px, base the snap direction on vertical align

I made the changes requested on IRC.
Comment 33 Markus Stange [:mstange] 2008-10-01 15:01:14 PDT
Created attachment 341352 [details]
Testcase for scaled radiobuttons and checkboxes
Comment 34 Markus Stange [:mstange] 2008-10-01 15:02:13 PDT
Created attachment 341353 [details]
Screenshot of testcase with patch v7
Comment 35 Josh Aas 2008-10-02 17:15:33 PDT
Comment on attachment 341350 [details] [diff] [review]
v7: only snap when the difference is <= 1px, base the snap direction on vertical align

+                                 const float verticalAlignFactor,

const?

Not done with the review, just got interrupted.
Comment 36 Josh Aas 2008-10-02 22:52:40 PDT
Comment on attachment 341350 [details] [diff] [review]
v7: only snap when the difference is <= 1px, base the snap direction on vertical align

Fix the const thing on checkin, otherwise this looks good to me.
Comment 37 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-10-06 17:19:10 PDT
Comment on attachment 341350 [details] [diff] [review]
v7: only snap when the difference is <= 1px, base the snap direction on vertical align

I'd think baseline should be aligned to the bottom like bottom and
text-bottom are, rather than aligned to the middle.  Or was there a reason you went the other way?


+/*
+ * CellRenderSettings
+ *
+ * naturalSize - The natural dimensions of the control.
+ *  If a control has no natural dimensions in either/both axes, set to 0.0f.
+ * minimumSize - The minimum dimensions of the control.
+ *  If a control has no minimum dimensions in either/both axes, set to 0.0f.
+ * marginSet - an array of margins; a multidimensional array of [2][3][4],
+ *  with the first two array elements being margins for Tiger or Leopard,
+ *  the next three being control size (mini, small, regular), and the 4
+ *  being the 4 margin values.
+ */
+struct CellRenderSettings {
+  NSSize naturalSizes[3];
+  NSSize minimumSizes[3];
+  float margins[2][3][4];
+};

Why not move the comments down into the struct, above each member?

Also, the description of marginSet should really refer to the first
dimension, second dimension, and third dimension, rather than 2
elements, 3 elements, and 4 elements (which sounds like 9 elements
total).


Maybe DrawCheckboxRadio should be called DrawCheckboxOrRadio?


Could you file a bug on getting rid of the radio-small and
checkbox-small values now, since the only reason we needed them in the
first place was for mac (I think)?


sr=dbaron
Comment 38 Markus Stange [:mstange] 2008-10-13 10:03:00 PDT
pushed with comments addressed: http://hg.mozilla.org/mozilla-central/rev/a9e2b417b631
Comment 39 Markus Stange [:mstange] 2008-10-13 12:59:49 PDT
(In reply to comment #37)
> Could you file a bug on getting rid of the radio-small and
> checkbox-small values now, since the only reason we needed them in the
> first place was for mac (I think)?

I filed bug 459708.
Comment 40 Henrik Skupin (:whimboo) 2008-10-13 14:31:05 PDT
Markus, which bug covers the no-gray-out action for checkboxes and radiobuttons when the window isn't focused anymore? Safari don't gray out these widgets when switching to another application while we do.
Comment 41 Markus Stange [:mstange] 2008-10-14 00:52:30 PDT
(In reply to comment #40)
> Markus, which bug covers the no-gray-out action for checkboxes and radiobuttons
> when the window isn't focused anymore?

This bug.

And bug 399030 covers it for selects.
Comment 42 Henrik Skupin (:whimboo) 2008-10-14 04:22:15 PDT
(In reply to comment #41)
> > Markus, which bug covers the no-gray-out action for checkboxes and radiobuttons
> > when the window isn't focused anymore?
> 
> This bug.

So why this bug is marked as fixed? We still have another behavior as other native OS X applications.
Comment 43 Henrik Skupin (:whimboo) 2008-10-14 10:31:12 PDT
Seems like my debug build was busted. Now I can verify this fix with the official nightly build: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081014 Minefield/3.1b2pre ID:20081014020405

Note You need to log in before you can comment on or make changes to this bug.