303806 - Wallpaper patch for better appearance on Windows XP / Luna

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Chris Cunningham]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050806 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050806 Firefox/1.0+

<a href="https://bugzilla.mozilla.org/show_bug.cgi?id=243078">Bug 243078</a> is
the "right" way to fix this bug, but it's not going to be fixed in time for Deer
Park, which means another six months or so of Firefox looking like a
seven-year-old Windows 95 app on modern Windows systems:

1. Icon spacing doesn't match Internet Explorer, and appears to have been
designed to maximise viewable space. This is fine assuming that the toolbar
buttons aren't going to be used that much, but having a 16px Back button isn't
great for people with less than 20/20 vision. It also looks ugly.
2. Menu bars have a 3D look which doesn't fully match Windown 95 anyway, but
looks awful on XP Luna. Menu highlights should follow the Windows Luna scheme
for now until the above bug is fixed, because it looks nicer on the most popular
Windows distro at the moment and doesn't look terrible on older versions (most
modern apps, such as recent Office versions, use the Windows XP look regardless).
3. Menu spacing is likewise miniscule, making it harder to hit individual items
as well as being less attractive and readable.

Patch coming up.

Reproducible: Always

Comment 1

[Chris Cunningham]

Attachment: Wallpaper patch to make toolbar buttons and menus look much nicer and be easier to hit on Windows

This is a temporary fix until Windows has "native" rendering of Mozilla menu
bars. It adds padding to toolbar icons which matches their spacing on MSIE and
improves both the visual appearance and target area of menus.

Comment 2

[Jaime Mitchell (use [e-mail] for email)]

Note Bug 253661

Comment 3

[Bryan]

(In reply to comment #1)
> Created an attachment (id=191908) [edit]
> Wallpaper patch to make toolbar buttons and menus look much nicer and be easier
> to hit on Windows
> 
> This is a temporary fix until Windows has "native" rendering of Mozilla menu
> bars. It adds padding to toolbar icons which matches their spacing on MSIE and
> improves both the visual appearance and target area of menus.

Are any of your changes resolved in the patch for bug 253661?  Can you attach a
before and after screenshot that show the changes you propose?

~B

Comment 4

[Chris Cunningham]

This specifically Windows-only and overrides browser.css in the Windows toolkit
rather than attempting to modify Firefox's Winstripe theme because this is a
platform-specific and hopefully short-lived hack which benefits all toolkit apps
on Windows.

253661 just celebrated its first birthday, still isn't fixed and doesn't address
either of the issues mine is intended to (menu highlight colour and toolbar icon
spacing on Windows XP). The two can co-exist. At the moment my sole interest is
in wallpapering over the current inadequacies in native menu support on my own
platform in time for the next browser release. I'd rather avoid any and all
discussion of platform intricacies until it's checked in.

 - Chris

Comment 5

[Asa Dotzler [:asa]]

Comment on attachment 191908 [details] [diff] [review]
Wallpaper patch to make toolbar buttons and menus look much nicer and be easier to hit on Windows

please request approval only after you've got a patch with sufficient reviews.

Comment 6

[Chris Cunningham]

The checkin for 253661 has, as suspected, messed with this patch (menus are too
widely spaced vertically with both applied).

Removing the padding on the menubar > menu selector fixes this and allows the
two patches to co-exist, but it'll be 11 hours until I can get a patch done.

 - Chris

Comment 7

[Chris Cunningham]

Attachment: Mk 2, fixes what 353661 broke

For some reason the patch for 253661 wants to enforce a pressed-in state for
menu bars on Windows despite this having gone out of fashion about five years
ago. This ifxes that and plays nicely with the menu bar spacing. Fixed the
indentation while I was at it.

Should be complete now. There's still some minor pixel bickering, and it
doesn't clone MSIE perfectly for a couple of reasons, but it's a massive
improvement.

 - Chris

Comment 8

[Uriah Narmed]

Is there a way to have these modifications apply *only* to Luna? As it currently
stands, this patch looks rather hideous under Windows 2000 and (presumably) XP
with the theme service disabled.

Comment 9

[Mike Connor [:mconnor]]

Comment on attachment 191994 [details] [diff] [review]
Mk 2, fixes what 353661 broke

theme stuff should get reviewed by the theme guy, in general.

Comment 10

[Bryan]

(In reply to comment #8)
> Is there a way to have these modifications apply *only* to Luna? As it currently
> stands, this patch looks rather hideous under Windows 2000 and (presumably) XP
> with the theme service disabled.

I wholeheartedly agree with comment 8 as testing the code on Windows 2000 and XP
Classic transforms what is/has been a great compromise (Winstripe) in terms of
attempting to integrate FF into both Classic and Luna interfaces into a hideous
beast.  Not to mention this patch introduces quite a few UI bugs that don't
currently exist for those users who have Windows 98/ME/2000 or choose to use XP
Classic.  I will agree that with this patch FF looks more "integrated" when
using XP Luna but would argue that if this is really desired then we either do
on of two things:

1) Create a modified Winstripe theme that is tailored to Luna and is called
Winstripe - Luna and rename the current theme to Winstripe - Classic AND offer
both in the default install of FF!

OR

2) Let's get to work on Bug 243078

~B

Comment 11

[Chris Cunningham]

This isn't design by committee. If you have alternative answers, open other
bugs. This bug is clearly labelled as a vehicle to a specific solution and is
not to be derailed for the sake of those who are specifically excluded from its
benefits.

