nsITheme support for Windows 9x/NT/2000



15 years ago
9 years ago


(Reporter: Tim Hill, Assigned: Tim Hill)


Windows 95
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)



(2 attachments, 7 obsolete attachments)



15 years ago
Not everyone is "XPerienced" yet, so nsITheme native drawing for XUL and HTML 
form controls should be supported under Windows 9x/NT/2000.  Windows has an API 
to draw most controls.  This would fix accessibility bugs (such as using the 
correct system scrollbar size/colors) and make Mozilla look better under 
different color schemes.

Comment 1

15 years ago
assigning to self

Comment 2

15 years ago
Created attachment 102123 [details] [diff] [review]
nsNativeThemeWin changes

This modifies nsNativeThemeWin to draw controls under non-XP operating systems
using DrawFrameControl, DrawEdge, and GDI.  Most controls are supported.

Comment 3

15 years ago
Created attachment 102125 [details] [diff] [review]
CSS changes

This makes a few XUL CSS changes to allow scrollbars and dropdown buttons to be
as small as Windows allows, and to remove the moz-appearance border from the
autocomplete dropdown list.

Comment 4

15 years ago
Requesting review

Comment 5

15 years ago
This is fantastic, Tim!  Comments:

Your CSS changes are likely incompatible with Windows XP, since you are changing the 
min size of controls (and this overrides the theme's specified size).

I would first verify that I'm right about this by running with your changes on WinXP.   If I am 
right, then we can correct the problem by removing the min-width/height rules from CSS 
completely and just hack the theme code to hand back the right values for WinXP vs. 2k.

Note that with your changes, we can actually eliminate virtually all of the CSS rules for 
XUL widgets in the classic skin, which should be a nice performance boost.

Also, be sure to test this patch in WinXP with the classic look, and make sure it behaves 
correctly.  This is a different code path, since mThemeDLL is there, but calls to getTheme 
will start returning NULL.  You should watch for this and treat it just like the Win2k/NT 
case (in which mThemeDLL is actually null).

Also study each control's state and double-check that you have all the states covered 
(hover/focus etc.).

This is fantastic.  I will give quick/responsive feedback to any new patches you submit for 
this bug.

Comment 6

15 years ago
Created attachment 102654 [details] [diff] [review]
nsNativeThemeWin patch 2

The patch now uses the old theme support when mThemeDLL is null or when the
theme handle is null, adds theme support for tabs, and adds focus rects for
HTML radio buttons/checkboxes.	I've tested on Windows 95, NT 4.0, and Windows
2000 and it works, but I don't have XP available to test.  I'm pretty sure all
the necessary states are covered.
Attachment #102123 - Attachment is obsolete: true

Comment 7

15 years ago
Created attachment 102655 [details] [diff] [review]
CSS patch 2

The min-width/min-height rules might as well go if the classic skin rules
aren't going to be kept around at all (and with full Windows/GTK/Aqua support I
guess there's no real reason for them). Let's make eliminating the rest of the
classic rules a separate bug though. :) It'd be nice to get this in 1.2...
Attachment #102125 - Attachment is obsolete: true

Comment 8

15 years ago
Not all platforms implement nsITheme, afaik BeOS and QNX don't. However I
believe that that most platforms use the windows css files for the classic
theme. Am I correct in saying that your removal of css rules will make classic
look bad on these ports? If so it looks like we'll need to create css files for
paltforms which don't yet implement nsITheme.

Comment 9

15 years ago
I am going to try this out on WinXP and let you know how it looks in Classic 
mode (as well as making sure everything's ok in Luna).  Great job!


Comment 10

15 years ago
Created attachment 103129 [details] [diff] [review]
Natve Theme patch 3

I looked through a bunch of Mozilla windows dialogs for any visual oddities and
found a couple.  This patch just adds support for drawing bottom-tabs, uses
-moz-appearance: none for a borderless textbox inside a dropdown, and fixes a
minor bug in drawing focus rectangles.
Attachment #102654 - Attachment is obsolete: true
Attachment #102655 - Attachment is obsolete: true

Comment 11

15 years ago
tim, with your latest patch I no longer see the groove around the 
menubar/toolbar in Phoenix.
This is extremely cool, but the "Old..." method names are a bit grungy. It would
be nice if they were replaced with something meaningful.

Comment 13

15 years ago
I agree.  Let's use the term "Classic" rather than "Old", since this matches
Windows XP's terminology.

Comment 14

15 years ago
Created attachment 104308 [details] [diff] [review]
Native theme patch 4

ok, renamed "old" to "classic", fixed toolbox.	For now I made the scrollbar
arrows non-overridable within nsITheme rather than modifying CSS, so that it
will still work when theming is off.  Really though the classic theme should
probably be forked for OS/2 and BeOS.  Have you tried this under XP yet Dave?


15 years ago
Attachment #103129 - Attachment is obsolete: true


15 years ago
Blocks: 175038


15 years ago
Attachment #104308 - Flags: review?(hyatt)

Comment 15

15 years ago

Comment 16

15 years ago
Created attachment 107899 [details] [diff] [review]
Native theme patch 5

I borrowed an XP machine to test this, and didn't see any problems on Classic,
or XP themes.  I also fixed some minor issues under XP themes -- bug 61188 and
bug 178792 and focus rectangles for checkboxes and radios.
Attachment #104308 - Attachment is obsolete: true


15 years ago
Attachment #107899 - Flags: review?(hyatt)

Comment 17

15 years ago
Just wondering.. will this also fix bug 130534 that only affects xp?


15 years ago
Attachment #104308 - Flags: review?(hyatt)

Comment 18

15 years ago
I think this is ready.


15 years ago
Attachment #107899 - Flags: review?(hyatt) → review+


15 years ago
Attachment #107899 - Flags: superreview?


15 years ago
Attachment #107899 - Flags: superreview? → superreview?(roc+moz)
+  if (!theme)
+    return ClassicThemeSupportsWidget(aPresContext, aFrame, aWidgetType);
+  return IsAlreadyStyled(aFrame, aWidgetType);

I think we should check IsAlreadyStyled for all widget types after calling
ClassicThemeSupportsWidget, and removing the calls to IsAlreadyStyled from
ClassicThemeSupportsWidget. Backgrounds and borders can be applied to lots of
form controls, and if we're going to hardcode assumptions about the kinds of
widgets which can be styled, we should do it in IsAlreadyStyled which can at
least be shared across platforms. [This is for another bug, but we really should
move IsAlreadyStyled somewhere shared so that all platforms can use it and
display native controls in HTML content.]

+      SystemParametersInfo(SPI_GETNONCLIENTMETRICS, sizeof(nc), &nc, 0);  

Mind checking to see if this succeeded and doing something sensible if it didn't?

+        if (contentState & NS_EVENT_STATE_ACTIVE && contentState &

[Optional] Here and below, I'd write "if ((contentState & (NS_EVENT_STATE_ACTIVE
because it's a wee bit more efficient. But you don't have to :-).

+        if (aWidgetType == NS_THEME_CHECKBOX) {
+          if (IsChecked(aFrame))
+            aState += DFCS_CHECKED;
+        }
+        else
+          if (CheckBooleanAttr(aFrame, mSelectedAtom))

[Optional] You can write this a little simpler as "if ((aWidgetType ==
NS_THEME_CHECKBOX) ? IsChecked(aFrame) : CheckBooleanAttr(aFrame,
mSelectedAtom))", but again, you don't have to.

