Style new search widgets on Mac OS X

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Toolkit
Themes
VERIFIED FIXED
10 years ago
7 years ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

(Depends on: 1 bug, {polish, verified1.9.1})

Trunk
mozilla1.9.2a1
All
Mac OS X
polish, verified1.9.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 15 obsolete attachments)

273 bytes, application/vnd.mozilla.xul+xml
Details
12.20 KB, patch
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
25.84 KB, patch
smichaud
: review+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
The search fields that have been given to us by bug 388811 should be polished a little, probably using the new border-image capabilities.

These things should be fixed:
- Focus rings don't look good. (Bug 448767 adds the possibility of using a
  different image in graphite mode, so this can finally be done using images.)
- The search icon should always be on the left. Currently that's not the case
  for search fields with a button. See bug 449465 comment 7.
Created attachment 334049 [details]
Better Search Field

I have done a bit of work on this already but it isn't quite finished.

The search field is built with border-image and I have been toying around with using box-shadow to get a better focus ring.
Created attachment 334079 [details]
Searchfield construction images

Box shadow is nice but I don't think it's suited for focus rings; we'll never get quite the right look that way.

Is there a reason you don't want to use images for the focus rings, Stephen?

I've generated some alpha transparent images from the native search fields and attached them. They're ready to be used as border-images.
(In reply to comment #2)
> Box shadow is nice but I don't think it's suited for focus rings; we'll never
> get quite the right look that way.
> 
> Is there a reason you don't want to use images for the focus rings, Stephen?

No it will always look a little "off". Although it's really pretty close.

The reason I was exploring using box-shadow is that I ran into some issue with using the image-border. There is an extra 3-4 pixels required for the focus ring so you have to do some negative padding stuff and there was some shifting and wierd redraw problems when actually focusing the search field. 

I seem to recall a wierd background problem I had as well but most of the work was done on the plane after my 8 hour bus ride so it's a bit hazy :)

I do have both ways worked up and I got the image-border working well enough. I am going to finish it and post a patch.
Status: NEW → ASSIGNED
> The reason I was exploring using box-shadow is that I ran into some issue with
> using the image-border. There is an extra 3-4 pixels required for the focus
> ring so you have to do some negative padding stuff and there was some shifting
> and wierd redraw problems when actually focusing the search field. 

Ah. If you find reproducible redraw problems, please file a bug for them. (Although that might be covered by bug 323255.)

Would it be a good idea to have the unfocused border image offset by the same margin as the focused one? Then you could be sure that focusing the search field doesn't shift it.

> I do have both ways worked up and I got the image-border working well enough. I
> am going to finish it and post a patch.

Nice.
Stephen, how's it going? I can take this bug if you have no time for it.
Stealing.
I'd like to do this with -moz-appearance: searchfield; or similar. Cocoa provides us with searchfield drawing functions, we should use them.
Assignee: nobody → mstange
Created attachment 341692 [details] [diff] [review]
v1, wip

This patch applies on top of bug 394892.
Created attachment 341771 [details] [diff] [review]
rendering part, v2

I had to shuffle some things to avoid painting errors on 10.4.
Attachment #341692 - Attachment is obsolete: true
Attachment #341771 - Flags: review?(joshmoz)
Attachment #341771 - Attachment description: rendering part, v1 → rendering part, v2
Created attachment 341772 [details] [diff] [review]
css part, v2

This is the CSS part. I used the "small" size as default and the "regular" size in the Places Library and the Applications pane in the preferences window. Since they're a different size, they need a regular-sized cancel button, too.
Attachment #334049 - Attachment is obsolete: true
Attachment #334079 - Attachment is obsolete: true
Attachment #341772 - Flags: review?(mconnor)
Comment on attachment 341772 [details] [diff] [review]
css part, v2

> #placesToolbar {
>-  margin-top: -3px;
>-  padding: 0 4px;
>+  padding: 0 4px 3px 4px;

nit: I find "0 4px 3px" better readable when the left and right padding is equal.

> textbox[empty="true"] {
>-  color: GrayText;
>+  color: #808080;
> }

hm?
Comment on attachment 341771 [details] [diff] [review]
rendering part, v2

This patch introduces rendering problems when opacity is involved; it's probably because I removed that context setup:
-    // Set up the graphics context we've been asked to draw to.
-    [NSGraphicsContext setCurrentContext:[NSGraphicsContext graphicsContextWithGraphicsPort:cgContext flipped:YES]];

I need to look into this again.
Attachment #341771 - Flags: review?(joshmoz)
(In reply to comment #10)
> (From update of attachment 341772 [details] [diff] [review])
> > #placesToolbar {
> >-  margin-top: -3px;
> >-  padding: 0 4px;
> >+  padding: 0 4px 3px 4px;
> 
> nit: I find "0 4px 3px" better readable when the left and right padding is
> equal.

I always forget what happens when only three values are specified, but now I realize that it's not that hard to remember. :)
I'll change it in the next version of the patch.

> > textbox[empty="true"] {
> >-  color: GrayText;
> >+  color: #808080;
> > }
> 
> hm?

GrayText is too dark, so I specified the right color. Is there a platform color that's equal to #808080?
Currently GrayText is what makes most sense semantically. Please file a new bug if you think there should be another native text color.
(In reply to comment #13)
> Currently GrayText is what makes most sense semantically. Please file a new bug
> if you think there should be another native text color.

Why is there a need for it to make sense semantically? Mac OS X doesn't have themes, the color is always the same.
> Why is there a need for it to make sense semantically?

Because we want to use the right colors in the appropriate places. We don't want #808080 here and #838383 there with hardly anybody understanding what's going on. GrayText as the general rule for disabled text and e.g. -moz-mac-placeholder-text as an exception for textboxes makes much more sense and helps maintainability / reuseability.

> Mac OS X doesn't have themes

There's graphite at least, but even the things that currently stay the same could change with a future OS X release.
Created attachment 344343 [details] [diff] [review]
rendering part, v3

Urgh. Making NSCells draw and scale correctly under both Tiger and Leopard is not easy.

I think I've fixed all cases now. The code may look a little messy but it works.

The "useNSImage" code path is also necessary for bug 399030 - NSPopUpButtonCells are just as buggy as NSSearchFieldCells.
Attachment #341771 - Attachment is obsolete: true
Attachment #344343 - Flags: review?(joshmoz)
Created attachment 344347 [details] [diff] [review]
add -moz-mac-placeholder-text platform color

as requested by Dão in comment 15
Attachment #344347 - Flags: review?(joshmoz)
Created attachment 344362 [details] [diff] [review]
css part, v3
Attachment #344362 - Flags: review?(rflint)
Attachment #341772 - Attachment is obsolete: true
Attachment #341772 - Flags: review?(mconnor)
Comment on attachment 344362 [details] [diff] [review]
css part, v3

this makes Search-close.png and Search-glass.png unused
What about the searchbutton mode? Looks like the clickable icon isn't styled anymore.
(In reply to comment #19)
> (From update of attachment 344362 [details] [diff] [review])
> this makes Search-close.png and Search-glass.png unused

Oh, right. I'll remove them.

(In reply to comment #20)
> What about the searchbutton mode? Looks like the clickable icon isn't styled
> anymore.

Exactly. That's what Alex Faaborg suggested in bug 449465 comment 12.
Josh and Ryan, can we expect a review of both of you soon? Thanks.
Flags: wanted1.9.1?
The rendering patch (attachment 344343 [details] [diff] [review]) needs to be updated for bug 458961. I'll do that today.
Comment on attachment 344343 [details] [diff] [review]
rendering part, v3

Means the other ones are still valid and ready for review?
Attachment #344343 - Attachment is obsolete: true
Attachment #344343 - Flags: review?(joshmoz)
Created attachment 347826 [details] [diff] [review]
rendering part, v3.1
Attachment #347826 - Flags: review?(joshmoz)
tchung, can we somehow drive this bug forward so we can get review asap? It would be great to have the new native style ready for 3.1.

Comment 27

10 years ago
setting TM to beta 2.   although polish, we just need this reviewed and hopefully landed soon.
Target Milestone: --- → mozilla1.9.1b2
Comment on attachment 347826 [details] [diff] [review]
rendering part, v3.1

I can do better. A new patch will follow soon.
Attachment #347826 - Flags: review?(joshmoz)
(In reply to comment #12)
> GrayText is too dark, so I specified the right color. Is there a platform color
> that's equal to #808080?

crashreporter is using something different at least [1]. Maybe graytext isn't implemented correctly in the first place [2]?

[1] http://mxr.mozilla.org/mozilla-central/search?string=disabledControlTextColor
[2] http://mxr.mozilla.org/mozilla-central/search?string=graytext&find=cocoa

See also http://developer.apple.com/documentation/macos8/HumanInterfaceToolbox/AppManager/ProgWithAppearanceMgr/Appearance.af.html

Comment 30

10 years ago
Crashreporter is using the nsColor disabledControlTextColor.

Comment 31

10 years ago
uh, right - dao basically said that...
(In reply to comment #29)
> crashreporter is using something different at least [1]. Maybe graytext isn't
> implemented correctly in the first place [2]?

It looks like [NSColor disabledControlTextColor] is the same as kThemeTextColorDialogInactive: #7f7f7f; which is almost exactly #808080. No idea why I thought it was too dark... let's just use GrayText as-is.
Created attachment 348429 [details] [diff] [review]
css part, v4
Attachment #344347 - Attachment is obsolete: true
Attachment #344362 - Attachment is obsolete: true
Attachment #348429 - Flags: review?(dao)
Attachment #344347 - Flags: review?(joshmoz)
Attachment #344362 - Flags: review?(rflint)
Created attachment 348430 [details] [diff] [review]
rendering part, v4

With the double-flip strategy I can avoid drawing problems on 10.4, so the NSImage stuff of the previous patch isn't necessary. I'll post a more detailed analysis in bug 465069.

This patch is on top of Steven's first patch in bug 464589.
Attachment #347826 - Attachment is obsolete: true
Attachment #348430 - Flags: superreview?(roc)
Attachment #348430 - Flags: review?(smichaud)
Comment on attachment 348429 [details] [diff] [review]
css part, v4

Where do the terms "regular" and "small" come from? Did you make them up? If it defaults to "small", maybe "small" should be "regular" and "regular" should be "big"?

Moreover, there's a lot of duplicate CSS. Just introduce a "size" attribute and handle it in textbox.css?
(In reply to comment #35)
> (From update of attachment 348429 [details] [diff] [review])
> Where do the terms "regular" and "small" come from?

They're from the Cocoa Interface Builder. Nearly every control comes in three sizes: "regular", "small" and "mini". They're also mentioned in the HIG:
http://developer.apple.com/documentation/UserExperience/Conceptual/AppleHIGuidelines/XHIGControls/chapter_19_section_1.html#//apple_ref/doc/uid/TP30000359-TP6
(last paragraph)

> Moreover, there's a lot of duplicate CSS. Just introduce a "size" attribute and
> handle it in textbox.css?

I hate introducing more visual XUL attributes... :(
But you're right about the duplication, so I'll do it.

(IMO it should be the theme that decides about the control's size, not the XUL code. See also bug 231313 comment 33 - we need CSS templates or something similar.)
Then the attribute should be called "cocoa-size" or so, or use a cocoa-small class name.

http://developer.apple.com/documentation/UserExperience/Conceptual/AppleHIGuidelines/XHIGControls/chapter_19_section_1.html#//apple_ref/doc/uid/TP30000359-TP6 suggests that "Most applications look best with regular-size controls", so I wonder if this shouldn't be our default, even if there are more instances of small search fields in Firefox.

Comment 38

10 years ago
fwiw, the textbox has a size attribute: https://developer.mozilla.org/en/XUL/textbox#a-size
Attachment #348430 - Flags: superreview?(roc) → superreview+
Created attachment 348553 [details] [diff] [review]
css part, v5
Attachment #348429 - Attachment is obsolete: true
Attachment #348553 - Flags: review?(dao)
Attachment #348429 - Flags: review?(dao)
Comment on attachment 348553 [details] [diff] [review]
css part, v5

>diff --git a/browser/themes/pinstripe/browser/places/organizer.css b/browser/themes/pinstripe/browser/places/organizer.css

> #placesToolbar {
>-  margin-top: -3px;
>-  padding: 0 4px;
>+  padding: 0 4px 3px 4px;
> }

nit: padding: 0 4px 3px;

>diff --git a/toolkit/themes/pinstripe/global/textbox.css b/toolkit/themes/pinstripe/global/textbox.css

> textbox[type="search"] {
>+  -moz-appearance: searchfield;
>+  border: none;
>+  padding: 2px 1px 1px 16px;

What's border:none needed for?
Is the padding right for rtl?

> .textbox-search-clear {
>-  list-style-image: url(chrome://global/skin/icons/Search-close.png);
>+  list-style-image: url(chrome://global/skin/icons/searchfield-regular-cancel.png);
>+  -moz-image-region: rect(0px, 14px, 14px, 0px);

nit: 0 rather than 0px

>+textbox[type="search"][cocoa-size="small"] .textbox-search-clear {

textbox[type="search"][cocoa-size="small"] > .textbox-search-icons > .textbox-search-clear
Created attachment 348557 [details] [diff] [review]
css part, v6

(In reply to comment #40)
> What's border:none needed for?

Looks like it's not needed.

> Is the padding right for rtl?

I think so. Cocoa always draws search icon on the left; I don't think there's a way to change that.
But now I see that in RTL mode both the search icon and the cancel button are on the left... well I guess we can leave it that way for now.
Attachment #348553 - Attachment is obsolete: true
Attachment #348557 - Flags: review?(dao)
Attachment #348553 - Flags: review?(dao)

Updated

10 years ago
Attachment #348557 - Flags: review?(dao) → review+
Created attachment 348558 [details] [diff] [review]
css part, v6.1

> textbox[type="search"][cocoa-size="small"] > .textbox-search-icons >
> .textbox-search-clear

It has to be
textbox[type="search"][cocoa-size="small"] > .textbox-input-box >
.textbox-search-icons > .textbox-search-clear
Attachment #348557 - Attachment is obsolete: true
Attachment #348558 - Flags: review?(dao)

Updated

10 years ago
Attachment #348558 - Flags: review?(dao) → review+
Comment on attachment 348558 [details] [diff] [review]
css part, v6.1

(In reply to comment #41)
> > Is the padding right for rtl?
> 
> I think so. Cocoa always draws search icon on the left; I don't think there's a
> way to change that.
> But now I see that in RTL mode both the search icon and the cancel button are
> on the left... well I guess we can leave it that way for now.

When this lands, please file a bug on either drawing the search icon at the logical start or locking the cancel button at the right.
Comment on attachment 348430 [details] [diff] [review]
rendering part, v4

+// This method is called when drawing search fields.
+@implementation NSView (SearchfieldDrawingWorkaround)
+- (NSText*)currentEditor { return nil; }
+@end

We need to know more about why this is necessary.

Furthermore, it may interfere with embedders.  So is it possible to
narrow the focus?  For example, will it suffice to add this
currentEditor method to the ChildView class?

(I'm building this patch and will be testing it.  More comments
later.)
(In reply to comment #44)
> For example, will it suffice to add this
> currentEditor method to the ChildView class?

That should work. I think it's called on the view that's passed to the inView: argument when the cell is drawn.
(In reply to comment #45)

Good.  But why do you need to make even [ChildView currentEditor:]
return nil?  What problem does it resolve?
(In reply to comment #46)
> What problem does it resolve?

If I don't implement that method an exception is thrown:
*** -[ChildView currentEditor]: unrecognized selector sent to instance ...

And if I just wrap the call in a @try {} the search glass icon isn't drawn.
Created attachment 348782 [details] [diff] [review]
css part, v6.2

The searchfields in the history / bookmarks sidebar also need cocoa-size="small".
Attachment #348558 - Attachment is obsolete: true
Comment on attachment 348430 [details] [diff] [review]
rendering part, v4

+// This method is called when drawing search fields.
+@implementation NSView (SearchfieldDrawingWorkaround)
+- (NSText*)currentEditor { return nil; }
+@end

In my tests I moved this currentEditor method to the ChildView class.
I had no problems.

I've also figured out why it's needed (and that it belongs in the
ChildView class):  [NSCell drawWithFrame:inView:] expects an NSControl
as its inView parameter, and may call [NSControl currentEditor] on it.
But since a ChildView object (like an NSView object) isn't a control,
it doesn't have a "current editor", or a currentEditor method.  So
calling currentEditor on it will trigger a Objective-C "unrecognized
selector" exception.  To prevent this, ChildView needs its own
currentEditor method.  Since a ChildView object never has a "current
editor", it should always return nil.

-  float diffWidth = rectWidth - sizes[smallerControlSizeIndex].width;
-  float diffHeight = rectHeight - sizes[smallerControlSizeIndex].height;
+  const NSSize size = sizes[smallerControlSizeIndex];
+  float diffWidth = size.width ? rectWidth -
                     sizes[smallerControlSizeIndex].width : 0.0f;
+  float diffHeight = size.height ? rectHeight -
                     sizes[smallerControlSizeIndex].height : 0.0f;

The last two lines should really be:

+  float diffWidth = size.width ? rectWidth - size.width : 0.0f;
+  float diffHeight = size.height ? rectHeight - size.height : 0.0f;

I suspect the following lines aren't necessary.  In other words, I
strongly suspect that bug 463805 comment #0 is simply wrong, and that
it's correct to pass a 'nil' to [NSCell drawWithFrame:inView:] as the
inView: parameter when [NSView focusView] returns nil (as the existing
code does).

But Apple doesn't fully (or fully enough) document the behavior of
[NSCell drawWithFrame:inView:] -- so I can't prove my point.  (Apple's
doc on this method says the following:  "This method draws the cell in
the currently focused view, which can be different from the
controlView passed in.  Taking advantage of this behavior is not
recommended, however."  This is ambiguous, but I interpret it as
saying that you should always pass the result of [NSView focusView] as
the inView: parameter.  If [NSView focusView] returns nil (as Apple
acknowleges it sometimes does), [NSCell drawWithFrame:inView:] will
deal with that.)

+  // If focusView is nil, use the frame's view as a fallback. If that's nil, too,
+  // we'll get drawing bugs on 10.4 and we can't do anything about it.
+  view = [NSView focusView] ? [NSView focusView] : view;

But (as best I can tell) this code does no harm.  So I'm willing to
live with it.  We should, however, keep an eye out for problems that
this change might cause.

The comment needs to change, though.  Did you see any specific drawing
bugs testing on OS X 10.4?  I didn't.  Or do you have some reason to
think that there will be problems?  In either case, please specify the
problems in the comment.
Created attachment 348848 [details] [diff] [review]
Rendering patch, v4plus (what I tested with)

Here's the patch I tested with, plus a tryserver build made using it.

On it I ran all the automated tests at
https://developer.mozilla.org/en/Mozilla_automated_testing, on both OS
X 10.5.5 and 10.4.11.

I didn't see any problems that I didn't also see without the patch.
Or at least I *basically* didn't.  I did see some wierdness doing the
reftests on OS X 10.4.11 (though not on OS X 10.5.5).  This may (of
course) have been caused by flakiness in the test harness, or in the
tests themselves.  But let's keep this in mind, in case we see
failures on the "official" testboxes.

https://build.mozilla.org/tryserver-builds/2008-11-18_13:54-smichaud@pobox.com-bugzilla450800-rendering-v4plus/smichaud@pobox.com-bugzilla450800-rendering-v4plus-firefox-try-mac.dmg
(In reply to comment #50)
> (From update of attachment 348430 [details] [diff] [review])
> +// This method is called when drawing search fields.
> +@implementation NSView (SearchfieldDrawingWorkaround)
> +- (NSText*)currentEditor { return nil; }
> +@end
> 
> In my tests I moved this currentEditor method to the ChildView class.
> I had no problems.

OK. Thanks for the research!

> The last two lines should really be:
> 
> +  float diffWidth = size.width ? rectWidth - size.width : 0.0f;
> +  float diffHeight = size.height ? rectHeight - size.height : 0.0f;

Oops.

> Did you see any specific drawing
> bugs testing on OS X 10.4?

Yes, they're in the second and third column of attachment 348325 [details] (of bug 465069). I'll post the results of my findings there tomorrow.
(In reply to comment #52)

>+ // If focusView is nil, use the frame's view as a fallback. If that's nil,
>+ // too, we'll get drawing bugs on 10.4 and we can't do anything about it.
>+ view = [NSView focusView] ? [NSView focusView] : view;
>
>> Did you see any specific drawing
>> bugs testing on OS X 10.4?
>
> Yes, they're in the second and third column of attachment 348325 [details] (of bug
> 465069). I'll post the results of my findings there tomorrow.

Are you saying that when you call [NSCell drawWithFrame:inView:[NSView
focusView]] and [NSView focusView] returns nil, these drawing bugs happen on
OS X 10.4?

Your test app at bug 465069 doesn't seem to demonstrate this -- I doubt that
[NSView focusView] ever returns nil while it's running.
(In reply to comment #53)
> Are you saying that when you call [NSCell drawWithFrame:inView:[NSView
> focusView]] and [NSView focusView] returns nil, these drawing bugs happen on
> OS X 10.4?

Yes. See bug 465069 comment 7.

> Your test app at bug 465069 doesn't seem to demonstrate this

Yeah, it's quite confusing...

> -- I doubt that
> [NSView focusView] ever returns nil while it's running.

It does. As soon as setCurrentContext has been called.
(In reply to comment #50)
> I suspect the following lines aren't necessary.

> +  // If focusView is nil, use the frame's view as a fallback. If that's nil,
> too,
> +  // we'll get drawing bugs on 10.4 and we can't do anything about it.
> +  view = [NSView focusView] ? [NSView focusView] : view;

You're right. "view" should always be something sensible, we don't need to use the focusView here. These lines really aren't necessary.
Created attachment 349037 [details] [diff] [review]
rendering part, v5

I updated the patch to apply on your new patch in bug 464589.
I also removed the lines mentioned in the previous comment.
Attachment #348430 - Attachment is obsolete: true
Attachment #348848 - Attachment is obsolete: true
Attachment #349037 - Flags: review?(smichaud)
Attachment #348430 - Flags: review?(smichaud)
Comment on attachment 349037 [details] [diff] [review]
rendering part, v5

Looks fine to me.
Attachment #349037 - Flags: review?(smichaud) → review+
Attachment #348782 - Flags: approval1.9.1?
Comment on attachment 349037 [details] [diff] [review]
rendering part, v5

Requesting approval1.9.1:
This patch greatly improves the appearance of search fields. Furthermore, it enhances the robustness of general native widget drawing; due to the investigation we did in bug 465069 we finally have a fairly good idea of what's going on :)
It also fixes bug 463805.
Attachment #349037 - Flags: approval1.9.1?
Attachment #348782 - Flags: approval1.9.1? → approval1.9.1+
Attachment #349037 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 349037 [details] [diff] [review]
rendering part, v5

a191=beltzner

Should we be doing something similar for the Location Bar and Search Bar?
Just a note on the current status: This bug can land after bug 464589 lands.

(In reply to comment #59)
> Should we be doing something similar for the Location Bar and Search Bar?

Maybe. The buttons in these fields might become a problem, though.
Depends on: 464589
Whiteboard: [waiting for bug 464589]
Can this land on trunk?
Whiteboard: [waiting for bug 464589]
Yep.
http://hg.mozilla.org/mozilla-central/rev/22614dd65a9f
http://hg.mozilla.org/mozilla-central/rev/02effdd7d4ad
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: wanted1.9.1?
Resolution: --- → FIXED
Whiteboard: [needs 191 landing when 464589 has landed on 191]
Target Milestone: mozilla1.9.1b2 → mozilla1.9.2a1
Until it hasn't been landed on the 1.9.1 branch we shouldn't remove the wanted1.9.1 flag. Just to make sure we don't forget anything.
Flags: wanted1.9.1?
Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20081206 Minefield/3.2a1pre.

The search widgets have a smooth focus tint now and it even works with graphite colors. Also they are located all on the left side now except the one in the search bar (which is expected).

Would be great to have this fix on the 1.9.1 branch soon.
Status: RESOLVED → VERIFIED
Landed on 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0d9683080541
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/866b85cc63ff
Flags: wanted1.9.1?
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing when 464589 has landed on 191]
Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090103 Shiretoko/3.1b3pre ID:20090103020545

Markus, would a reftest be possible to ensure the correct style? Or is this not possible or overhead?
Depends on: 448767
Flags: in-litmus?
Keywords: fixed1.9.1 → verified1.9.1
Target Milestone: mozilla1.9.2a1 → mozilla1.9.1b3

Updated

10 years ago
No longer depends on: 448767
Dao, why this bug shall not depend on bug 448767? Before the fix in this bug we haven't obeyed the system settings (blue vs. graphite) for the search widget.
(In reply to comment #67)
> Dao, why this bug shall not depend on bug 448767?

Because it's entirely unrelated. See bug 448767 comment 15.
Flags: in-litmus?
Target Milestone: mozilla1.9.1b3 → mozilla1.9.2a1
Depends on: 487518

Comment 69

9 years ago
(In reply to comment #37)
> Then the attribute should be called "cocoa-size" or so, or use a cocoa-small
> class name.

You definitely shouldn't be adding an attribute called 'cocoa-size' because that would make me upset that someone had added yet another inconsistently named xul attribute (new xul attributes are all lowercase and contain no punctuation). You also shouldn't be adding an attribute with 'cocoa' in it which you expect users outside of mac to have to use, but can never logically be used by other platforms.

If this attribute is meant to be a display hint only for theme purposes, which I think it is here, the correct way currently is to define a class for this purpose.

class="small", class="mini", and regular is just the default so no class (or class="smallsize" if you're concerned about conflicting with some other class name)
 
Please file a bug on fixing this.
Depends on: 663339
You need to log in before you can comment on or make changes to this bug.