407093 - draw NS_THEME_BUTTON using NSCell

Status: RESOLVED FIXED

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

Description

[Josh Aas]

We need to draw NS_THEME_BUTTON using NSCell.

Comment 1

[Josh Aas]

Attachment: fix v0.2

This is a work in progress. My first goal is to get a patch that does content with all square bevel buttons and chrome with all rounded bevel. That doesn't mean that is what will land. With this patch, content buttons work pretty well but there is no attempt at rendering chrome buttons.

Comment 2

[Josh Aas]

Attachment: fix v0.3

Bug fixes and more accurate rendering. Should be pixel-perfect for content buttons now. Standard-size chrome buttons should render now, non-standard size chrome buttons still won't render.

With this patch, we clip NSCell drawing to the actual frame. This ensures that we don't draw outside of our frame, which means that when NSCell renders stuff that is invisible to the naked eye outside of the frame we give it, that won't cause reftests to fail.

Comment 3

[Josh Aas]

Attachment: test app v1.0

This is the test app I have been using. The code is verbose, written to be easily modified for each case.

Comment 4

[Josh Aas]

Attachment: approach #1, v1.0

This is a complete version of what I'll label as approach #1. I'm not saying we should go with this strategy, but it is an option and this code can serve as a good baseline for other options. This approach has the advantage of having the cleanest and most robust implementation of all options to come.

With this approach #1, all content buttons render as square aqua buttons. This is flexible and consistent. Unlike Safari, content does not need to have more than one button type to fit sizing constraints. This button style takes a little getting used to but I think it is certainly good enough to consider, and at least for me I liked it more and more over time. The style seems to fit web content better than rounded aqua buttons do.

With this approach #1, chrome buttons that are requested at standard aqua sizes (normal, small, and mini) render as rounded aqua buttons. Any other size renders as the square aqua buttons. This is flexible and we avoid having to render fuzzier versions of the standard rounded aqua buttons due to scaling. This is also the approach that Apple is promoting - they don't even allow NSCell to render rounded aqua buttons at nonstandard sizes. Another advantage to this approach is that chrome authors will be able to tell easily if they have selected standard aqua sizes.

Since all of this push-button drawing is done with NSCell, all bugs that this blocks are fixed with this patch.

I realize that there are tradeoffs to doing this, which is why I am still undecided. For example, XUL authors would not be able to get rounded aqua push buttons at non-standard sizes. I'm not sure if we should care about that or not. It might be worth the benefits of this approach to forgo that.

Comment 5

[Josh Aas]

Attachment: screenshot #1, approach #1

Comment 6

[Josh Aas]

Comment on attachment 291851 [details] [diff] [review]
approach #1, v1.0

I am requesting review on this because we have to go in this direction anyway. Even if we choose to have rounded aqua buttons in content and at all sizes in chrome, this is a much better baseline to start with. Also, it would be nice to get feedback while we explore other options.

Comment 7

[Josh Aas]

Attachment: approach #1, v1.1

Comment 8

[Josh Aas]

"approach #1, v1.1" adds 10.4 compatibility

Comment 9

[Josh Aas]

Here is an opt universal try server build with "approach #1, v1.1":

https://build.mozilla.org/tryserver-builds/2007-12-06_09:[e-mail]-1196963438/[e-mail]

Comment 10

[Josh Aas]

After using my patch for a while, I think the best thing about this approach is that the square aqua buttons seem to fit in better with web content. Safari's round aqua buttons in web pages look gaudy to me now. Furthermore, fallback to generic gfx widgets is less visually jarring with my patch because both native and fallback widgets are square.

Comment 11

[Stuart Morgan]

Since this confused me, I want to clarify: this patch uses "bevel" buttons, which are distinct from "square" buttons (square is the style that Safari 3 uses for large buttons in content).

Comment 12

[Colin Barrett [:cbarrett]]

Comment on attachment 291865 [details] [diff] [review]
approach #1, v1.1