Dearly seeking review prior to the 1.5 branch. Menu appearance is completely
jarring on the flagship product on the default theme of its most popularly-used
operating system and this minor patch significantly enhances visual appeal of
toolkit apps in this case.

 - Chris

Comment 12

[Grey Nicholson (they/them)]

IE7b1 has got rid of the extra spacing around the toolbar buttons so it now
matches Firefox. Would changing just the style of the menus be acceptable for
Classic?

Comment 13

[Asa Dotzler [:asa]]

Kevin, can you review this patch?

Comment 14

[Kevin Gerich]

Comment on attachment 191994 [details] [diff] [review]
Mk 2, fixes what 353661 broke

I will test on my Windows/Linux box soon.

Please clarify exactly which CSS rules you changed/added as it's not totally
apparent from reading the patch. Menubar changes don't belong in
global/browser.css. What will these changes look like on Win Classic and other
platforms (GTK1?, OS/2?) that use Winstripe?

Comment 15

[Bryan]

(In reply to comment #14)
> (From update of attachment 191994 [details] [diff] [review] [edit])
> I will test on my Windows/Linux box soon.
> 
> Please clarify exactly which CSS rules you changed/added as it's not totally
> apparent from reading the patch. Menubar changes don't belong in
> global/browser.css. What will these changes look like on Win Classic and other
> platforms (GTK1?, OS/2?) that use Winstripe?

Please provide screenshots of the above changes on the respective platforms if
at all possible.  At the very least before and after's of Win2000 and or WinXP
Classic / Luna.

Thank you,

~B

Comment 16

[Chris Cunningham]

> Please clarify exactly which CSS rules you changed/added
> as it's not totally apparent from reading the patch.

That's mostly due to having to fix bug 353661's indentation. The important bit 
is here:

+/* make menus less vertically squashed */
+
+.menu-text,
+.menu-iconic-left,
+.menu-iconic-text  {
+  margin-top: 2px !important;
+  margin-bottom: 2px !important;
+}
+
+/* luna-style menu hovering */
+
+menupopup, popup {
+  border: 1px solid ThreeDShadow !important;
+  -moz-border-left-colors: ThreeDShadow !important;
+  -moz-border-top-colors: ThreeDShadow !important;
+  -moz-border-right-colors: ThreeDShadow !important;
+  -moz-border-bottom-colors: ThreeDShadow !important;
+  padding: 2px !important;
+  background-color: Menu !important;
+}
+menubar > menu {
+  border: 1px solid transparent !important;
+  margin: 0 !important;
+}
+menubar > menu[_moz-menuactive="true"] {
+  background-color : Highlight !important;
+  color: HighlightText !important;
+}
+
+/* size tweaks */
+
+menubar
+{
+  padding-top: 1px !important;
+  margin-bottom: 2px !important;
+}
+
+.tabs-closebutton {
+  padding: 1px !important;
+  border: 0 !important;
+  margin: 0 !important;
+}
+
+.toolbarbutton-icon {
+  padding: 4px;
+}
+
+toolbar[iconsize="large"] .toolbarbutton-icon {
+  padding: 2px;
+}
+
+.toolbarbutton-text {
+  padding-bottom: 1px;
+}
+
+#personal-bookmarks .toolbarbutton-icon {
+  padding: 0;
+}

Lines were lifted directly from my own userChrome.css, so most likely aren't 
in the right place, but this looked like a good place to override everything 
at once.

> Menubar changes don't belong in global/browser.css.

Understood (humour me, I'm new to this). Where should I be patching the menu 
bar?

> What will these changes look like on Win Classic and other
> platforms (GTK1?, OS/2?) that use Winstripe?

These platforms lack a unified look anyway; the changes to the menu bar 
highlighting will look out of place but the additional padding is likely to 
make the app more usable (as it does on Windows).

> Please provide screenshots of the above changes on the respective platforms 
> if at all possible.  At the very least before and after's of Win2000 and or
> WinXP Classic / Luna.

For the peanut gallery and people still using Windows 98 to continue to hijack 
the bug? No thanks. Anyone in a position to comment can apply the patch and 
see for themselves. Please go and help fix the bugs this one was prompted by 
instead of trying to force a group decision on this one. I told you what my 
position was in private mail, and have no intention on justifying myself to 
you in public on my own bug as well.

 - Chris

Comment 17

[Chris Cunningham]

Attachment: The same thing without the iffy patching, applied to the correct files

Previous patches are a mess because cvs diff thinks likes with Unix line
endings are different to files with Windows line endings. Awesome.

Split the changes up into what looks to be the appropriate files. This should
be easier to stomach for people who know wthat they're doing with the theme
system.

 - Chris

Comment 18

[Jos Vermeulen]

Attachment: Comparison between before and after

Comment 19

[Chris Cunningham]

Thanks.

Note that menu vertical padding is designed to match modern Windows apps such as
MS Office, Shareaza etc rather than copying Explorer / MSIE.

I'm only looking for this to be landed on the 1.5 branch. The trunk can wait for
a real solution, like native menu bar handling.

 - Chris

Comment 20

[u88484]

Removing request to block 1.5 since the 1.5flags are not being used anymore and
requesting to block 1.8b4.

Comment 21

[Asa Dotzler [:asa]]

Kevin, we'd like to get this. Can you review?

Comment 22

[Kevin Gerich]

Attachment: after applying patch on branch

Comment 23

[Kevin Gerich]

Comment on attachment 192535 [details] [diff] [review]
The same thing without the iffy patching, applied to the correct files

Sorry for the delay. I applied the pach to my branch build and the result
looked like the above screenshot. 

+#personal-bookmarks .toolbarbutton-icon { 
+  padding: 0; 
+}

