390734 - groupboxes need native theming on Windows

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

On vista, groupboxes have four issues:

1) The color is wrong: should be 213,223,229 and white with the default theme
2) Needs a white shadow (making it 2 pixels wide)
3) The label has too much spacing on either side
4) The corners are too curved

Comment 1

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Groupboxes on Vista

Comment 2

[Alex Faaborg [:faaborg] (Firefox UX)]

Will fixing this bug may require a separate theme for Vista? (which is unfortunately something we aren't committed to doing for Firefox 3).

Comment 3

[Alex Faaborg [:faaborg] (Firefox UX)]

Alternatively, we could drop the use of groupboxes entirely, in favor of horizontal bars and icons.

Comment 4

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Example of using horizontal bars and icons in a Vista preferences dialog

This is an example of using horizontal bars and icons in a Vista preferences dialog, from Windows Calendar.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

FTR, I don't think this blocks, but would be happy to go with the solution in comment #3 for Vista.

cc'ing vlad in case this is more native theming stuff.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Well, in theory it is native theme stuff, but we don't use the native theme bits to do any of this.  Sort of.  It's over here: http://mxr.mozilla.org/firefox/source/toolkit/themes/winstripe/global/groupbox.css#46

The radius is easy to fix (the -moz-border-radius), as is the padding around the caption text.  (Note that the caption padding shows up because it has a padding/margin, and has an explicit background color which should match the actual dialog background or it'll be an off-color block.)

The color of the border is really that "border: 1px solid ThreeDShadow"; ThreeDShadow comes from windows, via GetSysColor(COLOR_3DSHADOW).  You can experiment with ThreeDFace, ThreeDHighlight, and ThreeDLightShadow to see if those are any closer to the real color.  Also, the vista border seems to be closer to the 'groove' border style (dark-light offset); so try "border: 2px solid ThreeDShadow" or something.  There are other colors to see if you can pick out which one that is; nsILookAndFeel should have the full set.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: example

So here's a style that seems to work.  Note I had to use explicit -moz-border-*-colors to get the colors right, and "inactiveCaption" seemed to be the right shade (the "activeCaption" was too saturated).  If you try something like "2px groove inactiveCaption", you'll actually get a color that's darker and lighter than the inactiveCaption color due to what groove does... so that's no good.  I also dropped the radius down to 3px instead of 5px.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 306450 [details]
example

... though now it looks like 3px is too small for the radius, makes two of the corners look too sharp, so maybe 4px.  But you get the idea. :)

Comment 9

[Dão Gottwald [:dao]]

ThreeDHighlight should work instead of white, right?

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

See also bug 236107. Vlad, do we have a bug about the missing native theming support for groupboxes? I'm not able to find one.

Comment 11

[Ryan A. C.]

The reason the corners look so sharp is because of the way the border works. On the top left you're seeing the rounding of the darker color, so it looks right. On the top right and bottom left you're seeing rounding of a mixture of dark/light which for some reason makes it almost perfectly square, and on the bottom right it looks like 2px rounding because the lighter color is rounded while the darker one has less to round.
It looks better with 4px round, but still a little strange looking on the top right and bottom left.

Comment 12

[Stefan [:stefanh]]

fwiw, I added some native groupbox styling for mac in bug 421814. So, NS_THEME_GROUPBOX is now eagerly waiting for to be used in widget/src/windows/nsNativeThemeWin.cpp ;-)

Comment 13

[Alex Faaborg [:faaborg] (Firefox UX)]

Rob: is there a dupe of this bug that you've been working on?

Comment 14

[Robert Strong (they/them - no direct email)]

Bug 236107

Comment 16

[Robert Strong (they/them - no direct email)]

Attachment: screenshot

Alex, I just fixed the groupbox caption background in that other bug so I decided to take a look at fixing the border and space around the caption as well. Let me know what you think.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Rob, do Windows 7 and Vista use different theming backends? I have tested your patch from bug 236107 on Windows 7 and it looks great even on tab controls. I don't have a Vista right now.

