Closed Bug 314524 Opened 14 years ago Closed 14 years ago

Menu Bar highlight text color wrong ( menu.css problem )

Categories

(Core Graveyard :: Widget: OS/2, defect)

x86
OS/2
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: mikus, Assigned: mozilla)

References

Details

(Keywords: regression)

Attachments

(3 files)

User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9a1) Gecko/20051020 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9a1) Gecko/20051020 Firefox/1.6a1

When the cursor passes over (or clicks on) an entry in the Menu Bar, the background color of that entry does not change -- but the text color of that entry changes to white.  This makes it hard to read on a light background.

Reproducible: Always

Steps to Reproduce:
1. position the cursor over an entry in the Menu Bar
Actual Results:  
The menu bar entry text chaged to white.  This is inappropriate with the customized colors on my desktop.

Expected Results:  
Up to the beginning of October 2005, the menu bar entry was displayed with a text color and a background color appropriate to a "highlighted" entry.

I was using he Classic theme.

The problem is that line 127 in toolkit/themes/winstripe/global/menu.css is

  color: -moz-MenuBarHoverText;

however, '-moz-MenuBarHoverText' seems to be defined NOWHERE in the trunk.  Apparently, an undefined color defaults STATICALLY to "white".


My bypass was to replace that line with

  background-color: Highlight;
  color: HighlightText;

that restored the previous Menu Bar entry ighlight behavior.

.
Summary: Menu Bar hilite text color wrong → Menu Bar hilite text color wrong ( menu.css problem )
Version: unspecified → Trunk
One of the patches in bug 243078 seem to have caused this.
Blocks: 243078
It's defined in the Windows look-and-feel code, as it should be. You can't use the Windows-only theme and not the Windows look-and-feel...
OS/2 tends to use Windows bits wherever possible.
It just needs someone to sync up the look & feel code.
Summary: Menu Bar hilite text color wrong ( menu.css problem ) → Menu Bar highlight text color wrong ( menu.css problem )
Are you saying that the background color is now also wrong, or just the foreground color?  If it's the latter, I have a quick patch to fix this, but if it's the former it may be necessary to define another Mozilla-specific system color for the menubar hover background, -moz-MenuBarHover / eColor__moz_menubarhover, and put it in Winstripe and take care of it in the necessary themes.  This also might help fix menus on Windows 95, if it ever actually came to that (detecting Win95 might be cumbersome, though).

On that note, what other OSes use Winstripe (if any)?  It'd be nice to fix all the problems like this at once.
Assignee: nobody → pythonesque+bugzilla
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: General → Widget: OS/2
Product: Firefox → Core
As I understand it, in this case only the foreground is wrong (CSS-wise), since the extra color was added only to Winstripe and only for the foreground.

