We need to rewrite the native theme implementation for Cocoa widgets so that it can be used for chrome and content for all products including Firefox. We need to implement our theming using only nsITheme with no supporting CSS. This will allow us to respect web developer choices and will result in a more robust implementation (better looking with fewer glitches). There are a good number of nsITheme support bugs in gecko layout which we will need to identify and fix before this is possible. Another goal of this rewrite is to make the theme look more Mac-like than it does. Camino 1.x is a big step in the right direction but it is glitchy and not Mac-like enough.

This is v1.0. This is a large part of what we need to do, though there is plenty of tweaking left. What this patch does to our existig native theme impl: - Uses nsITheme for all theming with no supporting CSS. Among other things this means we do a much better job respecting site author's wishes. - Adds an API to nsITheme to tell layout when a widget takes care of drawing its own focus. This means we won't see gecko focus rings and native focus rings at the same time any more. - Adds a widget type, NS_THEME_DROPDOWN_BUTTON_POPUP, to fix a mistake we can't rectify correctly because of legacy concerns. By default, we draw a NS_THEME_DROPDOWN_BUTTON inside of popup widgets. This is the desired behavior on Windows and Linux and unthemed platforms, but in a native Mac OS X theme. We can't just not draw NS_THEME_DROPDOWN_BUTTON because that is used in other places by extension and XUL app developers. The solution is to introduce NS_THEME_DROPDOWN_BUTTON_POPUP and use that inside of NS_THEME_DROPDOWN_BUTTON. NS_THEME_DROPDOWN_BUTTON_POPUP is exactly the same as NS_THEME_DROPDOWN_BUTTON on Windows and GTK, but on Mac OS X the former just does not draw anything. - Widgets should do a much better job of drawing inside their frames now. This means no more clipped buttons and popup buttons. There still may be some clipped buttons and popup buttons until bug 369581 is fixed. This will only happen with really small widgets (< 20 pixels tall). - Buttons should be sized nicely now. Again, this may be a little glitchy for small widgets until bug 369581 is fixed. - Widget coloring should be greatly improved (especially selected items in select controls). - Lots of hacks removed, simplified code. - Focus rings on widgets should no longer leave traces behind when they disappear. I have seen it still happen sometimes, but I suspect that is a layout bug with invalidating widget overflow areas. I'll track that down later. - We're using HITheme correctly now in most cases, which fixes a few bugs. For one, we now draw text fields at the right size while they were off by a couple pixels before. Same goes for buttons. - Focus rings for all widgets with native focus rings draw completely and unclipped. This means no more oddly shaped selected buttons. What we still need to do: - Re-verify that all widgets draw completely inside their frames once bug 369581 is fixed. - Fix selected status in listboxes (selected items should go gray). - Track down and fix the invalidation bug that sometimes leaves focus ring pixels behind. This happens much less frequently now, but still on occasion. - make sure this patch build on Linux (volunteers?) - Listboxes have 1 pixel white border around their options. That is there because of a clipping bug which I'll have to track down and fix later. - Non-small checkboxes and radio buttons are no larger than their smaller versions. This is because some vertical alignment issues make large checkboxes and radio buttons look bad. This isn't really a problem for content HTML, in fact it is almost better, but in the future we probably want chrome to be able to use larger versions of those widgets. Not a big deal right now though. - Native focus rings don't work on text fields or multi select widgets. Again, a layout bug (or three). - might need tweaking on Mac OS X 10.3 and 10.5. If we can get this patch to a point where it compiles on all platforms and it passes reviews, I would like to land it and do followup fixes on account of the size. Once it lands I will file bugs on all known issues. This is already way better than what we have now. Asking dbaron for review first since I'd like to get the gecko/nsITheme API parts of this patch in shape first.

Intel-only build compiled fine with patch applied to fresh trunk.

> - make sure this patch build on Linux (volunteers?) Compiles on Linux for me, and doesn't crash on startup. What more could you want? The only glaring thing I've noticed (on Mac/Fx) in an hour or so is that the alltabs menu and context menus have arrowscrollboxes that they don't actually need, that scroll a pixel or two of whitespace. However, much as I hate to jinx this by mentioning it, in that hour I haven't once lost focus in textboxes and had to click the desktop and back to restore it, which is about 55 minutes longer than I've gone for months. Land it! Land it!

I ran this on Windows this morning and it all looked fine (only noticed one minor issue in combobox dropdown arrow alignment). No crashes.

I don't think you want to see this one... (a select drop down with background/colour/border specified, lifted from the style sheet of a vBulletin CMS control-panel). Screenshot included, and a screenshot of the latest Webkit build as bonus. ---- Minefield OS X 10.4.8 PowerPC. Except for the example attached, I saw only a few minor glitches, mostly what you've already mentioned. Probably material for follow-up bugs. No problems filling/submitting forms.

I made a quick run-through on 10.3.9 with all the testcases I gave josh the other day and saw lots of minor things. The biggest thing I noticed was that disabled buttons are now/still gfx. (That, and that web authors do evil, awful things to form widgets :P ;) ) Unfortunately I'm pretty busy atm and will be for the next few days, so I can't go split-out/annotate them, but I expect they mostly will be the sorts of things that fall in the "followup bug" category anyway (I hope!)

I tracked down another layout glitch that makes text in buttons look funny sometimes - see bug 369914.