This belongs in browser/themes/winstripe/browser/browser.css

The menu popup 1px border was not being applied. I think it's because this

+menupopup, popup {
+  border: 1px solid ThreeDShadow !important;
+  -moz-border-left-colors: ThreeDShadow !important;
+  -moz-border-top-colors: ThreeDShadow !important;
+  -moz-border-right-colors: ThreeDShadow !important;
+  -moz-border-bottom-colors: ThreeDShadow !important;
+  padding: 2px !important;
+  background-color: Menu !important;
+}

is in menu.css when it probably should be in popup.css

Comment 24

[Chris Cunningham]

Attachment: Patch against current branch code with issues addressed

This looks right built against current branch.

 - Chris

Comment 25

[Kevin Gerich]

Comment on attachment 193631 [details] [diff] [review]
Patch against current branch code with issues addressed

looks good

Comment 26

[Chris Cunningham]

Comment on attachment 193631 [details] [diff] [review]
Patch against current branch code with issues addressed

requesting approval and looking for someone to land this.

Comment 27

[Asaf Romano (gone)]

So, I was going to check this in, then realized it makes Firefox look pretty
weird on 2000/XP classic.

Kevin, are you sure we want to break the classic windows theme this way? We were
trying to avoid that for a long time now, the reason for the "policy change"
isn't clear to me.

Comment 28

[Bryan]

