groupboxes need native theming on Windows

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
Widget: Win32
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: faaborg, Assigned: rstrong)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {polish, verified1.9.1})

Trunk
mozilla1.9.2a1
x86
Windows Vista
polish, verified1.9.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [polish-hard][polish-visual][polish-p2])

Attachments

(8 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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
(Reporter)

Comment 1

10 years ago
Created attachment 275051 [details]
Groupboxes on Vista
(Reporter)

Comment 2

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

Comment 3

10 years ago
Alternatively, we could drop the use of groupboxes entirely, in favor of horizontal bars and icons.
(Reporter)

Comment 4

10 years ago
Created attachment 275060 [details]
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.
Blocks: 420232
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.
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.
Created attachment 306450 [details]
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 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

10 years ago
ThreeDHighlight should work instead of white, right?
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.
Version: unspecified → Trunk
(Reporter)

Updated

10 years ago
Blocks: 425582
(Reporter)

Updated

10 years ago
Blocks: 405605

Comment 11

10 years ago
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

10 years ago
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 ;-)

Updated

10 years ago
Depends on: 433566
(Reporter)

Updated

9 years ago
Whiteboard: [polish-hard][polish-visual]
Rob: is there a dupe of this bug that you've been working on?
Bug 236107
(Reporter)

Updated

9 years ago
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 236107
Status: RESOLVED → VERIFIED
Status: VERIFIED → REOPENED
Resolution: DUPLICATE → ---
Assignee: nobody → robert.bugzilla
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Keywords: fixed1.9.1
Resolution: FIXED → ---
Assignee: robert.bugzilla → nobody
Keywords: fixed1.9.1
Created attachment 364768 [details]
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.
Attachment #364768 - Flags: review?(faaborg)
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.
So shall we use this bug for the border in general? If yes, could you please update the summary?
Attachment #364768 - Flags: review?(faaborg) → superreview?(faaborg)
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.
(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 on attachment 364768 [details]
screenshot

You probably wanted to ask for ui-review.
Attachment #364768 - Flags: superreview?(faaborg) → ui-review?(faaborg)
Thanks... I wasn't wearing my damn glasses. ;)
fyi: filed Bug 480883 for issues with widgets not providing consistent start margin / padding values which prevent children of groupboxes aligning properly on Vista.
Created attachment 364839 [details]
screenshots - comparisons between new and old
Needs more padding for the bottom of the groupbox
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.
Filed bug 480898 for the issue with the sanitize dialogs
Created attachment 364887 [details] [diff] [review]
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.
Assignee: nobody → robert.bugzilla
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.

Updated

9 years ago
No longer blocks: 405605, 420232
No longer depends on: 433566
Summary: Groupbox displays incorrectly on Vista → groupboxes need native theming on Windows

Updated

9 years ago
Depends on: 236107

Updated

9 years ago
Duplicate of this bug: 433566

Comment 31

9 years ago
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 :(
This adds -moz-appearance: groupbox
>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?
Winstripe is the basis for different OSes, eg. linux and os/2, so the ultimate fallback shouldn't depend on widget/src/windows.
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

9 years ago
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).
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 on attachment 364768 [details]
screenshot

Looks great
Attachment #364768 - Flags: ui-review?(faaborg) → ui-review+
Created attachment 365031 [details]
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.
Created attachment 365125 [details] [diff] [review]
patch rev1
Attachment #364887 - Attachment is obsolete: true
Attachment #365125 - Flags: review?(vladimir)
Created attachment 365126 [details]
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
Attachment #365125 - Attachment is patch: true
Attachment #365125 - Attachment mime type: application/octet-stream → text/plain
(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 on attachment 365125 [details] [diff] [review]
patch rev1

looks fine to me!
Attachment #365125 - Flags: review?(vladimir) → review+
(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.
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.
regarding winstripe being used as the defacto default theme I filed bug 481174
Right, I agree that the fallback can't satisfy all platforms. It should be styled so that it's appropriate for Windows.
Whiteboard: [polish-hard][polish-visual] → [polish-hard][polish-visual][needs landing]
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.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
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

9 years ago
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
(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.
Whiteboard: [polish-hard][polish-visual][needs landing] → [polish-hard][polish-visual]

Updated

9 years ago
Blocks: 482972
(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
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?
Status: RESOLVED → VERIFIED
Depends on: 483187
(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
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.
Attachment #365125 - Flags: approval1.9.1?
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 on attachment 365125 [details] [diff] [review]
patch rev1

a191=beltzner
Attachment #365125 - Flags: approval1.9.1? → approval1.9.1+
Depends on: 484013
We'll also want bug 484013 for 1.9.1
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.
Keywords: fixed1.9.1
Target Milestone: mozilla1.9.2a1 → mozilla1.9.1b4
Depends on: 484374
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
Keywords: fixed1.9.1 → verified1.9.1
Target Milestone: mozilla1.9.1b4 → mozilla1.9.2a1

Updated

9 years ago
Depends on: 486955
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.
Whiteboard: [polish-hard][polish-visual] → [polish-hard][polish-visual][polish-p2]
You need to log in before you can comment on or make changes to this bug.