>+  NSButtonCell *cell = [[NSButtonCell alloc] initTextCell:nil];
>+  [cell setButtonType:NSMomentaryPushInButton];
>+  [cell setHighlightsBy:NSPushInCellMask];
>+  [cell setEnabled:!inDisabled];
>+  [cell setHighlighted:((inState & NS_EVENT_STATE_ACTIVE) && (inState & NS_EVENT_STATE_HOVER) || (inIsDefault && !inDisabled))];
>+  [cell setShowsFirstResponder:(inState & NS_EVENT_STATE_FOCUS)];
>+  [cell setTransparent:NO];

Are all of these necessary? I don't think the first two aren't needed as they are the defaults, and the last one does not make sense because there is no control view for this cell.

>+  if (!isHTML) {

>+  if (isHTML || isNonNaturalChromeSize) {

You should document the control flow a little bit. It's not exactly obvious what's happening right at first.

>+    // We have a separate rendering rect which is adjusted for the quirks of whatever
>+    // type of control we're drawing. Often they draw stuff we don't care about around
>+    // the part we do care about. Using this rect will get us the right size and we'll
>+    // clip out the cruft around what we want. You just have to use a test app to figure
>+    // out what the quirks for each control type are.

Maybe reference the bug # where people can find your test app?

>+        if (nsToolkit::OnLeopardOrLater()) {
>+          // On 10.5 there are two extra pixels on the bottom.
>+          renderRect.size.height += 2;
>+        }
>+        else {
>+          // There are also two extra pixels on the bottom on 10.4, but
>+          // we have to shave them off in the other direction because of
>+          // the CTM fix required for rounded bezel rendering on 10.4 below.
>+          renderRect.size.height -= 2;
>+        }

I'd write this as

// On 10.5 there are two extra pixels on the bottom.
// There are also two extra pixels on the bottom on 10.4, but
// we have to shave them off in the other direction because of
// the CTM fix required for rounded bezel rendering on 10.4 below.
renderRect.size.height += nsToolkit::OnLeopardOrLater() ? 2 : -2;

But it's up to you.

>+    [NSGraphicsContext saveGraphicsState];
>+    [NSGraphicsContext setCurrentContext:[NSGraphicsContext graphicsContextWithGraphicsPort:cgContext flipped:YES]];
>+
>+    // We clip to exactly the gecko rect so NSCell can't draw outside of it.
>+    [NSBezierPath clipRect:drawRect];

So I looked over the documentation and I think what I said earlier is wrong. I think since you are setting the clipping path you need to restore it after you draw.

The documentation for the class level saveGraphicsState says that it sends the current context a saveGraphicsState message and pushes the context onto a per-thread stack. The class-level restore pops that stack, sets whatever it finds as current, and sends it the restoreGraphicsStateMessage.

The instance-level ones push/pop the CTM, clipping rect, etc, on a per-context stack.

One thing to note is that on Leopard when you have QuartzGL on, the latter kind of saveGraphicsState becomes quite expensive, because all that information is stored on the GPU. We don't have to worry about that yet, because we can never get on fast paths because of what we have to do to support quickdraw plugins, but it's good to keep in mind.

Maybe what you want to do is this:

oldContext = [NSGraphicsContext currentContext]
[NSGraphicsContext setCurrentContext:...cgContext...]
[NSGraphicsContext saveGraphicsState]
//draw
[NSGraphicsContext restoreGraphicsState]
[NSGraphicsContext setCurrentContext:oldContext]


>+    // Square bezel buttons have a drawing bug that requires this transformation to be in place to deal
>+    // with our flipped coordinate systems.
>+    CGContextScaleCTM(cgContext, 1.0f, -1.0f);
>+    CGContextTranslateCTM(cgContext, 0.0f, -(2.0 * inBoxRect.origin.y + inBoxRect.size.height));


You should get rid of inBoxRect here. Move it above where you modify drawRect and use drawRect insted?

Also, say something like "On 10.4, bevel buttons don't draw correctly when origin at the top left" in the comment, to make it a little more clear what's going on.


>+      if (isHTMLFrame(aFrame))
>+        aResult->SizeTo(4, 2, 4, 3);
>+      else
>+        aResult->SizeTo(7, 2, 7, 4);
>       break;

Doesn't this mean that for buttons in chrome, the borders will be wrong when we fail at drawing push buttons and fall back to bevel buttons? Have you tested that case?




Exciting patch!! I hope some of the tricks we learn here can be applied to popups as well (they can draw in a style that looks similar to bevel buttons, it looks pretty good).

Also, it's possible that by doing some tricks with compositing operations, we can get a button that's got a color associated with it to look good and avoid falling back to gfx.

Comment 13

[Josh Aas]

(In reply to comment #11)
> Since this confused me, I want to clarify: this patch uses "bevel" buttons,
> which are distinct from "square" buttons (square is the style that Safari 3
> uses for large buttons in content).

The buttons in my approach #1 are technically called "NSRegularSquareBezelStyle", the ones you're talking about are "NSShadowlessSquareBezelStyle". That is why I called it what I did initially.

Comment 14

[Stuart Morgan]

I was going by IB names, which are apparently designed to cause maximum confusion with the constant names ;)

Comment 15

[Josh Aas]

Attachment: approach #2, v1.0

This is the same as approach #1 but it has a different button style for content, NSShadowlessSquareBezelStyle.

https://build.mozilla.org/tryserver-builds/2007-12-06_14:[e-mail]-1196980359/[e-mail]

Comment 16

[Josh Aas]

Note, new approaches are not necessarily things I think are an improvement. As I finish test implementations I'm just posting them there for others to see. Personally, I don't like approach #2 at all.

Comment 17

[Mike Pinkerton (not reading bugmail)]

how about a screenshot of approach #2? Does it look any visually different?

Also, what do selects look like? Your original image didn't show them at all (the More Actions dropdown in gmail is a div, not a real form control). 

Comment 18

[Josh Aas]

Selects are the same, the only thing that is different in these patches are push buttons. I'll get to selects after we've settled on a basic strategy for form controls. In approach #2, the buttons look like the big button style in WebKit.

Comment 19

[philippe (part-time)]

Attachment: Screen shot, approach #2

Gmail under approach #2 (with min. font-size set to 12px)

Comment 20

[philippe (part-time)]

Personally, I don't like approach 2 at all. That type eventually looks reasonable for multi-line buttons. But one line buttons look odd, and sunken into the page, esp. pages with a darkish background.

I've been using the patch for approach 1 on my own Camino build for the past few days (builds fine on 10.4.11 ppc/xcode 2.5, btw - in case you're worried). I'm gradually getting used to them. Not sure if it is with full enthusiasm, though.

Josh (comment 10) is certainly right when he states that those buttons integrate better with web pages (which are build of rectangular boxes or boxes with soft round corners). They also use less space on the page.
On the other hand, they look 'flatter'. The bevel/glass effect is more pronounced on the pill shaped buttons. That gives them (the traditional buttons) some more depth.

(and it is sure a huge change, more so for Camino users)

Comment 21

[Josh Aas]

I am working on some better options, will have new ones up soon. I have made a lot of progress and learned quite a bit but I'm not ready to summarize it here yet.

Comment 22

[Josh Aas]

Attachment: approach #3, v1.0

Classic rounded aqua buttons, scales to all sizes in chrome and content.

Comment 23

[Colin Barrett [:cbarrett]]

Comment on attachment 293892 [details] [diff] [review]
approach #3, v1.0


>+int controlSizeToEnum(NSControlSize cocoaConstant) {
>+  if (cocoaConstant == NSMiniControlSize)
>+    return miniControl;
>+  else if (cocoaConstant == NSSmallControlSize)
>+    return smallControl;
>+  else
>+    return normalControl;
>+}

Why not go the other way, and use the enum in the function below and then just translate once to the control size? Also this should probably be static (and/or inline).

>+static void inflatePushButtonRect(NSRect* rect, int controlSize)
>+{
>+  static int osIndex = nsToolkit::OnLeopardOrLater() ? leopardOS : tigerOS;
>+  rect->origin.x -= pushButtonMargins[osIndex][controlSize][leftMargin];
>+  rect->origin.y -= pushButtonMargins[osIndex][controlSize][bottomMargin];
>+  rect->size.width += pushButtonMargins[osIndex][controlSize][leftMargin] + pushButtonMargins[osIndex][controlSize][rightMargin];
>+  rect->size.height += pushButtonMargins[osIndex][controlSize][bottomMargin] + pushButtonMargins[osIndex][controlSize][topMargin];
>+}

Should this be inline? IIRC the compiler will inline a static if there is only one caller...

>+  if (drawRect.size.height <= NATURAL_MINI_ROUNDED_BUTTON_HEIGHT &&
>+      drawRect.size.width >= NATURAL_MINI_ROUNDED_BUTTON_MIN_WIDTH) {
>+    controlSize = NSMiniControlSize;
>+    if (drawRect.size.height == NATURAL_MINI_ROUNDED_BUTTON_HEIGHT) {
>+      doScaling = NO;
>+      // Just inflate the rect by the margin for the control.
>+      inflatePushButtonRect(&cellRenderRect, controlSizeToEnum(controlSize));
>+    }
>+    else {
>+      // The initial buffer size is the natural height of the control and either the target
>+      // width or the minimum width of the control.
>+      initialBufferSize = NSMakeSize(drawRect.size.width, NATURAL_MINI_ROUNDED_BUTTON_HEIGHT);
>+      if (initialBufferSize.width < NATURAL_MINI_ROUNDED_BUTTON_MIN_WIDTH)
>+        initialBufferSize.width = NATURAL_MINI_ROUNDED_BUTTON_MIN_WIDTH;
>+
>+      // Scale the initial buffer width up to compensate for the down-scaling we'll do.
>+      float scaleFactor = drawRect.size.height / NATURAL_MINI_ROUNDED_BUTTON_HEIGHT;
>+      if (scaleFactor != 0.0)
>+        initialBufferSize.width = initialBufferSize.width / scaleFactor;
>+
>+      bufferRenderRect.size = initialBufferSize;
>+      inflatePushButtonRect(&bufferRenderRect, controlSizeToEnum(controlSize));
>+    }
>+  }
>+  else if (drawRect.size.height <= NATURAL_SMALL_ROUNDED_BUTTON_HEIGHT &&
>+           drawRect.size.width >= NATURAL_SMALL_ROUNDED_BUTTON_MIN_WIDTH) {
>+    controlSize = NSSmallControlSize;
>+    if (drawRect.size.height == NATURAL_SMALL_ROUNDED_BUTTON_HEIGHT) {
>+      doScaling = NO;
>+      // Just inflate the rect by the margin for the control.
>+      inflatePushButtonRect(&cellRenderRect, controlSizeToEnum(controlSize));
>+    }
>+    else {
>+      // The initial buffer size is the natural height of the control and either the target
>+      // width or the minimum width of the control.
>+      initialBufferSize = NSMakeSize(drawRect.size.width, NATURAL_SMALL_ROUNDED_BUTTON_HEIGHT);
>+      if (initialBufferSize.width < NATURAL_SMALL_ROUNDED_BUTTON_MIN_WIDTH)
>+        initialBufferSize.width = NATURAL_SMALL_ROUNDED_BUTTON_MIN_WIDTH;
>+
>+      // Scale the initial buffer width up to compensate for the down-scaling we'll do.
>+      float scaleFactor = drawRect.size.height / NATURAL_SMALL_ROUNDED_BUTTON_HEIGHT;
>+      if (scaleFactor != 0.0)
>+        initialBufferSize.width = initialBufferSize.width / scaleFactor;
>+
>+      bufferRenderRect.size = initialBufferSize;
>+      inflatePushButtonRect(&bufferRenderRect, controlSizeToEnum(controlSize));
>+    }
>+  }
>+  else {
>+    controlSize = NSRegularControlSize;
>+    if (drawRect.size.height == NATURAL_NORMAL_ROUNDED_BUTTON_HEIGHT &&
>+        drawRect.size.width >= NATURAL_NORMAL_ROUNDED_BUTTON_MIN_WIDTH) {
>+      doScaling = NO;
>+      // Just inflate the rect by the margin for the control.
>+      inflatePushButtonRect(&cellRenderRect, controlSizeToEnum(controlSize));
>+    }
>+    else {
>+      // The initial buffer size is the natural height of the control and either the target
>+      // width or the minimum width of the control.
>+      initialBufferSize = NSMakeSize(drawRect.size.width, NATURAL_NORMAL_ROUNDED_BUTTON_HEIGHT);
>+      if (initialBufferSize.width < NATURAL_NORMAL_ROUNDED_BUTTON_MIN_WIDTH)
>+        initialBufferSize.width = NATURAL_NORMAL_ROUNDED_BUTTON_MIN_WIDTH;
>+
>+      // Scale the initial buffer width up to compensate for the down-scaling we'll do.
>+      float scaleFactor = drawRect.size.height / NATURAL_NORMAL_ROUNDED_BUTTON_HEIGHT;
>+      if (scaleFactor != 0.0)
>+        initialBufferSize.width = initialBufferSize.width / scaleFactor;
>+
>+      bufferRenderRect.size = initialBufferSize;
>+      inflatePushButtonRect(&bufferRenderRect, controlSizeToEnum(controlSize));
>+    }
>+  }

There is a LOT of duplicated code here... can you use the same strategy and pack those constants into arrays indexed on control size and remove the code duplication?

>+    // Resize the image to the final size.
>+    [buffer setScalesWhenResized:YES];
>+    [buffer setSize:drawRect.size];

Should we explicitly set the image interpolation factor here? It's not part of the graphics state, according to the documentation. The high quality one would probably be best, but it might negatively impact performance especially on repeated redraws.

>+    CGContextScaleCTM(cgContext, 1.0f, -1.0f);
>+    CGContextTranslateCTM(cgContext, 0.0f, -(2.0 * drawRect.origin.y + drawRect.size.height));

Is this needed on both Tiger and Leopard? Also, please put a comment (in both places) saying that it flips the image in place, and that it is necessary to work around a bug in the way Apple draws buttons.

Comment 24

[Josh Aas]

Attachment: approach #3, v1.1

Comment 25

[Colin Barrett [:cbarrett]]

Comment on attachment 293958 [details] [diff] [review]
approach #3, v1.1

>+  // Figure out what size cell control we're going to draw.
>+  int controlSize = NSRegularControlSize;
>+  if (drawRect.size.height <= NATURAL_MINI_ROUNDED_BUTTON_HEIGHT &&
>+      drawRect.size.width >= NATURAL_MINI_ROUNDED_BUTTON_MIN_WIDTH) {
>+    controlSize = NSMiniControlSize;
>+  }
>+  else if (drawRect.size.height <= NATURAL_SMALL_ROUNDED_BUTTON_HEIGHT &&
>+           drawRect.size.width >= NATURAL_SMALL_ROUNDED_BUTTON_MIN_WIDTH) {
>+    controlSize = NSSmallControlSize;
>+  }
>+  [cell setControlSize:controlSize];
>+
>+  // Get the natural height and minimum width is for the given control size.
>+  int enumControlSize = enumSizeForCocoaSize(controlSize);
>+  float naturalHeight = NATURAL_NORMAL_ROUNDED_BUTTON_HEIGHT;
>+  float minWidth = NATURAL_NORMAL_ROUNDED_BUTTON_MIN_WIDTH;
>+  if (enumControlSize == miniControlSize) {
>+    naturalHeight = NATURAL_MINI_ROUNDED_BUTTON_HEIGHT;
>+    minWidth = NATURAL_MINI_ROUNDED_BUTTON_MIN_WIDTH;
>+  }
>+  else if (enumControlSize == smallControlSize) {
>+    naturalHeight = NATURAL_SMALL_ROUNDED_BUTTON_HEIGHT;
>+    minWidth = NATURAL_SMALL_ROUNDED_BUTTON_MIN_WIDTH;
>+  }

Can't these two be combined? i.e.

>+  int controlSize = NSRegularControlSize;
>+  float naturalHeight = NATURAL_NORMAL_ROUNDED_BUTTON_HEIGHT;
>+  float minWidth = NATURAL_NORMAL_ROUNDED_BUTTON_MIN_WIDTH;
>+  if (drawRect.size.height <= NATURAL_MINI_ROUNDED_BUTTON_HEIGHT &&
>+      drawRect.size.width >= NATURAL_MINI_ROUNDED_BUTTON_MIN_WIDTH) {
>+    controlSize = NSMiniControlSize;
>+    naturalHeight = NATURAL_MINI_ROUNDED_BUTTON_HEIGHT;
>+    minWidth = NATURAL_MINI_ROUNDED_BUTTON_MIN_WIDTH;
>+  }
>+  else if (drawRect.size.height <= NATURAL_SMALL_ROUNDED_BUTTON_HEIGHT &&
>+           drawRect.size.width >= NATURAL_SMALL_ROUNDED_BUTTON_MIN_WIDTH) {
>+    controlSize = NSSmallControlSize;
>+    naturalHeight = NATURAL_SMALL_ROUNDED_BUTTON_HEIGHT;
>+    minWidth = NATURAL_SMALL_ROUNDED_BUTTON_MIN_WIDTH;
>+  }


>+  // Render, by scaling if the target height is not the natural height of the control we're drawing.
>+  if (drawRect.size.height != naturalHeight) {

I think it's better for readability to change this != to an == and switch the order of the blocks, for two reasons: 1) less negations 2) having the shorter block closer to the if

I don't have any other comments, change those two things and r=me.

Comment 26

[Josh Aas]

Attachment: fix v1.2

Comment 27

[Josh Aas]

Attachment: fix v1.3

Comment 28

[Josh Aas]

Attachment: fix v1.4

one more little optimization, I swear I'm done now

Comment 29

[philippe (part-time)]

With the v1.4 patch I ran into a build error
XCode 2.5, 10.4.11ppc, Camino build

/Users/phiw/camino-build/mozilla/widget/src/cocoa/nsNativeThemeCocoa.mm: In member function 'void nsNativeThemeCocoa::DrawPushButton(CGContext*, const HIRect&, PRBool, PRBool, PRInt32)':
/Users/phiw/camino-build/mozilla/widget/src/cocoa/nsNativeThemeCocoa.mm:249: error: invalid conversion from 'int' to 'NSControlSize'
/Users/phiw/camino-build/mozilla/widget/src/cocoa/nsNativeThemeCocoa.mm:255: error: invalid conversion from 'int' to 'NSControlSize'
/Users/phiw/camino-build/mozilla/widget/src/cocoa/nsNativeThemeCocoa.mm:255: error:   initializing argument 2 of 'void inflatePushButtonRect(NSRect*, NSControlSize)'
/Users/phiw/camino-build/mozilla/widget/src/cocoa/nsNativeThemeCocoa.mm:289: error: invalid conversion from 'int' to 'NSControlSize'
/Users/phiw/camino-build/mozilla/widget/src/cocoa/nsNativeThemeCocoa.mm:289: error:   initializing argument 2 of 'void inflatePushButtonRect(NSRect*, NSControlSize)'
make[6]: *** [nsNativeThemeCocoa.o] Error 1

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+static inline int enumSizeForCocoaSize(NSControlSize cocoaControlSize) {
+static inline void inflatePushButtonRect(NSRect* rect, NSControlSize cocoaControlSize)

Don't use 'inline' for static methods like this. And they should have a leading capital letter.

Don't forget to use -p in your diffs.

+  rect->origin.x -= pushButtonMargins[osIndex][controlSize][leftMargin];
+  rect->origin.y -= pushButtonMargins[osIndex][controlSize][bottomMargin];
+  rect->size.width += pushButtonMargins[osIndex][controlSize][leftMargin] + pushButtonMargins[osIndex][controlSize][rightMargin];
+  rect->size.height += pushButtonMargins[osIndex][controlSize][bottomMargin] + pushButtonMargins[osIndex][controlSize][topMargin];

Factor out pushButtonMargins[osIndex][controlSize].

Would it make sense to cache the NSButtonCell somewhere?

+    NSSize initialBufferSize = NSMakeSize(drawRect.size.width, naturalHeight);
+    if (initialBufferSize.width < minWidth)
+      initialBufferSize.width = minWidth;

Use PR_MAX to set the width

+    NSGraphicsContext* savedContext = [NSGraphicsContext currentContext];
+    [NSGraphicsContext setCurrentContext:[NSGraphicsContext graphicsContextWithGraphicsPort:cgContext flipped:YES]];
+    [NSGraphicsContext saveGraphicsState];
+
+    // We clip to exactly the gecko rect to make sure we don't draw outside of it.
+    [NSBezierPath clipRect:drawRect];
+
+    // This flips the image in place and is necessary to work around a bug in the way
+    // NSButtonCell draws buttons.
+    CGContextScaleCTM(cgContext, 1.0f, -1.0f);
+    CGContextTranslateCTM(cgContext, 0.0f, -(2.0 * drawRect.origin.y + drawRect.size.height));

Share this code.

So these buttons are always fully opaque?

Comment 31

[Josh Aas]

Attachment: fix v1.5

Comment 32

[Josh Aas]

Attachment: fix v1.6

Yes, it seems as though the buttons are always opaque.

Comment 33

[Colin Barrett [:cbarrett]]

Comment on attachment 294072 [details] [diff] [review]
fix v1.6

I really don't think it is necessary to cache the cell. They are not expensive objects to create.

http://www.mikeash.com/?page=pyblog/performance-comparisons-of-common-operations.html

I worry about the cell getting into a strange state, it would really be better to create a new one each time.

I'll check and see what WebKit does.

Comment 34

[Josh Aas]

I don't see how it could get into a strange state, until we have some evidence that it could happen lets just cache it.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If the caching version is just as simple we might as well do it. If it's more complex, then forget it until we have profile data.

Comment 36

[philippe (part-time)]

fwiw - patch v1.6 compiled just fine with XCode 2.5, 10.4.11ppc (Camino build).

Comment 37

[philippe (part-time)]

Attachment: screen shot

Is this the intended behaviour ? 
(esp the multiline button and the button with larger height (24px+)

screenshot from
http://dev.l-c-n.com/forms/button-height.html

Comment 38

[Josh Aas]

philippe - thanks! That is not the intended behavior. For some reason I thought layout picked the other button type to render when it was necessary, and none of my many test cases had any multiline buttons that looked bad with my current patch. I did some tests and I think if we render the square button type when the height is > 26px we'll be good. I picked that number by looking at various sites and looking at WebKit's behavior.

I'll have a new patch up soon.

Comment 39

[Josh Aas]

Attachment: fix v1.7

Comment 40

[philippe (part-time)]

(In reply to comment #39)
> Created an attachment (id=294271) [details]
> fix v1.7
> 
Ah, that is much better.

about the height: WebKit (latest build and Safari 3.04) switches to the square button when the (computed) height is 22px seen from here (10.4.11 ppc).

I've updated my test file to include height:22px and height:20px.
http://dev.l-c-n.com/forms/button-height.html

Comment 41

[Josh Aas]

Yeah, I saw that WebKit does it at 22px but we can do better. It looks fine at 26 with this type of scaling and at that size the square style is still not important.

Comment 42

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I'm noticing that we're still clipping the Aqua focus rings on buttons with "fix v1.7"; is that something that should be fixed here, or a new bug?

Comment 43

[philippe (part-time)]

'patch v1.7' almost solves those faint vertical lines that are sometimes visible on the left-side of the right end-cap on dark backgrounds (bug 384984). 'Almost', because I still can reproduce the issue for disabled buttons, at some font-sizes/height/width.

Comment 44

[Colin Barrett [:cbarrett]]

(In reply to comment #42)
> I'm noticing that we're still clipping the Aqua focus rings on buttons with
> "fix v1.7"; is that something that should be fixed here, or a new bug?

We may need to increase the size of the rendered rect after all.

I had to do this in my patch for dropdowns. Increase the size of the buffer, shift buffer's CTM by x+4, y-4, draw, shift the context's CTM by x-4, y+4, draw the image into the buffer.

The odd signs are because when you are drawing your buffer image into cgContext  you have flipped the origin. It can get a little confusing, so have a whiteboard at the ready.

Comment 45

[Josh Aas]

Yeah, I know this doesn't solve our problems with focus rings. I'd rather do that in a separate bug. There is more than just clipping that needs to be figured out.

landed on trunk

Comment 46

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #45)
> Yeah, I know this doesn't solve our problems with focus rings. I'd rather do
> that in a separate bug. There is more than just clipping that needs to be
> figured out.

Filed bug 409912 on the focus ring clipping.