(In reply to comment #27)
> So, I was going to check this in, then realized it makes Firefox look pretty
> weird on 2000/XP classic.
> 
> Kevin, are you sure we want to break the classic windows theme this way? We were
> trying to avoid that for a long time now, the reason for the "policy change"
> isn't clear to me.

Asaf,

It also renders "Use Small Icons" pretty much useless on all Windows platforms
as it adds quite a bit of padding around each toolbar icon.  Toolbar tweakers of
which there are many will scream!!!  There are also several "issues" introduced
by this patch, albeit nothing major just an eyesore here and there.

~B

Comment 29

[Kevin Gerich]

(In reply to comment #27)
> So, I was going to check this in, then realized it makes Firefox look pretty
> weird on 2000/XP classic.
> 
> Kevin, are you sure we want to break the classic windows theme this way? We were
> trying to avoid that for a long time now, the reason for the "policy change"
> isn't clear to me.

Asa wrote "we'd like to get this", and made this block 1.8b4. The "policy
change" isn't clear to me either. Personally I don't like the Classic breakage,
but on the other hand the current menu style has a Classic look on Luna, which
looks a bit strange. I don't feel very strongly about it, but I'd support a
decision to not check this in. I've emailed Asa about it.

Comment 30

[Asaf Romano (gone)]

Comment on attachment 193631 [details] [diff] [review]
Patch against current branch code with issues addressed

Minusing. At the very least, you should make sure Gnomestipe isn't affected by
supporting XP-Luna, mainly because the default system-theme on GNOME is pretty
similar to the current classic look. We have a sub-theme (themes/gnomestripe)
in which you can override Winstripe's style rules.

All of this assumes the classic breakage was part of "we want this".

Comment 31

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

Weighing in a little late, and sadly without a machine that's able to test the
patch locally (although hope to have that running soon!). From the screenshots
that I'm looking at in attachment 192678 [details] I can't see the breakages Mano and
Bryan are talking about in comment 26 and comment 27, although I can see the
wider spacing on the menuitems which *is* consistent with XP widgets and MS
Office 2003 but *isn't* consistent with IE6/IE7b1.

If the real issue here is the 3-d menu highlight business, can we just take
*that* change and leave the rest for now? We're obviously going to have to
either decide to not support classic or nor *fully* support Luna until bug
243078 is resolved.

Comment 32

[Kevin Gerich]

(In reply to comment #30)
> (From update of attachment 193631 [details] [diff] [review] [edit])
> Minusing. At the very least, you should make sure Gnomestipe isn't affected by
> supporting XP-Luna, mainly because the default system-theme on GNOME is pretty
> similar to the current classic look. We have a sub-theme (themes/gnomestripe)
> in which you can override Winstripe's style rules.

Gnomestripe already has its own popup.css and menu.css, so the menu style
changes should not apply there. 

Comment 33

[Asaf Romano (gone)]

Yes, but there are also changes to browser.css(s).

Comment 34

[Bryan]

(In reply to comment #31)
> From the screenshots that I'm looking at in attachment 192678 [details] [edit] I can't   
> see the breakages Mano and Bryan are talking about in comment 26 and comment 
> > 27, although I can see the wider spacing on the menuitems which *is*       
> consistent with XP widgets and MS Office 2003 but *isn't* consistent with    
> IE6/IE7b1.
Mike,

The screenshots are not an accurate representation of the full impact on
Classic.  For one it shows Classis with large icons.  Enabling small icons does
give you small icons but with all of the extra padding around them there is no
difference between the large icons.  What is the goal of the feature "Use Small
Icons"?  Is the goal not to allow the user to have a small toolbar?  With this
patch the large majority of the FF population will be stuck with a large toolbar
(those that don't know how to tweak FF via userChrome.css).  Also the
screenshots do not show the huge highlight/divider created in the bookmarks
manager that looks fugly in Classic.  Not only does Classic look "hideous" as
comment 8 states but there are several of these eyesores that the screenshots do
not show on Classic.

As a compromise is it not possible to do what I suggest in comment 10?  Ship two
themes in FF on windows?  Winstripe - Classic and Winstripe - Luna?

~B

Comment 35

[Wolf [:wolf]]

The 3d menu highlight is the only change I'm seeing here thats definitely an
improvement. (based on the screenshots, so i'm very likely missing stuff. :-))

I don't think shipping two default themes is the answer, as there should be an
ok, if not ideal, compromise here. (Plus, this probably isn't a big enough deal
to warrant the extra code.)

What's the reasoning for changing the padding around the small toolbar buttons
(as Comment 28 and Comment 34 mention)?

Comment 36

[Bryan]

(In reply to comment #35)
> What's the reasoning for changing the padding around the small toolbar buttons
> (as Comment 28 and Comment 34 mention)?

With IE7 coming out would it not be better to focus on that (Considering FF 1.5
and IE 7 will be out around the same time if I recall correctly, give or take a
bit)? We already know that M$ has changed the look of the toolbar in terms of
padding (see comment 12 and comment 31) and it will be available on XP SP2
machines.  If our goal really is to replicate/emulate IE (since that is the app
we're replacing) I believe that makes sense as long as it doesn't make Classic
look like **** (keep in mind M$ still supports Classic)

~B

Comment 37

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

Shipping two default themes isn't the right answer. We don't want to force users
to have to change themes (that function is an enhancement for tinkerers, not a
requirement for Classic users). That said, I'd be happy for a "better classic
theme" to be posted front and center on AMO at some point for the few users who
really, really care about that sort of thing.

Also, let's not worry too much about chasing IE7, especially not based on what
we're seeing in screencaps on Ars Technica reviews of Vista or in the public
beta. We should be doing what is a) the most consistent possible with the
platform, and b) what ends up being best for the user.

For example: large padding around small icons isn't best for the user, and if
that's a fallout from this patch, then this patch shouldn't be taken.

Comment 38

[Chris Cunningham]

I must be missing something, because I could swear that it was me who opened
this bug, my comment #0 which explains exactly what it's doing and my patch.
Therefore it puzzles me that said patch has now been minused as if it were
defective, apparently as a tool to override approval.

Addressing these one by one.

Comment #27: Windows hasn't shipped using a Classic theme for nearly five years.
The Classic look was "policy" because the native theming stuff doesn't extend to
menus on Windows (see bug 243078, already linked from comment #1), and this bug
was deliberately designed to force a "policy change" by actually doing the
legwork of submitting a patch.

Comment #28: Take it up with Microsoft. This patch makes Firefox's toolbar
button padding in both modes match that of Windows in its default configuration.
I am as interested in hearing the cries of sorrow from people who want to use
the default theme in some "minimalist" configuration with all the buttons jammed
up on the menu bar or whatever as I am in hearing how crummy this looks in Classic.

Comment #30: the only change to browser.css is an override of the new toolbar
button padding from Winstripe (and some indentation fixes). It will have no
effect on Gnomestripe.

Comment #31: the bug is for both, review and approval were asked for both and
were granted for both.

Comment #35: Small icons with large padding (a) matches the Windows Luna
appearance exactly and (b) makes small icons usefully easy to hit. This is why
Microsoft increased button padding for Luna and it's why I added it here.

Let's reiterate. Classic is ugly, unusable and antiquated. In my opinion it is
better to have a default configuration which looks right on the default
appearance of the most popular host OS for the application than one which does
not look right on the default appearance of the most popular host OS for the
application. These changes only make Classic look as awkward as Luna looks now,
only mith easier-to-hit buttons.

I'd really like that r+ restored considering that the patch does what it says it
does and already has approval.

 - Chris

Comment 39

[Asaf Romano (gone)]

We're not going to totally break the Classic theme (to be clear, i do consider
the last patch as doing so) becuase it's not the default; it's clearly something
we're trying to support (and already support in 1.0!). The fact that reading
this bug doesn't make it clear (at least until comment 27) scares me.

Renominating the value of this...

Comment 40

[Ben Basson]

Attachment: Other menus

Microsoft Office, OpenOffice and Limewire don't look correct under Windows
Classic either. Screenshot attached.

Comment 41

[Chris Cunningham]

Not breaking Classic means breaking Luna. This is a binary decision. I want the
bit flipped, and comment #0 makes my position abundantly clear. If by "we" you
mean drivers, I'd point out that Asa's position has been pointed out twice now:
by originally posting bug 253661, and by approving this.

To put it mildly, the last 24 hours have not been particularly pleasant for a
first-time contributor. It would appear that it is considerably easier to stop
something from getting it into the tree than it is to land it. With the patch
written and working, I now have a month to go and lobby people to try and get my
review and approval again for 1.5b2, and barely any net access to do so.

 - Chris

Comment 42

[Wolf [:wolf]]

This bug really needs an attitude check, based on several comments here. Comment
#11 and #16 (final paraphraph) don't exactly give the feel of wanting an easy
time pushing a change, instead they're dismissive and defensive, rather than
actually responding to the concerns brought up here. 

That being said, my thoughts here.

Small Buttons w/ Large Padding match IE6's definition of Luna. IE7b1 on XP does
not do the same. Also, Large Padding isn't present in Office 2003, and those
toolbars are no less usable. So blindly following IE6 here is most likely bad. I
don't want to see these changes land. I think it breaks the compactness of small
icons for no real benefit. If you need the extra space to hit the icon, use the
large icons. That's kinda the point. :-)

The 3d-menu change is right on, as the current state of things is pretty ugly. I
hope to see this in. 

The padding/border changes are trying to track a moving target. Classic on
Windows 2003 (upon which supposedly Vista was reset around before moving
forward.) is different than Classic on XP. On 2003, Classic's menus are non-3d,
which is more Luna-ish. Office 2003 does its own thing on Luna, and many apps
follow it. I think the best idea here is for Firefox, to simply make its menus
as usable as possible, and try to be as native as possible. Without nitpicking
the appearance to death, which I think these changes might be trying to do, but
I'm neutral to this change. Its up to those responsible for policy on such
things and the UI of the browser to decide. :-)

Comment 43

[Chris Cunningham]

> Without nitpicking the appearance to death, which I think 
> these changes might be trying to do, but I'm neutral to this 
> change. Its up to those responsible for policy on such
> things and the UI of the browser to decide. :-)

This patch already had review from the Winstripe owner and approval from a driver.

The word "wallpaper" is in the bug description for a reason. This is not a
future-proof patch. It is not the best solution to the problem. It is a trivial
cosmetic fix for an imminently-shipping product. I'm not even looking for it to
get trunk checkin, given its status as a stop-gap measure.

> If you need the extra space to hit the icon, use the
> large icons. That's kinda the point.

The only reason the large / small toolbar icons choice even exists is to mimic
Internet Explorer. Given the existence of alternative themes such as Microfox or
Noia which cater specifically for people who use alternative toolbar
configurations and icon sizes, I don't see why the default padding shouldn't
match that of IE (Luna might not be perfect, but it was tested on some real
human beings).

I'm sorry for the attitude, but I felt that hanging a sign on the bug saying "if
you don't actually use XP Luna, hit Back now" was inappropriate before
commentary was generated. I'd hoped that the initial comment had made it quite
clear that I wasn't interested in a compromise solution for 1.5.

 - Chris

Comment 44

[Asa Dotzler [:asa]]

This bug is going to land on the branch. It's approved to land and we want it.
End of story. 

Comment 45

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

I see now that attachment 193522 [details] illustrates the new padding on small icons.
While I think they're a little *too* padded out now, to the point where they
look like they're wasting a bit of vertical space, it's not as bad as I was
imagining from the comments that have been posted.

Comment 46

[Asaf Romano (gone)]

checked in on the branch:
Checking in browser/themes/winstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v  <--  browser.css
new revision: 1.17.2.2; previous revision: 1.17.2.1
done
Checking in toolkit/themes/winstripe/global/browser.css;
/cvsroot/mozilla/toolkit/themes/winstripe/global/browser.css,v  <--  browser.css
new revision: 1.9.4.1; previous revision: 1.9
done
Checking in toolkit/themes/winstripe/global/menu.css;
/cvsroot/mozilla/toolkit/themes/winstripe/global/menu.css,v  <--  menu.css
new revision: 1.5.2.2; previous revision: 1.5.2.1
done
Checking in toolkit/themes/winstripe/global/popup.css;
/cvsroot/mozilla/toolkit/themes/winstripe/global/popup.css,v  <--  popup.css
new revision: 1.4.2.1; previous revision: 1.4
done
Checking in toolkit/themes/winstripe/global/toolbarbutton.css;
/cvsroot/mozilla/toolkit/themes/winstripe/global/toolbarbutton.css,v  <-- 
toolbarbutton.css
new revision: 1.7.2.1; previous revision: 1.7
done

Comment 47

[Grey Nicholson (they/them)]

Is there an equivalent bug for Thunderbird or should I file it?

Comment 48

[Peter van der Woude [:Peter6]]

Attachment: screenshot win2k/classic - FF default theme

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050901
Firefox/1.0+ ID:2005090201

On Win2k/classic this is outright horrible.
It's throwing away pixels by the truckload and doesn't fit in either.
Is there any chance that non XP windows versions keep it as was ?

Comment 49

[Mathieu Pellerin]

omg, you can't be shipping a product with those kind of menu, it's totaly
breaking the feeling of firefox integrating gently with your system if using
win9x/me/nt/2k or newer version with classic theme.

Comment 50

[Peter van der Woude [:Peter6]]

I filed Bug 306827 for comment #48

Comment 51

[Chris Cunningham]

(In reply to comment #47)
> Is there an equivalent bug for Thunderbird or should I file it?

Go right ahead. All that needs to be done is for the changes to be checked into
toolkit/themes/qute.

 - Chris

Comment 52

[Jos Vermeulen]

The pre-checkin state blended in with almost all themes. However, now it only
blends in with a couple of themes (keep in mind there ARE other themes than
Luna).  There's too much space wasted (also on Luna).

Also, I worked for a helpdesk a while and there are a lot of requests to make
Windows look "the way it did". Just because it's shipped with it, doesn't mean
people use it.

Also (2), there are a lot of people/companies not using Windows XP itself as an
operating system.

Comment 53

[Chris Cunningham]

Another Classic checkin made the branch since this was reviewed, which has added
an additional border above the menu bar and a couple of extra pixels padding.
These need hacked out for 1.5. Additional patch coming up.

 - Chris

Comment 54

[Ben Goodger (use ben at mozilla dot org for email)]

OK - I do not approve of this change. The browser now looks completely different
(9/2/2005). We've had discussions about spacing in the past and while there have
occasionally been proponents for wider spacing we have always gone with trimmer
spacing. 

In addition to the staypuff spacing, there's now also a mysterious groove at the
top of the browser window. 

I will be backing out this patch today. I don't disapprove of the general menu
changes (since they look strange on lunaxp), but the rest of this has to go. 

Comment 55

[dimitrisp]

Attachment: Demonstration of difference with IE on WinXP and the default theme.

I think it wastes too much space.. see screenshot.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050719
Firefox/1.0+ Mnenhy/0.7.2.0 ID:2005090207

Comment 56

[Ben Goodger (use ben at mozilla dot org for email)]

All major browser design and UI changes must now be approved by me.

Comment 57

[Chris Beard]

(In reply to comment #53)
> Another Classic checkin made the branch since this was reviewed, which has added
> an additional border above the menu bar and a couple of extra pixels padding.
> These need hacked out for 1.5. Additional patch coming up.

Where is this patch?  Who reviewed it?  Who approved the checkin?

/cb

Comment 58

[Chris Beard]

It looks like we should back out the changes which add the groove at the top and
the extra padding, but keep the menu changes.

/cb

Comment 59

[Chris Beard]

marking this bug as blocking1.8b4 and removing fixed1.8 to put it back on our
radar for 1.5

Comment 60

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: backing this out

I'm backing out this patch until the padding issues are fixed. This backout
patch also backs out rv1.6.8.1 of toolbar.css which adds an unnecessary groove
to the top of all toolboxes. Sorry, Gavin ;-) 

The patch for 303806 was checked in only to the branch so I'm backing it out of
there, although I think I'll have to back out toolbar.css from both places to
correct the groove. 

If you want to fix the menus, remove all the excess padding and then let's try
for another version. I'll review.

Comment 61

[Ben Goodger (use ben at mozilla dot org for email)]

This backout has been checked in on the branch. The backout for the toolbar.css
part of 301039 has been checked in on the branch and the trunk. 

Comment 62

[:Gavin Sharp [email: [e-mail]]]

I'm not sure why you're apologizing to me, none of these changes were mine.
Personally, I didn't like the groove added in bug 253661, I just checked in a
followup fix that made things consistent once the initial patch was reviewed,
approved, and landed. Maybe you're confusing me with someone else?

Comment 63

[Asa Dotzler [:asa]]

Chris, can you provide a patch with all of the menu changes but none of the
toolbar padding changes?

Comment 64

[:Gavin Sharp [email: [e-mail]]]

To be clear, from what I had gathered, bug 301039's patch was just about fixing
an inconsistency with the border that was added in bug 253661. I think it's
about time someone steps in a cleans up this mess, so no need to apologize to me! :)

Comment 65

[Ben Goodger (use ben at mozilla dot org for email)]

No groove at the top. 

All that is required are the menu changes. 

Comment 66

[Chris Cunningham]

Killing the changes to browser.css (both of them) and toolbarbutton.css will
remove the button padding changes. With the offending "groove" patch (not mine,
thanks) backed out, that should make menus perfect on Luna. I'm not in a
position to produce a patch right at this minute but quite a few people on the
cc list are.

Firefox's UI "policy" issues are still pretty opaque to outsiders, by the way.
Currently there is little external contributors can do other than lobby drivers
and submit patches which reflect their point of view, and in the last month two
alternative camps of contributors have both been getting reviews and approval
for patches which directly clash with one another. I hope that the new policy of
super-approval is going to be accompanied by some documentation indicating the
desired direction of the UI.

 - Chris

Comment 67

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

(In reply to comment #66)
> Firefox's UI "policy" issues are still pretty opaque to outsiders, by the way.

I certainly hope you're not implying / assuming that our UI policies are
transparent to insiders ... or even that we have them at all.

> alternative camps of contributors have both been getting reviews and approval
> for patches which directly clash with one another. I hope that the new policy 

Well, you've also stepped into one of the most subjective issues available, that
is to say you're looking at theme and UI style changes. Historically, in any
industry, aesthetic design by committee has been a disaster, and conflicts with
traditional OSS processes. You're quite right in comment 0 when you say that we
should be addressing bug 243078 - that's the right answer. Until then, we're
always going to be pissing someone off, meaning that these bugs will be snipe
fests with the eventual goal of mitigating/compromising to some solution that's
equal parts paletable and nauseating to various groups.

> super-approval is going to be accompanied by some documentation indicating the
> desired direction of the UI.

That'd be nice, and it's our hope to get that squared away for v2.0. Although my
comments might be / have been pithy, that's just my style; I honestly appreciate
your effort and thoughts on this matter, Chris.

Comment 68

[Chris Cunningham]

Attachment: Previous patch sans toolbar changes

Hand-hacked patch removing toolbar padding.

I still can't address this properly with the right tools, but from first
principles this should work with boxerboi's last Classic patch backed out.

I have no idea what flags are appropriate here, so feel free to set them (and
please God document the flagging process better).

Comment 69

[Oliver Saier]

(In reply to comment #68)
> Created an attachment (id=194797) [edit]
> Previous patch sans toolbar changes
> 
> Hand-hacked patch removing toolbar padding.
It makes the nemu bar 1px thicker that in eg Win Explorer. May I suggest:
 /* ::::: menu/menuitems in menubar ::::: */
 
 menubar > menu {
-  border: 1px solid transparent;
/*-*/   padding-top: 0px;
+  border: 1px solid transparent !important;
/*+  padding-top: 1px;*/

 menubar > menu[_moz-menuactive="true"] {
/*+  padding: 1px 3px 1px 2px !important;*/
+  padding: 0px 3px 1px 2px !important;

Comment 70

[Oliver Saier]

hmm sorry it's still 1px thicker.
here:
 menubar > menu {
-   padding-bottom: 1px;*
+   padding-bottom: 0px;*

 menubar > menu[_moz-menuactive="true"] {
+  padding: 0px 3px 0px 2px !important;

Comment 71

[Mike Connor [:mconnor]]

Attachment: patch, seems to be pixel-perfect in default Luna

hand-editing patches is usually problematic, but in this case patch managed to
fuzz things.  Tested on default Luna XP Pro, this seems to be a pixel-perfect
match (I was able to copy/paste the highlighted Tools menu option between
screencap excerpts and it worked).

There was a concern about this change affecting GTK2 builds, but gnomestripe
overrides these files so these changes shouldn't ever affect GTK2.

Comment 72

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 195008 [details] [diff] [review]
patch, seems to be pixel-perfect in default Luna

Looks good to me!

r=[e-mail]

Comment 73

[Oliver Saier]

Ok thickness is good but what about the highlight rectangle? As for what I'm
concerned it'd be fine, but it actually matches Notepad's bar:
- 0px away from titlebar
- 1px away from toolbar
- 0px away from windows's left frame (when highlighting File)

Items is Win Explorer are highlighted differently:
- 1px away from titlebar
- 2px away from toolbar
- 2px away from windows's left frame (when highlighting File)

Items is Internet Explorer are highlighted differently again:
- 0px away from titlebar
- 1px away from toolbar
- 2px away from windows's left frame (when highlighting File)

Comment 74

[Oliver Saier]

You also know that eg Notepad's toolbar is 2px thinner than IE's. That's a
hybrid bar we have now =)

Comment 75

[Asa Dotzler [:asa]]

Comment on attachment 195008 [details] [diff] [review]
patch, seems to be pixel-perfect in default Luna

please get this in ASAP.

Comment 76

[Mike Schroepfer]

a=schrep

Comment 77

[Asa Dotzler [:asa]]

 Mike, can you land this for us?

Comment 78

[Mike Connor [:mconnor]]

fixed branch/trunk

Comment 79

[Mathieu Pellerin]

Attachment: menu as seen on classic theme/9x/2k

I thought some people made it clear this change was destructive? Please
revert...

Comment 80

[elfguy]

Why can't this simply be a config option. Add the proper stuff to the
userChrome.css file if the user sets the option to "non-ugly mode"?

Comment 81

[Mike Connor [:mconnor]]

Its not destructive, it just doesn't look quite right on non-Luna themes, which
was a choice we made.  Either we look right on the majority of systems, or the
minority.  Please take advocacy comments elsewhere.

Comment 82

[Marian POPESCU]

(In reply to comment #71)
> Created an attachment (id=195008) [edit]
> patch, seems to be pixel-perfect in default Luna
> 
> hand-editing patches is usually problematic, but in this case patch managed to
> fuzz things.  Tested on default Luna XP Pro, this seems to be a pixel-perfect
> match (I was able to copy/paste the highlighted Tools menu option between
> screencap excerpts and it worked).
> 
> There was a concern about this change affecting GTK2 builds, but gnomestripe
> overrides these files so these changes shouldn't ever affect GTK2.

There is somehow a problem with this patch: you can see in the attached image.
I use Fedora Core 4 (with standard libraries and latest updates) and I see this
strange behaviour: when closing Firefox the "sanitize" window is displayed but
it has no 3D effects (controls are "flat" and border-less).

Comment 83

[Marian POPESCU]

Attachment: Sanitize window with no 3D effects on controls

The sanitize window shows up when closing Firefox but it has no visible 3D
effects (controls are displayed "flat"); when starting again Firefox I can see
the sanitize window with 3D effects this time. 
Can be reproduced with a clean, new profile.

Comment 84

[Bryan]

(In reply to comment #81)
> Its not destructive, it just doesn't look quite right on non-Luna themes, which
> was a choice we made.  Either we look right on the majority of systems, or the
> minority.  Please take advocacy comments elsewhere.

I agree it's not destructive but it does look unprofessional in Classic as parts
of FF don't have this hideous new look as it appears someone didn't have enough
"wallpaper" to take care of the JavaScript Console.  Also I do not like the
additional padding in the menus.  No it's not as bad as it was when the previous
patch landed but it's still more then it was prior to this bug.  Despite what
some have said I have not seen any real numbers in terms of "majority of
systems".  We have no idea how many ppl use XP Classic or XP Luna, as it stands
it's mere speculation.  I do know that my company is rolling out XP with the
Classic interface to thousands of machines.  

~B

Comment 85

[u88484]

Please post "off-topic" thoughts and comments about how ugly this is and why it
should be backed out in
http://forums.mozillazine.org/viewtopic.php?p=1724154#1724154 so as to quit bug
spamming.

Comment 86

[Worcester12345]

Has anyone had a look at these?

http://forums.mozillazine.org/viewtopic.php?t=55084&highlight=office+XP+Outlook
http://forums.mozillazine.org/viewtopic.php?t=238325&highlight=office+XP+Outlook
http://forums.mozillazine.org/viewtopic.php?t=3530&highlight=office+XP+Outlook
http://forums.mozillazine.org/viewtopic.php?t=1120&highlight=office+XP+Outlook
http://forums.mozillazine.org/viewtopic.php?t=237351&highlight=office+XP+Outlook
http://forums.mozillazine.org/viewtopic.php?t=122886&highlight=office+XP+Outlook

These might be some help for those who are in need of modified themes for one
reason or another. I am just more concerned that functionality continues to
improve. Thanks.

Comment 87

[Worcester12345]

I guess I should have added that those threads may also shed some further light
on how to best improve this bug fix if necessary to make the program work the
best way for the most people. Just trying to help here.

Comment 88

[Mathieu Pellerin]

Connor, ... I don't even know where to start ... Luna is NOT the majority of
systems, is certainly NOT the majority of systems Firefox is running on and
potentialy, the number of luna-system running Firefox will decrease as IE7 jumps in.

Asa said it several times, one strong point of firefox is its compatibility with
9x/NT/2k systems. Your breaking it apart; your slowly making it look like
Opera's interface is a perfect integration with the OS (do I have to point out
sarcasm here?)

I dont get it, really; the majority of brainers in Mozilla thinks it's a good
change?



------- Additional Comment #81 From Mike Connor 2005-09-07 06:32 PDT [reply] -------

Its not destructive, it just doesn't look quite right on non-Luna themes, which
was a choice we made.  Either we look right on the majority of systems, or the
minority.  Please take advocacy comments elsewhere.

Comment 89

[u88484]

Think I found three more bugs that this introduced.

1) Go to Tools->Options->Downloads, The box that shows the place to save the
bar, it is colord grey.
2) Click browse in Tools->Options->Downloads->In the input bar, the text is not
centered vertically.
3) type text into any input box, high the text, (text turns whites) and
highlight is right up against the text so say the first letter is an L or R the
first pixel of each letter is blended into the input box.
4) Bookmark manager->click move button->the focus ring is against the text on
the first folder. 

Comment 90

[John Hawkins]

After checkin noticed a new white line visible at top of help dialogs -- ran
search for dupes, found none - filed bug: 

https://bugzilla.mozilla.org/show_bug.cgi?id=307447

Comment 91

[u88484]

Attachment: Screenshot of bugs introduced by this patch

All of these bugs might not have been introduced by this patch but I just
noticed them all today.

Comment 92

[Cpuroast]

Attachment: Menu padding between Firefox and Windows Explorer using Luna

It seems that with Luna active, Firefox still has more padding then windows
explorer itself, which makes it look odd :)

This is on :
Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8b4) Gecko/20050907
Firefox/1.4

Which I assume has the patch landed.

Comment 93

[Chris Cunningham]

[e-mail]: this bug is FIXED. The procedure for dealing with bugs
which are found after a bug is FIXED is to open a new bug on them. Considering
how trivial the final patch is (only sixteen lines have changed) I'm highly
sceptical that it broke thing unrelated to menu bars anyway, so this is the
wrong place to be bringing them up.

[e-mail]: either you have some toolbar button bumping up the padding
off the side of your menu bar or you're using an old build / patch. The menu bar
is exactly the same height as on Internet Explorer in the final patch. As for
the menus being wider, you happen to have picked a menu where the labels +
mnemonics force the menu to be wider than the IE one shown.

 - Chris

Comment 94

[scratch]

[e-mail]: keep in mind that the menu height in both internet
explorer and firefox is relative to your system menu height, and probably with
different variables.

Comment 95

[psellhorst]

(In reply to comment #81)
> Either we look right on the majority of systems, or the minority.

...or you use native widgets (for the UI, not the content area)
and look right on *all* systems.

Comment 96

[Tarmo Järvalt]

Attachment: userChrome.css to restore the Classic look

Not perfect but good enought for me to get rid of that ugly Luna induced ****.

Comment 97

[Tarmo Järvalt]

Or alternatively:
http://kb.mozillazine.org/Firefox_windows_classic

about:config rightclick menu is still borked though.

Comment 98

[Jerry Baker]

(In reply to comment #78)
> fixed branch/trunk

And within two days of landing this "fix" there have been five bugs filed
because now Firefox looks stupid. For some reason they are being marked as a
duplicate of bug 243078. So much for the theory that there will be less problem
this way.

Comment 99

[Shayne Jewers]

maybe something could be done like the classic menus extension that detects if
your using luna or classic, then uses either menu style? That way we could have
the best of both worlds without making it ugly on one theme and good on the other.

Comment 100

[Ng Ming Hong]

I don't want to spam this closed bug. But FYI an extension is already available:
http://ilpolipo.free.fr/fx/classicmenus/

Comment 101

[Jerry Baker]

(In reply to comment #44)
> This bug is going to land on the branch. It's approved to land and we want it.
> End of story. 

Asa, anyone who has participated in Mozilla already knows that it is not rule by
democracy, mertiocracy, or anything other than aristocracy in the worst sense. I
think reminding people of that is probably redundant.

Comment 102

[Bug Tracker]

This fix possibly created a regression please see BUG 321357.