What'll happen with the native theming work I'm doing I don't know, and really can't predict right now.
(In reply to comment #5)
> As I understand it, in this case only the foreground is wrong (CSS-wise), since
> the extra color was added only to Winstripe and only for the foreground.
> 
> What'll happen with the native theming work I'm doing I don't know, and really
> can't predict right now.
> 
I
 would agree with you, except that the change included:

>  menubar > menu[_moz-menuactive="true"] {
> -  padding: 1px 3px 1px 2px !important;
> -  background-color : Highlight !important;
> -  color: HighlightText !important;

There were changes made to deal with this in the windows native theming code, but these don't affect OS/2 (which as far as I can tell doesn't do native theming), so it could be that it's simply lost this behavior.  The initial bug report also seems to be restoring this background color.  If we don't want to add back the background-color: Highlight line, and it needs to be restored to its original behavior, the simplest solution here would be to define a new color for this situation.

Please tell me if I'm missing anything or there's an obvious alternative.
TBH, I don't really care what you do with the current code in trunk CVS - I specifically said I didn't want it checked in.

I cannot tell you what will ultimately be needed to fix it after the theming work (the stuff that landed on trunk should not have done so and does not count) I'm doing currently as I have not finished this part of it. The only this I do know is this: the CSS in the winstripe theme will visually look like a normal Windows XP Classic appearance if no native theming is used (which will probably be what you get on OS/2).

Until I actually have that ready (bug 313388), I can't help you here. What's worse, is that any changes made to it now will only make my life harder, as they'll cause conflicts in my CVS tree, and even after that, will be overriten by my work anyway.
(In reply to comment #5)
> As I understand it, in this case only the foreground is wrong (CSS-wise), since
> the extra color was added only to Winstripe and only for the foreground.
> 

WRONG.  [I find a general problem with bugzilla, in that it looks at problems from the developer's point rather than from the user's point.]

For me as a user, there occurred a REGRESSION.  It used to be that on a flyover, BOTH the Menu Bar entry background color AND the entry text color changed in my system.  (I have customized my desktop colors - changes in behavior are more noticeable). 

YES (developer's view) - because a problem line was ADDED to the menu.css file, the extra color was added only to the foreground.

but NO (user's view) - somewhere something was REMOVED (e.g., maybe from the menu.css file) such that whereas __before__ the background was affected by a flyover, __after__ the changes (PLURAL!) the background no longer appears to be affected.

If the foreground color change is fixed, but the background were now to be left unchanging:  (1) the visual effect of a flyover might be less noticeable to the user,  and (2) whatever color gets used for the text, it might be inappropriate to the background color specified by the user in his operating system settings.
Please, Mikus, you are NOT helping.

Bugzilla is a tool for DEVELOPERS, not users!

Unless you can make decent technical comments about what is wrong, or what needs to be fixed, please do not comment unless specifically asked. It is precicely because of such useless comments that I filed extra bugs from bug 243078, so I could fix things without the useless comments. We thank you for pointing out a regression (if people had listened to ME this bug would never have existed!), we know what is wrong, and we know a number of ways of fixing it.

The current plan, at least from my side, is to continue my work in bug 313388, which will fix this bug, among others. Whether someone else wants to fix this bug specifically in the meantime or not is up to them.
No need to shout, methinks.  Anyway, yes, I recognize your problem, which as I feared is because (as far as I can tell) OS/2 is themed more like Windows 95 than Windows Classic.

There's no real (correct) way to make a theme behave that way at the moment, OS/2 doesn't actually have native theming for anything like this, and I don't want to touch anything until James has uploaded his 'fix things on Windows without themes' patch anyway.  So don't expect to see much progress on this for awhile.  I apologize for the inconvenience.
Since 
Keywords: regression
(In reply to comment #3)
> OS/2 tends to use Windows bits wherever possible.
> It just needs someone to sync up the look & feel code.

From what Mikus found out that would mean that we just need to add the branch that does
   case eColor__moz_menubarhovertext:
      idx = COLOR_MENUTEXT;
      break;                                                                    
to widget/src/os2/nsLookAndFeel.cpp, right? Let me give that a try (altough it's hard to see the result as I cannot reproduce Mikus' color setup).
Not necessarily. The changes included removing the background colour, so (depending on other things) that may need to be put back as well. I don't actually have any way to know if it is a problem.
Though I know Mikus customized his colors, the background-color:Highlight didn't exist until the checkin for bug 303806; James's patch restored the original OS/2 menubar, except for the text color issue.  If that's the correct behavior, the patch that Peter suggested would fix the problem.  Since any fix really depends on OS/2's default menubar behavior, would some OS/2 user please tell me the behavior in other applications?
This shows the behavior of menu colors on OS/2:
- On the left, standard menus of "real" OS/2 applications
- In the middle, SeaMonkey colors.
  Mozilla apps never imported the real background color of 
  the menubar, it is just that the defaults for both are grey.
  It also never honored the color of the bar marking the active
  menu item.
- On the right, the current Firefox behavior.
  It seems better than in September but the color of the
  highlighted item in the main menubar is white.
OK, the "fix" for the white text in the active main menuitem. Comment #12 almost worked, just that the colors have different macros on OS/2. This would make it work like the middle example in attachment 201671 [details]. Before I ask for reviews, let me see if it is possible to make it use the standard OS/2 colors for the active menu item and the menu bar, too.
The fix you have there is the same fix that I created initially.  This is fine, and could probably go onto trunk (James, I don't think your patches touch OS/2, so it hopefully won't cause any conflicts).  Making the menubar backgrounds work properly, though, is a bit trickier, as I outlined in comment #4.  Such a fix probably shouldn't be attempted until James is done with his patch for bug 313388;  I'm marking that as a dependency for getting this bug truly fixed.

Active menu item sounds quite doable, though, and even desirable.  Feel free to attach a patch that does that as well.
Blocks: 313388
No longer blocks: 313388
Depends on: 313388
Yeah, that fix looks good. That will at least restore the previous behaviour, I believe, which is good enough for now.

You may want to file another bug for making the whole menubar act right on OS/2, as I suspect that will be harder. It would either need to use #ifdefs in the winstripe theme, or implement native theming for the menubar, I'd guess (the thin raised border is the correct behaviour for winstripe on Windows).
Comment on attachment 201682 [details] [diff] [review]
Fix to restore previous OS/2 behavior

OK, so let's review this one and get it into the trunk.

(As suggested I just filed bug 314843 for further work on this issue.)
Attachment #201682 - Flags: superreview?(mozilla)
Attachment #201682 - Flags: review?(mozilla)
Comment on attachment 201682 [details] [diff] [review]
Fix to restore previous OS/2 behavior

Is this a problem in 1.8?
Attachment #201682 - Flags: superreview?(mozilla)
Attachment #201682 - Flags: superreview+
Attachment #201682 - Flags: review?(mozilla)
Attachment #201682 - Flags: review+
Assignee: pythonesque+bugzilla → mozilla
(In reply to comment #20)
> (From update of attachment 201682 [details] [diff] [review] [edit])
> Is this a problem in 1.8?
> 
This is fallout from the unfortunate trunk checkin for bug 243078, so 'tis not.
This fix checked in. The rest should be handled in the other bug.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Something went wrong on checkin. Could someone please check in this build bustage fix?
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.