Closed Bug 394892 Opened 14 years ago Closed 13 years ago

Draw checkboxes and radiobuttons with NSButtonCell

Categories

(Core :: Widget: Cocoa, defect, P3)

All
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1b2

People

(Reporter: cbarrett, Assigned: mstange)

References

Details

Attachments

(5 files, 14 obsolete files)

14.89 KB, patch
cbarrett
: review+
Details | Diff | Splinter Review
5.44 KB, text/html
Details
32.05 KB, patch
jaas
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
20.99 KB, text/html
Details
93.17 KB, image/png
Details
Attached patch fix v0.7 (obsolete) — Splinter Review
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.
Attachment #279627 - Flags: review?(joshmoz)
Attachment #279627 - Attachment is patch: true
Attachment #279627 - Attachment mime type: application/octet-stream → text/plain
Assignee: joshmoz → cbarrett
Attachment #279627 - Flags: review?(joshmoz)
Attached patch fix 1.0 (obsolete) — Splinter Review
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 )
Attachment #279627 - Attachment is obsolete: true
Attachment #279797 - Flags: review?(joshmoz)
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.
Attachment #279797 - Flags: review?(joshmoz) → review-
Attached patch fix v1.1 (obsolete) — Splinter Review
Attachment #279797 - Attachment is obsolete: true
Attachment #279974 - Flags: review?(joshmoz)
Attachment #279974 - Flags: superreview?(roc)
Attachment #279974 - Flags: review?(joshmoz)
Attachment #279974 - Flags: review+
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.
Attachment #279974 - Attachment is obsolete: true
Attachment #279974 - Flags: superreview?(roc) → review-
Attached patch fix v1.2 (obsolete) — Splinter Review
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)
Attachment #280391 - Flags: review?(joshmoz)
Attachment #280391 - Flags: superreview?(roc)
Attachment #280391 - Flags: review?(joshmoz)
Attachment #280391 - Flags: review+
Attachment #280391 - Flags: approval1.9?
Attached patch fix v1.3 (obsolete) — Splinter Review
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.
Attachment #280391 - Attachment is obsolete: true
Attachment #280468 - Flags: review?(joshmoz)
Attachment #280391 - Flags: superreview?(roc)
Attachment #280391 - Flags: approval1.9?
Attachment #280468 - Flags: superreview?(roc)
Attachment #280468 - Flags: review?(joshmoz)
Attachment #280468 - Flags: review+
Attachment #280468 - Flags: approval1.9?
Disabled elements should not get focus rings either. Also, don't use NSSetFocusRingStyle.
Attachment #280468 - Attachment is obsolete: true
Attachment #280474 - Flags: review?(joshmoz)
Attachment #280468 - Flags: superreview?(roc)
Attachment #280468 - Flags: approval1.9?
Attachment #280474 - Flags: superreview?(roc)
Attachment #280474 - Flags: review?(joshmoz)
Attachment #280474 - Flags: review+
Attachment #280474 - Flags: approval1.9?
Attached patch patch v1.4.1 (obsolete) — Splinter Review
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?
Attachment #280474 - Attachment is obsolete: true
Attachment #280528 - Flags: review?(joshmoz)
Attachment #280474 - Flags: superreview?(roc)
Attachment #280474 - Flags: approval1.9?
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?
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 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;
Attachment #280528 - Flags: review?(joshmoz) → review-
Attached patch patch v1.5 (obsolete) — Splinter Review
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.
Attachment #280528 - Attachment is obsolete: true
Attachment #281367 - Flags: review?(joshmoz)
Flags: blocking1.9+
Target Milestone: --- → mozilla1.9 M10
Blocks: 54488
No longer blocks: 394092
Target Milestone: mozilla1.9 M10 → ---
Target Milestone: --- → mozilla1.9 M10
Target Milestone: mozilla1.9 M10 → mozilla1.9 M11
Priority: -- → P4
Target Milestone: mozilla1.9 M11 → ---
Attachment #281367 - Flags: review?(joshmoz)
Is this still relevant? I don't see any activity since September...
Yes, it is. We've had to work on other stuff, this is still a blocker.
Assignee: cbarrett → joshmoz
Attached patch radio fix v1.0Splinter Review
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.
Attachment #281367 - Attachment is obsolete: true
Attachment #296265 - Flags: review?(cbarrett)
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.
Attachment #296265 - Flags: review?(cbarrett) → review+
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.
Attachment #296265 - Flags: superreview?(roc)
Priority: P4 → P3
Comment on attachment 296265 [details] [diff] [review]
radio fix v1.0

+  [mRadioButtonCell setBezelStyle:NSRoundedBezelStyle];

Can't this go in the constructor?
Attachment #296265 - Flags: superreview?(roc) → superreview+
radio fix landed on trunk
probably not going to make it for 1.9, tough call but we're fast running out of time
Flags: wanted1.9+
Flags: blocking1.9-
Flags: blocking1.9+
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?
(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.
Assignee: joshmoz → mstange
Status: NEW → ASSIGNED
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]];
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.
Attachment #325457 - Attachment is obsolete: true
Attachment #325582 - Flags: review?(joshmoz)
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.
Attachment #325582 - Attachment is obsolete: true
Attachment #325582 - Flags: review?(joshmoz)
Blocks: 424151
Attached patch Fix v0.4: Adapt for Leopard (obsolete) — Splinter Review
Attachment #325601 - Attachment is obsolete: true
Attachment #330509 - Flags: review?(joshmoz)
(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 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.
Attachment #330509 - Attachment is obsolete: true
Attachment #330509 - Flags: review?(joshmoz)
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.
Attachment #331093 - Flags: review?(joshmoz)
Blocks: 446647
Comment on attachment 331093 [details] [diff] [review]
Fix v0.5: Remove minimum widget size, use scaling and snapping

More changes coming...
Attachment #331093 - Attachment is obsolete: true
Attachment #331093 - Flags: review?(joshmoz)
Attached patch fix v0.6 (obsolete) — Splinter Review
Attachment #340990 - Flags: review?(joshmoz)
Attachment #340990 - Flags: review?(joshmoz) → review+
Attachment #340990 - Flags: review+
I made the changes requested on IRC.
Attachment #340990 - Attachment is obsolete: true
Attachment #341350 - Flags: review?(joshmoz)
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 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.
Attachment #341350 - Flags: review?(joshmoz) → review+
Attachment #341350 - Flags: superreview?(dbaron)
Attachment #341350 - Flags: superreview?(dbaron) → superreview+
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
pushed with comments addressed: http://hg.mozilla.org/mozilla-central/rev/a9e2b417b631
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
Blocks: 459708
(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.
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.
Hardware: PC → All
(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.
(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.
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
Status: RESOLVED → VERIFIED
Depends on: 462233
No longer depends on: 462233
Depends on: 462233
You need to log in before you can comment on or make changes to this bug.