Closed
Bug 390734
Opened 17 years ago
Closed 16 years ago
groupboxes need native theming on Windows
Categories
(Core :: Widget: Win32, defect)
Tracking
()
VERIFIED
FIXED
mozilla1.9.2a1
People
(Reporter: faaborg, Assigned: robert.strong.bugs)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Keywords: polish, verified1.9.1, Whiteboard: [polish-hard][polish-visual][polish-p2])
Attachments
(8 files, 1 obsolete file)
37.35 KB,
image/png
|
Details | |
21.35 KB,
image/png
|
Details | |
699 bytes,
text/html
|
Details | |
7.38 KB,
image/png
|
faaborg
:
ui-review+
|
Details |
228.71 KB,
image/png
|
Details | |
232.44 KB,
image/png
|
Details | |
8.01 KB,
patch
|
vlad
:
review+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
48.72 KB,
image/png
|
Details |
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•17 years ago
|
||
Reporter | ||
Comment 2•17 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•17 years ago
|
||
Alternatively, we could drop the use of groupboxes entirely, in favor of horizontal bars and icons.
Reporter | ||
Comment 4•17 years ago
|
||
This is an example of using horizontal bars and icons in a Vista preferences dialog, from Windows Calendar.
Updated•17 years ago
|
Blocks: vista-theme
Comment 5•17 years ago
|
||
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.
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•17 years ago
|
||
ThreeDHighlight should work instead of white, right?
Comment 10•17 years ago
|
||
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
Comment 11•17 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•17 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 ;-)
Reporter | ||
Updated•16 years ago
|
Whiteboard: [polish-hard][polish-visual]
Reporter | ||
Comment 13•16 years ago
|
||
Rob: is there a dupe of this bug that you've been working on?
Assignee | ||
Comment 14•16 years ago
|
||
Reporter | ||
Updated•16 years ago
|
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → DUPLICATE
Updated•16 years ago
|
Status: RESOLVED → VERIFIED
Assignee | ||
Updated•16 years ago
|
Status: VERIFIED → REOPENED
Resolution: DUPLICATE → ---
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → robert.bugzilla
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•16 years ago
|
Assignee | ||
Updated•16 years ago
|
Assignee: robert.bugzilla → nobody
Keywords: fixed1.9.1
Assignee | ||
Comment 16•16 years ago
|
||
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)
Comment 17•16 years ago
|
||
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•16 years ago
|
||
So shall we use this bug for the border in general? If yes, could you please update the summary?
Assignee | ||
Updated•16 years ago
|
Attachment #364768 -
Flags: review?(faaborg) → superreview?(faaborg)
Assignee | ||
Comment 19•16 years ago
|
||
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.
Assignee | ||
Comment 20•16 years ago
|
||
(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•16 years ago
|
||
Comment on attachment 364768 [details]
screenshot
You probably wanted to ask for ui-review.
Attachment #364768 -
Flags: superreview?(faaborg) → ui-review?(faaborg)
Assignee | ||
Comment 22•16 years ago
|
||
Thanks... I wasn't wearing my damn glasses. ;)
Assignee | ||
Comment 23•16 years ago
|
||
fyi: filed Bug 480883 for issues with widgets not providing consistent start margin / padding values which prevent children of groupboxes aligning properly on Vista.
Assignee | ||
Comment 24•16 years ago
|
||
Assignee | ||
Comment 25•16 years ago
|
||
Needs more padding for the bottom of the groupbox
Assignee | ||
Comment 26•16 years ago
|
||
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.
Assignee | ||
Comment 27•16 years ago
|
||
Filed bug 480898 for the issue with the sanitize dialogs
Assignee | ||
Comment 28•16 years ago
|
||
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 29•16 years ago
|
||
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•16 years ago
|
No longer blocks: 405605, vista-theme
No longer depends on: 433566
Summary: Groupbox displays incorrectly on Vista → groupboxes need native theming on Windows
Comment 31•16 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 :(
Assignee | ||
Comment 32•16 years ago
|
||
This adds -moz-appearance: groupbox
Assignee | ||
Comment 33•16 years ago
|
||
>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•16 years ago
|
||
Winstripe is the basis for different OSes, eg. linux and os/2, so the ultimate fallback shouldn't depend on widget/src/windows.
Assignee | ||
Comment 35•16 years ago
|
||
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•16 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).
Assignee | ||
Comment 37•16 years ago
|
||
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. :(
Reporter | ||
Comment 38•16 years ago
|
||
Comment on attachment 364768 [details]
screenshot
Looks great
Attachment #364768 -
Flags: ui-review?(faaborg) → ui-review+
Assignee | ||
Comment 39•16 years ago
|
||
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.
Assignee | ||
Comment 40•16 years ago
|
||
Attachment #364887 -
Attachment is obsolete: true
Attachment #365125 -
Flags: review?(vladimir)
Assignee | ||
Comment 41•16 years ago
|
||
I went with the styling on the left since it IMO most closely matches Windows classic on XP and Vista
Assignee | ||
Updated•16 years ago
|
Attachment #365125 -
Attachment is patch: true
Attachment #365125 -
Attachment mime type: application/octet-stream → text/plain
Comment 42•16 years ago
|
||
(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+
Comment 44•16 years ago
|
||
(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.
Assignee | ||
Comment 45•16 years ago
|
||
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.
Assignee | ||
Comment 46•16 years ago
|
||
regarding winstripe being used as the defacto default theme I filed bug 481174
Comment 47•16 years ago
|
||
Right, I agree that the fallback can't satisfy all platforms. It should be styled so that it's appropriate for Windows.
Updated•16 years ago
|
Whiteboard: [polish-hard][polish-visual] → [polish-hard][polish-visual][needs landing]
Assignee | ||
Comment 48•16 years ago
|
||
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
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Assignee | ||
Comment 49•16 years ago
|
||
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•16 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
Assignee | ||
Comment 51•16 years ago
|
||
(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]
Assignee | ||
Comment 52•16 years ago
|
||
(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•16 years ago
|
||
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
Assignee | ||
Comment 54•16 years ago
|
||
(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
Assignee | ||
Comment 55•16 years ago
|
||
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.
Assignee | ||
Updated•16 years ago
|
Attachment #365125 -
Flags: approval1.9.1?
Assignee | ||
Comment 56•16 years ago
|
||
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•16 years ago
|
||
Comment on attachment 365125 [details] [diff] [review]
patch rev1
a191=beltzner
Attachment #365125 -
Flags: approval1.9.1? → approval1.9.1+
Assignee | ||
Comment 58•16 years ago
|
||
We'll also want bug 484013 for 1.9.1
Assignee | ||
Comment 59•16 years ago
|
||
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
Comment 60•16 years ago
|
||
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
Reporter | ||
Comment 61•16 years ago
|
||
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.
Description
•