+      return NS_OK;

[Optional] It would be a bit easier to see that this code is correct if you set
some default values for the outputs of this function in these cases.

+        aState = aState | DFCS_PUSHED | DFCS_FLAT;

Here and below you can write "aState |= DFCS_PUSHED | DFCS_FLAT;". Actually it's
probably a little safer to use |= instead of += in all your code here.

+  #define SET_RECT(R,l,t,r,b) R.left=l; R.top=t; R.right=r; R.bottom=b;

Why didn't you just use ::SetRect? If you want the parameters in this order then
would you mind just defining this as an inline function? Macros suck.
static inline void SetRectXY(RECT& aRect, int aL, int aT, int aR, int aB) {
  aRect.left = aL; aRect.top = aT; aRect.right = aR, aRect.bottom = aB;

+  HDC hdc = ((nsRenderingContextWin*)aContext)->mDC;

Please use NS_STATIC_CAST.

+    case NS_THEME_BUTTON: {
+      if (focused) {
+        // draw dark button focus border first
+        HBRUSH brush;
+        brush = ::GetSysColorBrush(COLOR_3DDKSHADOW);
+        ::FrameRect(hdc, &widgetRect, brush);

Put this inside "if (brush)".

+        widgetRect.left++; widgetRect.top++;
+        widgetRect.right--; widgetRect.bottom--;
+      }
+    }
+    // Draw controls supported by DrawFrameControl

Please add a comment noting that you're falling through to the next case.

+      ::FrameRect(hdc, &widgetRect, brush);

"if (brush)"

+        patBmp = ::CreateBitmap(8, 8, 1, 1, patBits);
+        brush = (HBRUSH) ::CreatePatternBrush(patBmp);

These should check for failure too.

Comment 20

15 years ago
Created attachment 108128 [details] [diff] [review]
Native theme patch 6

Patch addressing roc's comments.  It seems simple enough to turn off theming
for other styled widgets, but Hyatt's existing code was only for buttons and
fields, so I left that alone..
Attachment #107899 - Attachment is obsolete: true


15 years ago
Attachment #108128 - Flags: superreview?(roc+moz)
Attachment #108128 - Flags: review?(hyatt)
Comment on attachment 108128 [details] [diff] [review]
Native theme patch 6

+	 brush = (HBRUSH) ::CreatePatternBrush(patBmp);

You should check patBmp != NULL before using it.


Also, I'm taking th liberty of carrying forward hyatt's r=

Go ahead and find someone to check it in for you...
Attachment #108128 - Flags: superreview?(roc+moz)
Attachment #108128 - Flags: superreview+
Attachment #108128 - Flags: review?(hyatt)
Attachment #108128 - Flags: review+

Comment 22

15 years ago
Created attachment 108143 [details] [diff] [review]
Final patch

Comment 23

15 years ago
Fix checked in.
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 24

15 years ago
Maybe wrong place to ask, but programs using native os scrollbars in Windows 
make the arrows smaller when window is too small. Try this with any program 
using scrollbars and you will see that the up and down arrow are minimized in 
This does not happend in Mozilla though, they dissappear when the window has a 
certain height. Is this (just guessing) due to that this was not supported 
before and the arrows were hidden? Is this behavior possible in Mozilla?


15 years ago
Blocks: 179507


15 years ago
Attachment #107899 - Flags: superreview?(roc+moz)
(In reply to comment #24)
I think that's a good question, José, and a valid bug. I've filed bug 292284 for

Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.