Draw checkboxes and radiobuttons with NSButtonCell

VERIFIED FIXED in mozilla1.9.1b2

Status

()

Core
Widget: Cocoa
P3
normal
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: cbarrett, Assigned: mstange)

Tracking

Trunk
mozilla1.9.1b2
All
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -
wanted1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 14 obsolete attachments)

14.89 KB, patch
cbarrett
: review+
Details | Diff | Splinter Review
5.44 KB, text/html
Details
32.05 KB, patch
Josh Aas
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
20.99 KB, text/html
Details
93.17 KB, image/png
Details
(Reporter)

Description

10 years ago
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.
Attachment #279627 - Flags: review?(joshmoz)
(Reporter)

Updated

10 years ago
Attachment #279627 - Attachment is patch: true
Attachment #279627 - Attachment mime type: application/octet-stream → text/plain
(Reporter)

Updated

10 years ago
Assignee: joshmoz → cbarrett

Updated

10 years ago
Attachment #279627 - Flags: review?(joshmoz)
(Reporter)

Comment 1

10 years ago
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 )
Attachment #279627 - Attachment is obsolete: true
Attachment #279797 - Flags: review?(joshmoz)

Comment 2

10 years ago
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-
(Reporter)

Comment 3

10 years ago
Created attachment 279974 [details] [diff] [review]
fix v1.1
Attachment #279797 - Attachment is obsolete: true
Attachment #279974 - Flags: review?(joshmoz)

Updated

10 years ago
Attachment #279974 - Flags: superreview?(roc)
Attachment #279974 - Flags: review?(joshmoz)
Attachment #279974 - Flags: review+
(Reporter)

Comment 4

10 years ago
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-
(Reporter)

Comment 5

10 years ago
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)
Attachment #280391 - Flags: review?(joshmoz)

Updated

10 years ago
Attachment #280391 - Flags: superreview?(roc)
Attachment #280391 - Flags: review?(joshmoz)
Attachment #280391 - Flags: review+

Updated

10 years ago
Attachment #280391 - Flags: approval1.9?
(Reporter)

Comment 6

10 years ago
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.
Attachment #280391 - Attachment is obsolete: true
Attachment #280468 - Flags: review?(joshmoz)
Attachment #280391 - Flags: superreview?(roc)
Attachment #280391 - Flags: approval1.9?

Updated

10 years ago
Attachment #280468 - Flags: superreview?(roc)
Attachment #280468 - Flags: review?(joshmoz)
Attachment #280468 - Flags: review+
Attachment #280468 - Flags: approval1.9?
(Reporter)

Comment 7

10 years ago
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.
Attachment #280468 - Attachment is obsolete: true
Attachment #280474 - Flags: review?(joshmoz)
Attachment #280468 - Flags: superreview?(roc)
Attachment #280468 - Flags: approval1.9?

Updated

10 years ago
Attachment #280474 - Flags: superreview?(roc)
Attachment #280474 - Flags: review?(joshmoz)
Attachment #280474 - Flags: review+
Attachment #280474 - Flags: approval1.9?
(Reporter)

Comment 8

10 years ago
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?
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?
(Reporter)

Comment 10

10 years ago
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

10 years ago
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-
(Reporter)

Comment 12

10 years ago
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.
Attachment #280528 - Attachment is obsolete: true
Attachment #281367 - Flags: review?(joshmoz)

Updated

10 years ago
Flags: blocking1.9+
Target Milestone: --- → mozilla1.9 M10
(Reporter)

Updated

10 years ago
Blocks: 54488
No longer blocks: 394092
Target Milestone: mozilla1.9 M10 → ---

Updated

10 years ago
Target Milestone: --- → mozilla1.9 M10

Updated

10 years ago
Target Milestone: mozilla1.9 M10 → mozilla1.9 M11

Updated

10 years ago
Priority: -- → P4
Target Milestone: mozilla1.9 M11 → ---

Updated

10 years ago
Attachment #281367 - Flags: review?(joshmoz)
Is this still relevant? I don't see any activity since September...

Comment 14

10 years ago
Yes, it is. We've had to work on other stuff, this is still a blocker.

Updated

10 years ago
Assignee: cbarrett → joshmoz

Comment 15

10 years ago
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.
Attachment #281367 - Attachment is obsolete: true
Attachment #296265 - Flags: review?(cbarrett)
(Reporter)

Comment 16

10 years ago
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 17

10 years ago
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)

Updated

10 years ago
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+

Comment 19

10 years ago
radio fix landed on trunk

Comment 20

9 years ago
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?
(Assignee)

Comment 22

9 years ago
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.
Assignee: joshmoz → mstange
Status: NEW → ASSIGNED
(Assignee)

Comment 23

9 years ago
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]];
(Assignee)

Comment 24

9 years ago
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.
Attachment #325457 - Attachment is obsolete: true
Attachment #325582 - Flags: review?(joshmoz)
(Assignee)

Comment 25

9 years ago
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.
Attachment #325582 - Attachment is obsolete: true
Attachment #325582 - Flags: review?(joshmoz)
(Assignee)

Updated

9 years ago
Blocks: 424151
(Assignee)

Comment 26

9 years ago
Created attachment 330509 [details] [diff] [review]
Fix v0.4: Adapt for Leopard
Attachment #325601 - Attachment is obsolete: true
Attachment #330509 - Flags: review?(joshmoz)
(Assignee)

Comment 27

9 years ago
(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.
(Assignee)

Comment 28

9 years ago
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)
(Assignee)

Comment 29

9 years ago
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.
Attachment #331093 - Flags: review?(joshmoz)
(Assignee)

Updated

9 years ago
Blocks: 446647
(Assignee)

Comment 30

9 years ago
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)
(Assignee)

Comment 31

9 years ago
Created attachment 340990 [details] [diff] [review]
fix v0.6
Attachment #340990 - Flags: review?(joshmoz)

Updated

9 years ago
Attachment #340990 - Flags: review?(joshmoz) → review+

Updated

9 years ago
Attachment #340990 - Flags: review+
(Assignee)

Comment 32

9 years ago
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.
Attachment #340990 - Attachment is obsolete: true
Attachment #341350 - Flags: review?(joshmoz)
(Assignee)

Comment 33

9 years ago
Created attachment 341352 [details]
Testcase for scaled radiobuttons and checkboxes
(Assignee)

Comment 34

9 years ago
Created attachment 341353 [details]
Screenshot of testcase with patch v7

Comment 35

9 years ago
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

9 years ago
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+
(Assignee)

Updated

9 years ago
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
(Assignee)

Comment 38

9 years ago
pushed with comments addressed: http://hg.mozilla.org/mozilla-central/rev/a9e2b417b631
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
(Assignee)

Updated

9 years ago
Blocks: 459708
(Assignee)

Comment 39

9 years ago
(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
(Assignee)

Comment 41

9 years ago
(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
(Assignee)

Updated

9 years ago
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.