allow native buttons to render at any size (much smaller minimum sizes)

RESOLVED FIXED

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Josh Aas, Assigned: Josh Aas)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

12.82 KB, patch
mano
: review+
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
Right now native buttons drawn with HITheme can only draw at height 17, 19, and 22 or higher. So, if we want a size 15, 16, 18, or 20 button for example, we can't draw that. This is an HITheme limitation.

To solve this problem, I propose drawing into an offscreen buffer and scaling the images such that we don't distort the endcaps.
(Assignee)

Comment 1

11 years ago
Created attachment 263283 [details] [diff] [review]
fix v0.9

This allows us to render any size greater than 20x12, a huge improvement over what was effectively 70x22.

If we need to do any offscreen scaling, we first resize to adjust height only and if we need to shrink the width we do that by "cutting out the middle" of the image to preserve the shape of endcaps.

This has one major bug remaining, and it could use a couple more comments. The bug is that since we now set min-width and min-height on chrome buttons to conform to Apple HIG, the button/icons in the URL bar got a lot taller and the URL bar is thus too tall. I haven't figured out how to stop that from happening yet.

Mano - any ideas?
set min-width/height for these in browser's browser.css; it already has rules for overriding -moz-appearance for these buttons.

Another approach would be to keep the current nsNativeTheme sizes in place for the xul namespace and not set min-width/height in button.css at all.
(Assignee)

Updated

11 years ago
Blocks: 376695
(Assignee)

Comment 3

11 years ago
Created attachment 263321 [details] [diff] [review]
fix v1.0

This fixes the URL bar issue with v0.9 and also fixes bug 376695.
Attachment #263283 - Attachment is obsolete: true
Attachment #263321 - Flags: review?(mano)
(Assignee)

Comment 4

11 years ago
Created attachment 263326 [details] [diff] [review]
fix v1.0.1

Need to make sure to draw into the CGContext we're given in DrawButton instead of the current graphics context. Thanks for pointing that out cbarrett.
Attachment #263321 - Attachment is obsolete: true
Attachment #263326 - Flags: review?(mano)
Attachment #263321 - Flags: review?(mano)
Comment on attachment 263326 [details] [diff] [review]
fix v1.0.1

Looks good to me. Excited about this!
Attachment #263326 - Flags: review+

Comment 6

11 years ago
Does this applies to disabled html buttons as well?
On my Camino build with patch v1.0, a disabled input[type="submit"] is 2px taller than the enabled one.
(Assignee)

Comment 7

11 years ago
I'd rather you not test this stuff in Camino until Camino stops using its own CSS for things. Because it uses that CSS bug reports from Camino don't really mean much.
Don't remove the margin around .button-text, it will break iconic buttons.

Also, s/0px/0/
(Assignee)

Comment 9

11 years ago
Created attachment 263379 [details] [diff] [review]
fix v1.0.2
Attachment #263326 - Attachment is obsolete: true
Attachment #263379 - Flags: review?(mano)
Attachment #263326 - Flags: review?(mano)
Comment on attachment 263379 [details] [diff] [review]
fix v1.0.2

r=mano on the toolkit & browser bits.
Attachment #263379 - Flags: review?(mano) → review+
(Assignee)

Updated

11 years ago
Attachment #263379 - Flags: superreview?(roc)
Comment on attachment 263379 [details] [diff] [review]
fix v1.0.2

sr=pink
Attachment #263379 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 12

11 years ago
landed on trunk
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
I backed this out because it broke 3 mochitests and no-one was around to clean up.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 14

11 years ago
Created attachment 263436 [details] [diff] [review]
fix v1.1

The unit test failures let me to finding some other things to fix.

First of all, the Apple HIG documentation says that the minimum button size should be 68, but that is a type and it is actually 79. I have filed that doc bug with Apple and fixed that in this patch.

Secondly, it turns out HITheme can render buttons at widths as small as 28. That is small enough for us so I made the minimum width 28 and it eliminates the need to ever make a second offscreen buffer for width scaling.

Third, I have changed the pinstripe button test itself to reflect the minimum chrome aqua button size, have better descriptions, and I eliminated the button-small test because that doesn't really mean anything any more (it doesn't have a different minimum size than a regular button).
Attachment #263379 - Attachment is obsolete: true
Attachment #263436 - Flags: review?(mano)
Comment on attachment 263436 [details] [diff] [review]
fix v1.1

assuming these huge buttons look OK ;)

Please file a bug on cleaning up button-small usage in Pinstripe, I would bet this breaks the appearance of the buttons in the toolbar customization dialog.
Attachment #263436 - Flags: review?(mano) → review+
(Assignee)

Comment 16

11 years ago
landed on trunk again, filing followup per comment #15
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Comment 17

11 years ago
+[NSGraphicsContext graphicsContextWithGraphicsPort:flipped:] was introduced in 10.4.  When running on 10.3, we get:

*** +[NSGraphicsContext graphicsContextWithGraphicsPort:flipped:]: selector not recognized

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/widget/src/cocoa/nsNativeThemeCocoa.mm&rev=1.24&mark=186#184

http://developer.apple.com/documentation/Cocoa/Reference/ApplicationKit/Classes/NSGraphicsContext_Class/Reference/Reference.html#//apple_ref/occ/clm/NSGraphicsContext/graphicsContextWithGraphicsPort:flipped:
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 18

11 years ago
checked in a compile fix that just renders to the current context, filed bug 379640 on rendering to the correct context
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Depends on: 382147
You need to log in before you can comment on or make changes to this bug.