... which is not only against platform standards but also annoying when there's no icon to react to hover (try the Error Console's Evaluate button).

Yeah, the changes to toolkit's toolbarbutton.css should be reverted.

Can the whole theme be reverted? This has to be a joke. I apologize for the SPAM. I know everything hasn't been checked in but you have to be kidding me. The icons checked in thus far look very "amateurish". Check these links for a professionally done visual refresh of the winstripe icons... http://cheeaun.phoenity.com/weblog/2004/06/back-to-winstripe.html http://cheeaun.phoenity.com/weblog/2004/06/homesick-winstripe.html http://cheeaun.phoenity.com/weblog/2004/06/winstripe-stoppage.html http://cheeaun.phoenity.com/weblog/2004/06/winstripe-reloaded.html http://cheeaun.phoenity.com/weblog/2004/08/scrollbar-tweaks.html http://cheeaun.phoenity.com/weblog/2004/08/firefox-dust-2.html ~B

(In reply to comment #2) > Can the whole theme be reverted? This has to be a joke. Please stop spamming, comments like these aren't productive.

This reverts the changes to toolkit's toolbarbutton.css in favor of a specific rule on the go button. The Firefox toolbar is not affected by this patch.

*** Bug 347555 has been marked as a duplicate of this bug. ***

The previous patch did affect the main toolbar when using Luna, this one doesn't.

Looking at today's Thunderbird2 build (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060805 Thunderbird/2.0a1 ID:2006080506), it seems that this bug also affects Thunderbird, yet this bug is filed against Firefox. Will the patch also fix Thunderbird, or should I file a separate bug?

(In reply to comment #7) > Will the patch also fix Thunderbird, or should I file a separate bug? This patch will fix all users of the toolkit, Thunderbird included. No need to file a separate bug.

(In reply to comment #6) > The previous patch did affect the main toolbar when using Luna, this one > doesn't. Will I have to file yet another bug for getting the native appearance for all toolbar buttons back (even the ones of the navigation toolbar)? Your second patch will still leave the issues that * the bookmarks and history button won't have an obvious pressed state when the sidebar is open * extensions which copied the class attributes in order to get seamless appearance will be forced to provide a "hover" image and use the new drop-down style * drop-downs don't seem connected to the button any longer (see bug 347432) * and worst of all: the main toolbar feels overall less native (rather Mac-ish)

(In reply to comment #9) > (In reply to comment #6) > > The previous patch did affect the main toolbar when using Luna, this one > > doesn't. > > Will I have to file yet another bug for getting the native appearance for all > toolbar buttons back (even the ones of the navigation toolbar)? No, that can be dealt with here, too. I just wanted to get the changes made to toolkit reversed first , since I consider that to be most important.

Though perhaps if it is a more contentious issue it should be it's own bug. I'd like to see what mconnor thinks of this patch first, though.

*** Bug 347567 has been marked as a duplicate of this bug. ***

The last patch didn't fix the go button, this should be the one. I tested all of the main toolbar buttons and the go button, on Windows Classic and Luna.

*** Bug 347697 has been marked as a duplicate of this bug. ***

(In reply to bug 347692 comment #2) > bug 347416 will not effect this since the standard navigation toolbar items are > intentionally not having a native style. Where has that been decided? One of the criteria for the new theme listed at http://wiki.mozilla.org/FX2_Visual_Update/Default_Theme_Update was explicitly to "respect OS native look and feel" which that change nicely contradicts.

*** Bug 347802 has been marked as a duplicate of this bug. ***

CCing self. (In reply to comment #15) > (In reply to bug 347692 comment #2) > > bug 347416 will not effect this since the standard navigation toolbar items are > > intentionally not having a native style. I'm not sure what Mossop meant by 'not having a native style'. I can't believe that the idea is to purposely remove the button bevel, because that would make things look terrible.

Comparing the currently nightly to beta 1: - Navigation toolbar buttons have less padding, and have spaces between them instead, decreasing the clickable area horizontally and vertically - Navigation toolbar buttons and bookmarks toolbar buttons no longer have button bevel on hover state - Colour saturation in toolbar buttons is poor when not in hover state Let me know if any of these belong in a different bug

Comment on attachment 232370 [details] [diff] [review] real patch v2 On Linux this patch fixes the issue reported in bug 347447, but it regresses button bevel on hover. The old theme had it, and so does the new theme, but the new theme with this patch applied does not. Is that an intentional change?

(In reply to comment #19) > (From update of attachment 232370 [details] [diff] [review] [edit]) > On Linux this patch fixes the issue reported in bug 347447, but it regresses > button bevel on hover. The old theme had it, and so does the new theme, but > the new theme with this patch applied does not. Is that an intentional change? No, that wasn't intentional. I was trying to maintain the current look on windows (no border on hover) which is triggered by the "-moz-appearance: toolbarbutton" rule. I suppose I can just ifdef that for MOZ_WIDGET_GTK2. Ideally, we could leave the moz-appearance: toolbarbutton set on all platforms... Screenshots of how that looks using Windows XP Luna are at http://gavinsharp.com/tmp/hover-luna.png (ignore the Go button hover, that obviously is incorrect). I'll attach the patch with the ifdef, if you'd like to test on linux again, but if the styling from the screenshot looks OK perhaps we should just leave it on all platforms.

Myk (or anyone else), could you test this one on Linux?

Not sure I follow this. As I read this the new patch will leave a button bevel on hover under linux builds on the standard navigation toolbar items. As I understood it the new theme was not intended to have this button bevel so why should linux get it and not windows? The only reason that the button bevel was still there under linux as far as I can tell is that the -moz-appearance wasn't removed from the gnomestripe styles as it was from winstripe in the original theme patch. Was that intentional or just an oversight?

(In reply to comment #22) > The only reason that the button bevel was still there under linux as far as I > can tell is that the -moz-appearance wasn't removed from the gnomestripe styles > as it was from winstripe in the original theme patch. Was that intentional or > just an oversight? I was assuming that the new theme was tested on Linux, and that the removal from Winstripe but not Gnomestripe was intentional. Perhaps that's not the case...

Why would you want the Windows theme buttons not to have a bevel? This looks stupid.

(In reply to comment #24) > Why would you want the Windows theme buttons not to have a bevel? This looks > stupid. Someone else made that decision, not me. Current branch builds have no bevel effect. I'm just trying to fix the underlying bug while maintaining the status quo.

(In reply to comment #25) > Someone else made that decision, not me. To make the two different issues (all toolbar buttons vs. navigation toolbar buttons) easier to handle, I've filed bug 348442 for making the navigation toolbar look&feel native. Now Gavin's patch should be enough to fix this obvious bug and the decision which led to this bug in the first place can be revised in bug 348442.

Will this bug also solve the problem displayed in the screenshot? It is a comparison of the Firefox Menu Buttons extension both installed in the old Winstripe theme and in the new theme.

(In reply to comment #27) > Will this bug also solve the problem displayed in the screenshot? Probably not (unless that extension doesn't set the buttons' class to toolbarbutton-1). It could be argued that most extensions have to adapt their code for the new theme. OTOH bug 348442 (see comment #26 above) is about making this not happen.

(In reply to comment #27) > Will this bug also solve the problem displayed in the screenshot? > It is a comparison of the Firefox Menu Buttons extension both installed in the > old Winstripe theme and in the new theme. That is really more the aim of bug 348436 I think.

This doesn't mess with the native theming at all, except for the "Go" button (where the native styling gives an odd, misaligned border). This has the effect of re-adding the hover-bevel effect when using Luna, but doesn't affect the main toolbarbuttons with Classic (because of http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/themes/winstripe/browser/browser.css&rev=1.17.2.31&mark=240#239). It also fixes bug 348442. I'll post screenshots from Luna and Classic.

The mouse is over the home button in both these screenshots.

(In reply to comment #30) > It also fixes bug 348442. Not completely: the buttons are still missing some padding.

(In reply to comment #32) > (In reply to comment #30) > > It also fixes bug 348442. > > Not completely: the buttons are still missing some padding. Do you know how to fix that? I tried, but any padding I add is seemingly overridden by the -moz-appearance rule.

(In reply to comment #33) > Do you know how to fix that? I tried, but any padding I add is seemingly > overridden by the -moz-appearance rule. No, the padding is overridden by the !important rules added by the visual refresh (which shouldn't be there in the first place!). You'll have to go through the original changes ( http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=browser.css&branch=MOZILLA_1_8_BRANCH&root=/cvsroot&subdir=mozilla/browser/themes/winstripe/browser&command=DIFF_FRAMESET&rev1=1.17.2.27&rev2=1.17.2.28 ) and revert the changes related to padding - and probably those to border and margin as well, I'm afraid.

(In reply to comment #34) > You'll have to go through the original changes ( > http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=browser.css&branch=MOZILLA_1_8_BRANCH&root=/cvsroot&subdir=mozilla/browser/themes/winstripe/browser&command=DIFF_FRAMESET&rev1=1.17.2.27&rev2=1.17.2.28 > ) and revert the changes related to padding - and probably those to border and > margin as well, I'm afraid. That should probably be done in bug 348436/bug 348442, once this lands.

(In reply to comment #30) > This has the effect > of re-adding the hover-bevel effect when using Luna, but doesn't affect the > main toolbarbuttons with Classic (because of > http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/themes/winstripe/browser/browser.css&rev=1.17.2.31&mark=240#239). Sorry, I didn't quite understand - is removing the bevel on Classic intentional or is it covered in some other bug?

(In reply to comment #36) > Sorry, I didn't quite understand - is removing the bevel on Classic intentional > or is it covered in some other bug? I can only assume that the removal of classic hover-borders was intentional. Re-adding it would require removing the "border:0!important" rule that was added. This patch doesn't do that.

This fixes this bug, and bug 348442, and fixes the go button on linux. I've tested this on my own linux build, and on Windows with Classic and Luna.

On bug 348911 comment 4, Michael Wu claims the patch on this bug fixes that bug, which was caused by a bad patch on bug 347470.

Unbitrotted after the fix for bug 347412, and fix the back button hover when the button is disabled.

This is an updated version of "with ifdef for Linux". It restores the default toolbarbutton styling without affecting the main toolbarbuttons.

Comment on attachment 234159 [details] [diff] [review] more conservative fix let's get this before freeze, r+a=me on this theme only, branch only patch

I landed the conservative fix on the branch. That patch didn't fix bug 348442, the "better patch" here should probably be moved there, and updated as necessary. I'm not sure if it fixes bug 348911 or bug 347447, testing would be appreciated. mozilla/browser/themes/winstripe/browser/browser.css 1.17.2.37 mozilla/toolkit/themes/winstripe/global/toolbarbutton.css 1.7.2.4

Comment on attachment 234129 [details] [diff] [review] bitrot fix (and tweaks) clearing this for now, will discuss this later

b2 bits done, pushing out

open design issue -> beltzner

Bug 348442 is the open design issue, I consider this bug fixed.