Visual refresh doesn't natively style toolbar buttons anymore

RESOLVED FIXED in Firefox 2

Status

()

Firefox
Toolbars and Customization
--
major
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Simon Bünzli, Assigned: Gavin)

Tracking

({fixed1.8.1})

unspecified
Firefox 2
x86
Windows XP
fixed1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Fx2 theme change])

Attachments

(2 attachments, 8 obsolete attachments)

(Reporter)

Description

12 years ago
... 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.
Blocks: 347454
Severity: normal → major

Comment 2

12 years ago
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.
Created attachment 232336 [details] [diff] [review]
patch

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.
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #232336 - Flags: review?(mconnor)
*** Bug 347555 has been marked as a duplicate of this bug. ***
Created attachment 232351 [details] [diff] [review]
real patch

The previous patch did affect the main toolbar when using Luna, this one doesn't.
Attachment #232336 - Attachment is obsolete: true
Attachment #232351 - Flags: review?(mconnor)
Attachment #232336 - Flags: review?(mconnor)

Comment 7

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

Comment 9

12 years ago
(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. ***
Created attachment 232370 [details] [diff] [review]
real patch v2

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.
Attachment #232351 - Attachment is obsolete: true
Attachment #232370 - Flags: review?(mconnor)
Attachment #232351 - Flags: review?(mconnor)
*** Bug 347697 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 15

12 years ago
(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.

Updated

12 years ago
Blocks: 347447
*** Bug 347802 has been marked as a duplicate of this bug. ***

Comment 17

12 years ago
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.
Flags: blocking-firefox2?

Comment 18

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

Updated

12 years ago
Flags: blocking-firefox2? → blocking-firefox2+
Whiteboard: [Fx2 theme change]
Target Milestone: --- → Firefox 2
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.
Created attachment 233327 [details] [diff] [review]
with ifdef for Linux

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...

Comment 24

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

Updated

12 years ago
Blocks: 348442
(Reporter)

Comment 26

12 years ago
(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.
Created attachment 233373 [details]
Screenshot

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.
(Reporter)

Comment 28

12 years ago
(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.
Created attachment 233483 [details] [diff] [review]
don't mess with the native styling

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.
Attachment #232370 - Attachment is obsolete: true
Attachment #233327 - Attachment is obsolete: true
Attachment #233373 - Attachment is obsolete: true
Attachment #233483 - Flags: review?(mconnor)
Attachment #232370 - Flags: review?(mconnor)
Created attachment 233484 [details]
screenshot

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

Comment 32

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

Comment 34

12 years ago
(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.

Comment 36

12 years ago
(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.
Created attachment 234098 [details] [diff] [review]
better patch

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.
Attachment #233483 - Attachment is obsolete: true
Attachment #233484 - Attachment is obsolete: true
Attachment #234098 - Flags: review?(mconnor)
Attachment #233483 - Flags: review?(mconnor)

Comment 39

12 years ago
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.
Blocks: 348911
Created attachment 234129 [details] [diff] [review]
bitrot fix (and tweaks)

Unbitrotted after the fix for bug 347412, and fix the back button hover when the button is disabled.
Attachment #234098 - Attachment is obsolete: true
Attachment #234129 - Flags: review?(mconnor)
Attachment #234098 - Flags: review?(mconnor)
Target Milestone: Firefox 2 → Firefox 2 beta2
Created attachment 234159 [details] [diff] [review]
more conservative fix

This is an updated version of "with ifdef for Linux". It restores the default toolbarbutton styling without affecting the main toolbarbuttons.
Attachment #234159 - Flags: review?(mconnor)
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
Attachment #234159 - Flags: review?(mconnor)
Attachment #234159 - Flags: review+
Attachment #234159 - Flags: approval1.8.1+
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
Attachment #234129 - Flags: review?(mconnor)
b2 bits done, pushing out
Target Milestone: Firefox 2 beta2 → Firefox 2
open design issue -> beltzner
Assignee: gavin.sharp → beltzner
Status: ASSIGNED → NEW
Bug 348442 is the open design issue, I consider this bug fixed.
Assignee: beltzner → gavin.sharp
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.