172751 - nsITheme support for Windows 9x/NT/2000

Status: RESOLVED FIXED

(SeaMonkey :: Themes, defect)

Description

[Tim Hill]

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

[Tim Hill]

assigning to self

Comment 2

[Tim Hill]

Attachment: nsNativeThemeWin changes

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

Comment 3

[Tim Hill]

Attachment: 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

[Tim Hill]

Requesting review

Comment 5

[David Hyatt]

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

[Tim Hill]

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

Comment 7

[Tim Hill]

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

Comment 8

[timeless]

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

[David Hyatt]

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

[Tim Hill]

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

Comment 11

[Stephen Walker]

tim, with your latest patch I no longer see the groove around the 
menubar/toolbar in Phoenix.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[David Hyatt]

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

Comment 14

[Tim Hill]

Attachment: 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?

Comment 15

[Stephen Walker]

hyatt?

Comment 16

[Tim Hill]

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

Comment 17

[José Jeria]

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

Comment 18

[David Hyatt]

I think this is ready.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  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 &
NS_EVENT_STATE_HOVER) {

[Optional] Here and below, I'd write "if ((contentState & (NS_EVENT_STATE_ACTIVE
| NS_EVENT_STATE_HOVER)) == (NS_EVENT_STATE_ACTIVE | NS_EVENT_STATE_HOVER))"
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.

+    case NS_THEME_TAB_PANEL:
+    case NS_THEME_TAB_PANELS:
+      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
+    case NS_THEME_CHECKBOX:

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

[Tim Hill]

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

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 108128 [details] [diff] [review]
Native theme patch 6

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

You should check patBmp != NULL before using it.

sr=roc+moz

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

Go ahead and find someone to check it in for you...

Comment 22

[Tim Hill]

Attachment: Final patch

Comment 23

[Tim Hill]

Fix checked in.

Comment 24

[José Jeria]

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

Comment 25

[Martijn Wargers (dead)]

(In reply to comment #24)
I think that's a good question, José, and a valid bug. I've filed bug 292284 for
that.