(In reply to comment #16)
> Created an attachment (id=364768) [details]
> screenshot

Wow, that's great. A native border looks so much better.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So shall we use this bug for the border in general? If yes, could you please update the summary?

Comment 19

[Robert Strong (they/them - no direct email)]

I believe this covers everything in comment #0 except the caption background which I fixed in bug 236107 so I don't see any value in changing the summary... I'll move the patch from bug 236107 over here at some point so that is clearer.

Comment 20

[Robert Strong (they/them - no direct email)]

(In reply to comment #17)
> Rob, do Windows 7 and Vista use different theming backends?
I'm very sure the patch I'm working on will work properly on Windows 7 as well as Win2K and above except for the XP Luna caption background issue noted in bug 236107.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 364768 [details]
screenshot

You probably wanted to ask for ui-review.

Comment 22

[Robert Strong (they/them - no direct email)]

Thanks... I wasn't wearing my damn glasses. ;)

Comment 23

[Robert Strong (they/them - no direct email)]

fyi: filed Bug 480883 for issues with widgets not providing consistent start margin / padding values which prevent children of groupboxes aligning properly on Vista.

Comment 24

[Robert Strong (they/them - no direct email)]

Attachment: screenshots - comparisons between new and old

Comment 25

[Robert Strong (they/them - no direct email)]

Needs more padding for the bottom of the groupbox

Comment 26

[Robert Strong (they/them - no direct email)]

note: the Sanitize dialog uses 14em to create psuedo columns inside its groupbox along with a width of 30em... the 14em is way too large for this considering the window is only 2em larger and there is also a groupbox as the container for the columns.

Comment 27

[Robert Strong (they/them - no direct email)]

Filed bug 480898 for the issue with the sanitize dialogs

Comment 28

[Robert Strong (they/them - no direct email)]

Attachment: patch in progress

This should be pretty much it but I want to test on WinXP and Win2K if possible. Also, WinCE has issues with groupboxes so this might need to be tweaked.

Comment 29

[Dão Gottwald [:dao]]

Comment on attachment 364887 [details] [diff] [review]
patch in progress

>--- a/toolkit/themes/winstripe/global/groupbox.css
>+++ b/toolkit/themes/winstripe/global/groupbox.css

> groupbox {
>-  margin: 5px;
>-  border: 1px solid ThreeDShadow;
>-  -moz-border-radius: 5px;
>-  padding: 5px;
>-}
>-
>-groupbox:-moz-system-metric(windows-classic) {
>-  border-width: 2px;
>-  -moz-border-top-colors: ThreeDShadow ThreeDHighlight;
>-  -moz-border-right-colors: ThreeDHighlight ThreeDShadow;
>-  -moz-border-bottom-colors: ThreeDHighlight ThreeDShadow;
>-  -moz-border-left-colors: ThreeDShadow ThreeDHighlight;
>-  -moz-border-radius: 0px;
>+  margin: 3px;
>+  padding: 3px 3px 6px 3px;
>+  -moz-appearance: groupbox;
> }

The should still be a CSS border in case native theming is disabled.

Comment 31

[Robert Kaiser]

IMHO, it's just wrong to not have -moz-appearance/nsITheme hooked up for groupboxes. And with the current resolving of bugs around those issues, we now have ended up with no bug handling that case of doing the right thing :(

Comment 32

[Robert Strong (they/them - no direct email)]

This adds -moz-appearance: groupbox

Comment 33

[Robert Strong (they/them - no direct email)]

>diff --git a/widget/src/windows/nsNativeThemeWin.cpp b/widget/src/windows/nsNativeThemeWin.cpp
>--- a/widget/src/windows/nsNativeThemeWin.cpp
>+++ b/widget/src/windows/nsNativeThemeWin.cpp
>...
>@@ -2533,16 +2556,20 @@ RENDER_AGAIN:
>     }
>     // Draw ToolTip background
>     case NS_THEME_TOOLTIP:
>       ::FrameRect(hdc, &widgetRect, ::GetSysColorBrush(COLOR_WINDOWFRAME));
>       InflateRect(&widgetRect, -1, -1);
>       ::FillRect(hdc, &widgetRect, ::GetSysColorBrush(COLOR_INFOBK));
> 
>       break;
>+    case NS_THEME_GROUPBOX:
>+      ::DrawEdge(hdc, &widgetRect, EDGE_ETCHED, BF_RECT | BF_ADJUST);
>+      ::FillRect(hdc, &widgetRect, (HBRUSH) (COLOR_BTNFACE+1));
>+      break;
Dao, doesn't this handle that case?

Comment 34

[Dão Gottwald [:dao]]

Winstripe is the basis for different OSes, eg. linux and os/2, so the ultimate fallback shouldn't depend on widget/src/windows.

Comment 35

[Robert Strong (they/them - no direct email)]

Just for my own clarity, the linux case is handled already... true?
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/gnomestripe/global/groupbox.css

leaving os/2 and potentially others that need to be handled.

Comment 36

[Robert Kaiser]

The Linux case is lame hardcoding of a style that happens to match the current GNOME HIG though it's one of the major annoyances for users of other desktop environments. We currently happen to support any integration in GNOME only, but it probably would be better to move this hardcoding into native theme stuff as well so it's easier to correct when we might start supporting anything other than GNOME (or a miracle happens and they create a HIG that makes more sense).

Comment 37

[Robert Strong (they/them - no direct email)]

Thanks kairo, that's what I thought was the case. I wanted to verify that linux wasn't affected by this patch since it was specifically called out in comment #34. btw: I didn't see a widget: gtk bug for groupbox. :(

Comment 38

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 364768 [details]
screenshot

Looks great

Comment 39

[Robert Strong (they/them - no direct email)]

Attachment: WinXP screenshots

The issue with the caption background and the caption text color are still present but are no worse than they are without this patch. The space between the border and the caption text appears a pixel to close but isn't IMO as bad as it was before. Overall I think this is a big improvement on XP and all around for the Classic as well.

Comment 40

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Comment 41

[Robert Strong (they/them - no direct email)]

Attachment: sample screenshots without -moz-appearance

I went with the styling on the left since it IMO most closely matches Windows classic on XP and Vista

Comment 42

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #41)
> I went with the styling on the left since it IMO most closely matches Windows
> classic on XP and Vista

There are two things which are visible:

1. Why there is only a rounded corner for the top-left corner?
2. You should push up the caption by I think 2px. It's positioned a bit too low.

Otherwise it looks sweet.

Comment 43

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 365125 [details] [diff] [review]
patch rev1

looks fine to me!

Comment 44

[Dão Gottwald [:dao]]

(In reply to comment #37)
> Thanks kairo, that's what I thought was the case. I wanted to verify that linux
> wasn't affected by this patch since it was specifically called out in comment 34.

It was more of a general statement. Gnomestripe happens to override groupbox.css, but it doesn't have to.

Comment 45

[Robert Strong (they/them - no direct email)]

It doesn't have to but it should if it wants anything whatsoever to be gnome/linux specific... in my opinion it should otherwise it should expect to have a ui that is windows specific... otherwise either the windows theme ends up being close to windows but not quite as close to windows as it could be and the linux theme ends up being close to linux but not quite as close to linux as it could be. As it applies to this specific case it isn't applicable since linux provide a linux theme in gnomestripe so the winstripe theme doesn't have to try to be a 'happy' medium between the linux and windows which would prevent us from doing the best we can per platform.

Comment 46

[Robert Strong (they/them - no direct email)]

regarding winstripe being used as the defacto default theme I filed bug 481174

Comment 47

[Dão Gottwald [:dao]]

Right, I agree that the fallback can't satisfy all platforms. It should be styled so that it's appropriate for Windows.

Comment 48

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/1776ced33da9

I'll file a followup for the XP Luna groupbox issue that is still present if I can't find one already files.

Comment 49

[Robert Strong (they/them - no direct email)]

note: Vlad told me this didn't need an sr+ but if it did he would sr it so I landed with a verbal sr=vlad.

Comment 50

[Bryan]

It would be wonderful if all of the buttons in the various tab/panels could be cleaned up and sized properly (all one size to fit the largest) and aligned, instead of the various size buttons we have that are rarely aligned.

The IE 7/8 General and Content tabs in Internet Properties are great examples of that.  I'll file a bug if I can't find one.

~B

Comment 51

[Robert Strong (they/them - no direct email)]

(In reply to comment #50)
> It would be wonderful if all of the buttons in the various tab/panels could be
> cleaned up and sized properly (all one size to fit the largest) and aligned,
> instead of the various size buttons we have that are rarely aligned.
That would most likely be a Firefox -> Preferences bug since it would likely need to be fixed in the way the xul is layed out and styled in the pref panes.

Comment 52

[Robert Strong (they/them - no direct email)]

(In reply to comment #48)
> I'll file a followup for the XP Luna groupbox issue that is still present if I
> can't find one already files.
Filed Bug 482912 for this

Comment 53

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090312 Minefield/3.2a1pre

I'll file a bug for the remaining vertical alignment issue of the groupbox caption.

Is this too late in the cycle for 1.9.1 or can we try to get it into the branch?

Comment 54

[Robert Strong (they/them - no direct email)]

(In reply to comment #53)
> Is this too late in the cycle for 1.9.1 or can we try to get it into the
> branch?
It will likely make it for 1.9.1

A couple of notes on this bug's dependencies

Bug 236107 was originally for Linux and has morphed quite a bit. It has a couple of patches that landed to change the background color for the groupbox captions on Windows.

Bug 483187 is for the caption alignment. The original styling had a lot of padding and margin which made the alignment of the caption better by around two pixels but screwed up the spacing of other elements by 3 or 4 pixels.

Bug 425582 is a meta bug for Firefox 3.0 RC1.

Bug 482972 is unrelated to this bug and is for Linux

Comment 55

[Robert Strong (they/them - no direct email)]

If this bug's patch is landed for 1.9.1 we will probably want to take the patches in bug 483187, bug 483327, and bug 480898.

Comment 56

[Robert Strong (they/them - no direct email)]

Comment on attachment 365125 [details] [diff] [review]
patch rev1

Drivers, I'd like to get this for 1.9.1 with at minimum the patch from bug 483187 at the same time. Ideally the patches from bug 483327 and bug 480898 as well but those are still waiting on review.

Comment 57

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 365125 [details] [diff] [review]
patch rev1

a191=beltzner

Comment 58

[Robert Strong (they/them - no direct email)]

We'll also want bug 484013 for 1.9.1

Comment 59

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-1.9.1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8c648639cc89

cc'ing people from comm-central apps since this might affect them as well. In particular see bug 483327, bug 480898, bug 483187, and bug 484013 as how this affected Firefox.

Comment 60

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090318
Minefield/3.6a1pre ID:20090318052249

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b4pre) Gecko/20090322
Shiretoko/3.5b4pre ID:20090322140038

Comment 61

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug's priority relative to the set of other polish bugs is:
P2 - Polish issue that is in a secondary interface, occasionally encountered, and is easily identifiable.

Only appears in secondary UI, but a pretty identifiable problem when it is encountered.