112980 - Implement nsITheme API for code-level rendering of widgets

Status: VERIFIED FIXED

(Core Graveyard :: Skinability, defect)

Description

[David Hyatt]

Comment 1

[Hixie (not reading bugmail)]

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

Comment 2

[David Hyatt]

Attachment: Work so far. Not intended for review.

Comment 3

[David Hyatt]

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.

Comment 4

[David Hyatt]

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

Comment 5

[David Hyatt]

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

Comment 6

[Hixie (not reading bugmail)]

cool -- screenshots? :-)

Comment 7

[David Hyatt]

Attachment: Patch v2. Re-enables dynamic skin switching without doing a full frame reconstruction. Adds rules for tabs, scrollbars, toolbars, toolboxes, statusbars, and toolbar buttons.

Comment 8

[David Hyatt]

Attachment: Screenshot of Mozilla in the Cinnamon tint of the Coughdrop theme.

Comment 9

[Brendan Eich [:brendan]]

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=[e-mail] on the non-layout stuff.

/be

Comment 10

[Brendan Eich [:brendan]]

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

/be

Comment 11

[Hixie (not reading bugmail)]

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

Comment 12

[Håkan Waara]

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

Comment 13

[David Hyatt]

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 14

[Joe Hewitt (gone)]

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 15

[Joe Hewitt (gone)]

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

Comment 16

[Hixie (not reading bugmail)]

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

Comment 17

[Mike Pinkerton (not reading bugmail)]

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

Comment 18

[David Hyatt]

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

Comment 19

[David Hyatt]

Attachment: Cleaned up patch.

Comment 20

[Chris Waterson]

Comment on attachment 62005 [details] [diff] [review]
Cleaned up patch.

sr=waterson. great work!

Comment 21

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 62005 [details] [diff] [review]
Cleaned up patch.

r=[e-mail]

Comment 22

[David Hyatt]

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

Comment 23

[David Hyatt]

Fixed.

Comment 24

[Roland Mainz]

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

Comment 25

[Pratik]

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.

Comment 26

[Pratik]

Attachment: Screenshot of URL bar after changing themes

Comment 27

[Mike Pinkerton (not reading bugmail)]

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

Comment 28

[Pratik]

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!

Comment 29

[John Morrison]

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.

Comment 30

[Mike Kaply [:mkaply]]

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.