Some random ramblings about this: As I understand it, nsITheme needs to support: * Getting told by an nsIFrame to paint something of a particular type (as specified by the computed value of -moz-appearance of the frame's element), * Telling the nsIFrame that calls it that is has a certain padding, * Telling frames whether a particular coordinate is 'hot' or not. The padding it returns should either override the frame's padding or be added to the frame's padding. It either should or should not have an affect on the frame's computed value of 'padding'. (My point is we should define what will happen here, so that we know what is and what is not a bug.) IMHO, when we are using nsITheme to render a frame, we should ignore the frame's border property completely. (i.e. -moz-appearance makes border irrelevant.)

Hixie, yes, I ignore both border and background properties. CSS padding augments any padding also specified by the control, and is not ignored. This is needed to make text shift down and to the right on a button press (for example). Font and color are treated as having been specified at the same rule that defined an appearance, so they can be overridden by a more specific rule.

I now have XUL dialog buttons completely working. Radio groups and checkboxes largely work (only the hover state doesn't work because the radio and checkbox images don't ever go into :hover).

*** Bug 112981 has been marked as a duplicate of this bug. ***

cool -- screenshots? :-)

Comment on attachment 61676 [details] [diff] [review] Patch v2. Re-enables dynamic skin switching without doing a full frame reconstruction. Adds rules for tabs, scrollbars, toolbars, toolboxes, statusbars, and toolbar buttons. I'm not fit to r= the layout/* changes, but the rest look ok to me. - Note the magic #defines that must match values in some WinXP header file with BIG SCARY COMMENTS, ok? - Expand tabs in Makefile.in, if not in all makefiles: LIBRARY_NAME = gkbase_s REQUIRES = xpcom \ string \ + layout_xul \ dom \ content \ gfx \ Index: layout/base/src/makefile.win =================================================================== RCS file: /cvsroot/mozilla/layout/base/src/makefile.win,v retrieving revision 3.73 diff -u -r3.73 makefile.win --- layout/base/src/makefile.win 21 Nov 2001 09:47:03 -0000 3.73 +++ layout/base/src/makefile.win 14 Dec 2001 01:53:21 -0000 @@ -39,6 +39,7 @@ imglib2 \ gfx2 \ content \ + layout_xul \ gfx \ $(NULL) r=brendan@mozilla.org on the non-layout stuff. /be

Also, run the leak and bloat tests, and switch skins a lot, and report results. /be

hyatt: Looks great! I have issues with the isDisabled, isChecked and isSelected functions in nsNativeThemeWin.cpp. Using attributes directly like this IMHO is not going to work on the long term. I think what we should do is bite the bullet and implement the pseudo-class setting stuff that we've been talking about. Make the XBL for the widgets toggled the :checked, :indeterminate (tri-state), :disabled, and :enabled pseudo-classes, then use those in the theme API to determine status instead of hardcoding knowledge for every widget set we want to support. This would require us to add two functions to all elements, element.setPseudoClassState('pseudo', bState) bState = element.setPseudoClassState('pseudo') ...which know how to toggle the states of: 'enabled', 'disabled' (mutually exclusive) 'checked', 'indeterminate' (mutually exclusive) 'focus' (only one element can be set at a time) 'active' (only one element can be set at a time) 'hover' (only one element can be set at a time) So then the XBL for XUL checkboxes would, as well as toggling its attributes on a click, set the pseudo-class. This would make it easier to style the control in CSS and would make this API a lot more resilient when it comes to implementing other controls in XBL (e.g. a widget set that uses japanese attribute names). e.g. this.setPseudoClassState('checked', nowChecked); (Note that selected items are classified as checked for now. Feel free to add :unchecked, it's only not in the selectors spec because of an oversight.)

You added an old, non tri-license to the new file gfx/public/nsITheme.h... also, it's MPL.

Ian, we should break out a separate bug to implement those new pseudoclasses. In the end, I still have to examine attributes, so having to check for disabled/checked/selected as attributes isn't a big deal in the short term, but I do agree that those pseudoclasses should be implemented (along with :open) and that the native theme renderer should be patched when they are ready. The reason I don't think it's a big deal is that I will have to check for many more attributes that CSS is never going to specify before I'm done anyway, e.g., the value of a progressmeter, orientation of widgets, types of scrollbar buttons (decrement vs. increment), etc. etc. BTW, I assume you meant this: bState = element.setPseudoClassState('pseudo') to be this: bState = element.getPseudoClassState('pseudo')

Comment on attachment 61682 [details] Screenshot of Mozilla in the Cinnamon tint of the Coughdrop theme. This screenshot can't land, it is pink. Please change it to blue before you land it.

Comment on attachment 61676 [details] [diff] [review] Patch v2. Re-enables dynamic skin switching without doing a full frame reconstruction. Adds rules for tabs, scrollbars, toolbars, toolboxes, statusbars, and toolbar buttons. sr=hewitt

hyatt: sure thing, I've filed bug 115462. (Note that I think that the scrollbar button thing should be handled by different -moz-appearance keywords rather than one keyed off pseudos or attributes. I agree with your other points though.)

why isn't nsITheme in idl and the constants defined there?

Because it's hopelessly unscriptable (given its dependence on all the non-IDL gfx and layout interfaces).

Comment on attachment 62005 [details] [diff] [review] Cleaned up patch. sr=waterson. great work!

Comment on attachment 62005 [details] [diff] [review] Cleaned up patch. r=ben@netscape.com

Fixed. I'll cover individual conversion of widgets to native renderers with separate bugs.

Fixed.

Is there any bug to implement the same thing for the GTK+/Xlib ports ?

Its great to get this bug fixed. Howevere there are some issues while changing themes and I'm not sure if they are known and filed as sepearte bugs. 1. The icons on personal tooolbar don't change. The old theme icons persist. Try switching between classic and modern. 2. On linux, after I change theme, the URL bar's dropdown arrow changes position. I'm going to attach a screenshot for this next.

psolanki, this bug has _nothing_ to do with switching themes.

Whoops! really sorry about that then. I guess Asa's comments on mozillazine.org threw me off. "as well as the return of dynamic theme switching with checkins for bug 112980." Sorry!

Well, I suppose not "nothing" since a better way to do skin switching without reconstructing all the frames kinda fell out of this work. But even if this bug were only about skin switching, defects in implementation would be more effectively handled as separate, specific bug reports.

OK, somewhere it might actually be useful to know what the heck nsITheme is and does so that other platforms can implement it? usually that would be in the header, but this header has squat.