How are you addressing the native popup widget problem?

Fwiw, patch compiles on 10.3.9 with the 3.0 sdk (ff-debug build) - case eColor_highlight: // CSS2 color case eColor_TextSelectBackground: - case eColor__moz_menuhover: res = GetMacBrushColor(kThemeBrushPrimaryHighlightColor, aColor, NS_RGB(0x00,0x00,0x00)); break; - case eColor_highlighttext: // CSS2 color + case eColor_highlight: // CSS2 color + case eColor__moz_menuhover: + res = GetMacAccentColor(eColorOffset_mac_accentregularshadow, aColor, NS_RGB(0x33,0x6F,0xCB)); + break; I guess this is material for a follow-up bug since it might require theme changes, but I think a more correct way would be to use kThemeBrushAlternatePrimaryHighlightColor for "highlight". That way, the color would be changeable in system prefs and we could also get rid of the -moz-mac-alternateprimaryhighlight stuff further down in the file.

Related: bug 262191 - Default font for form controls is too large bug 298706 - List controls (multiline <select>s) should be more customizable through style bug 175279 is probably obsolete by now ? ---- Some things I noticed: * text fields (input[type="text"]). When the page author specifies a border (border: [width] [style] [color]), the background becomes transparent - sample: the search box on http://elpais.com, top right of page, or http://news.com.com/ * possibly related to previous (?): the NetError page now lacks the white background (it works fine in a non-patched build) * input buttons and especially select dropdowns seem to have a bit too much padding on left and right. I had noticed that earlier on unpatched Camino trunk builds. Iirc, that started with the patches in bug 366722

Thanks for the feedback. I know what the problem with the backgrounds is, I will fix it in the next revision of the patch. I've also fixed some problems with text positioning and clipping in popup buttons. None of these changes so far affect the mozilla/layout portions of the patch, so I'd like to wait for review of the initial patch before posting another one.

Comment on attachment 254452 [details] [diff] [review] fix v1.0 >Index: gfx/public/nsITheme.h You need to create a new IID for nsITheme. >+ /** >+ * Does the nsITheme implementation draw its own focus ring for this widget?? Could you indent this line one character less and end with only a single question mark? >+ virtual PRBool ThemeDrawsFocusForWidget(nsPresContext* aPresContext, I don't like the name ThemeDrawsFocusForWidget very much, but it does seem to fit with the existing names on this interface better than anything else I can think of. Maybe ThemeDrawsWidgetFocus is a little better, but not much. >Index: gfx/public/nsThemeConstants.h >+// This is the button that draws inside popup select widgets >+#define NS_THEME_DROPDOWN_BUTTON_POPUP 105 >+ What you're calling a "popup select widget" is what the rest of the comments call a "combobox". So your comment basically says the same thing that the comment above NS_THEME_DROPDOWN_BUTTON says, but less clearly. What *else* is the NS_THEME_DROPDOWN_BUTTON constant used for? I think that's where the new name needs to come from. >Index: layout/forms/nsButtonFrameRenderer.cpp >+ nsPresContext* presContext = mFrame->GetPresContext(); >+ nsITheme *theme = presContext->GetTheme(); >+ if (!theme || !theme->ThemeDrawsFocusForWidget(presContext, mFrame, NS_THEME_BUTTON)) { You can't do this this way, since you're not calling ThemeSupportsWidget, and you shouldn't make any other nsITheme calls for a constant if ThemeSupportsWidget is false. The normal way to call ThemeSupportsWidget is via nsIFrame::IsThemed. And you should normally use the actual mAppearance rather than hard-coding NS_THEME_BUTTON. So this code should look more like: nsPresContext *presContext = mFrame->GetPresContext(); const nsStyleDisplay *disp = mFrame->GetStyleDisplay(); if (mFrame->IsThemed(disp) && presContext->GetTheme()->ThemeDrawsFocusForWidget(presContext, mFrame, disp->mAppearance) { ... and likewise in all the other places you use this pattern. I have to stop for now; I've only skimmed the remainder of the patch.

(In reply to comment #12) > I don't like the name ThemeDrawsFocusForWidget very much, but it does seem to > fit with the existing names on this interface better than anything else I can > think of. Maybe ThemeDrawsWidgetFocus is a little better, but not much. I'd say don't bother changing it unless we come up with something better.

I think I am actually going to get rid of NS_THEME_DROPDOWN_BUTTON_POPUP. The problem is that even if I don't draw the button inside the combobox, space is still reserved for it and thus it can cut off text in a combobox. Any solution to this problem needs to get rid of the dropdown button in the combobox. Either set "display:none" on it or remove it from the DOM. I tried to set display:none in the forms.css code (#ifdef'd for Mac OS X) inside the "select > input[type="button"]" block but that just hides the whole combobox and not just the button inside it. I'm not sure where we put the button inside the combobox in the first place, maybe we can just #ifdef it out there. Perhaps the right thing to do is not put a dropdown button inside of combobox widgets, and on Windows and Linux just draw it at the right spot inside the combobox when comboboxes are drawn. Putting an actual dropdown button inside the combobox in the first place seems like a move that wasn't well thought out.

Comment on attachment 254452 [details] [diff] [review] fix v1.0 Obsolete, this patch is incomplete and I've decided to break it up into a bunch of smaller bugs/patches.

This theme rewrite is essentially complete. Marking fixed.