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.

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.

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.

> 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.

This patch applies on top of bug 394892.

I had to shuffle some things to avoid painting errors on 10.4.

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.

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.

(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.

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.

as requested by Dão in comment 15

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.

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?

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.

setting TM to beta 2. although polish, we just need this reviewed and hopefully landed soon.

Comment on attachment 347826 [details] [diff] [review] rendering part, v3.1 I can do better. A new patch will follow soon.

(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

Crashreporter is using the nsColor disabledControlTextColor.

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.

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.

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.

fwiw, the textbox has a size attribute: https://developer.mozilla.org/en/XUL/textbox#a-size

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

(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.

> 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

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.

The searchfields in the history / bookmarks sidebar also need cocoa-size="small".

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.

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.

I updated the patch to apply on your new patch in bug 464589. I also removed the lines mentioned in the previous comment.

Comment on attachment 349037 [details] [diff] [review] rendering part, v5 Looks fine to me.

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.

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.

Can this land on trunk?

Yep. http://hg.mozilla.org/mozilla-central/rev/22614dd65a9f http://hg.mozilla.org/mozilla-central/rev/02effdd7d4ad

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.

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.

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

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?

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.

(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.