547608 - make lightweight themes work properly on OS/2

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Walter Meinl]

User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9.2) Gecko/20100221 Firefox/3.6
Build Identifier: Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9.2) Gecko/20100221 Firefox/3.6

After a successful build of firefox-3.6 for OS/2 I was redirected to the site indicated above when starting the browser the first time. Hovering over the persona themes showed a quite incomplete picture, status bar and tab bar changed but not the menu and bookmarks bar. It can be reproduced also with recent trunk builds.
The reason is the parts of the winstripe theme were forked to pmstripe. menu.css and toolbar.css of pmstripe are missing the -moz-lwtheme bits that were introduced to the other platform themes. For the tabbar and the statusbar we use winstripe theme and therefore, these are aware of the lightweight themes.
One may say on OS/2 we don't need those fancy themes, but I'm sure people _will_ have a look at the personas. Moreover, it would make the same work to disable them completely (we 'd have to fork additional files from the winstripe theme) than to enable them completely.

Reproducible: Always

Comment 1

[Walter Meinl]

Attachment: patch

Comment 2

[Walter Meinl]

Comment on attachment 428498 [details] [diff] [review]
patch

A few explanations

>diff --git a/toolkit/themes/pmstripe/global/menu.css b/toolkit/themes/pmstripe/global/menu.css
>--- a/toolkit/themes/pmstripe/global/menu.css
>+++ b/toolkit/themes/pmstripe/global/menu.css
>@@ -47,34 +48,38 @@
> 
> menu,
> menuitem {

>-  background-color:  Menu;
This was introduced by Peter in order to have menu bar completely colored as defined from the Scheme Palette. However, the lightweight theme wants to use this colors as well, which results in a colored area on top of the theme.
I tried to exclude the coloring of the menu background from lightweight themes but not from the default theme with the code below.
>+}
>+
>+menu:not(:-moz-lwtheme),
>+menuitem:not(:-moz-lwtheme) {
>+  background-color:  menu;
> }
These bits are converted from the winstripe theme
>+}
>+
>+menubar > menu:-moz-lwtheme {
>+  -moz-appearance: none;
>+  border-style: none;
>+}
>+
>+menubar > menu:-moz-lwtheme:not([disabled="true"]) {
>+  color: inherit !important;
>+}
Unlike windows we do not automatically change the background color, when we hover with the mouse pointer over a menu, the color only changes, when one of the menuitems was pressed once.
>+
>+menubar > menu:-moz-lwtheme[_moz-menuactive="true"][open="true"] {
>+  background-color: Highlight;
>+  color: HighlightText !important;
>+  text-shadow: none;
> }
 
>diff --git a/toolkit/themes/pmstripe/global/toolbar.css b/toolkit/themes/pmstripe/global/toolbar.css
>--- a/toolkit/themes/pmstripe/global/toolbar.css
>+++ b/toolkit/themes/pmstripe/global/toolbar.css
>  * use your version of this file under the terms of the MPL, indicate your
>@@ -44,43 +45,58 @@
> @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> 
> /* ::::: toolbox ::::: */
> 
> toolbox {
>   -moz-appearance: toolbox;
>   border: none;
>   background-color: ThreeDFace;

>+}
>+
>+toolbox:not(:-moz-lwtheme) {
>   color: ButtonText;
> }
> 
 
> toolbar[type="menubar"], menubar {
>-  background-color: Menu;

This is part of the coloring as mentioned above
> 
>+toolbar[type="menubar"], menubar:not(:-moz-lwtheme) {
>+  background-color: Menu;
>+}
>+
> toolbar:first-child, menubar:first-child {
>   min-width: 1px; /* DON'T DELETE! */
>   border-top: none !important;

copied from winstripe

>+/* ::::: lightweight theme ::::: */
>+ 
>+toolbox:-moz-lwtheme,
>+toolbar:-moz-lwtheme {
>+  -moz-appearance: none;
>+  background: none;
>+  border-style: none;
> }

Comment 3

[Walter Meinl]

Attachment: removed unnecessary changes

during editing the patch I realized some unnecessary left-overs from earlier work
>toolbox:not(:-moz-lwtheme) {
>   color: ButtonText;
>}
this version of the patch removes these changes

Comment 4

[Walter Meinl]

Dave, you don't have to build trunk or 1.9.2 from scratch. You can use a recent trunk build, rename in the chrome directory classic.jar to *.zip, unzip classic.zip apply the patch to skin/classic/global/{menu,toolbar}.css, refresh classic.zip and copy classic.zip back to classic.jar. Then open the browser and change to the personas site to test it. When you apply a lightweight theme you can switch back to the default theme using the Add-ons manager.

Comment 5

[Walter Meinl]

Attachment: same patch dos line endings removed

sorry, just realized dos line endings in the menu.css part of the patch

Comment 6

[Walter Meinl]

Comment on attachment 428498 [details] [diff] [review]
patch

the first patch was ok, sorry for confusion should have looked at the diff before :-(

Comment 7

[Dave Yeo]

Comment on attachment 428498 [details] [diff] [review]
patch

Tested on trunk where it works very well.

Comment 8

[Walter Meinl]

(In reply to comment #7)
> (From update of attachment 428498 [details] [diff] [review])
> Tested on trunk where it works very well.

>+menubar > menu:-moz-lwtheme[_moz-menuactive="true"][open="true"] {
/*  background-color: Highlight; */
>+  color: HighlightText !important;

Dave, with the patch a lightweight theme would give a gray menu background. When you comment out that line with background-color in menu.css it will take the color of the OS/2 menu-background. Which one would you prefer?
For my taste, the OS/2 menu-background look a bit more 'native', but I would as well stick to the gray color, cause those lightweight themes don't stick to an OS/2 convention anyway.

Comment 9

[Dave Yeo]

(In reply to comment #8)
> 
> Dave, with the patch a lightweight theme would give a gray menu background.
> When you comment out that line with background-color in menu.css it will take
> the color of the OS/2 menu-background. Which one would you prefer?
> For my taste, the OS/2 menu-background look a bit more 'native', but I would as
> well stick to the gray color, cause those lightweight themes don't stick to an
> OS/2 convention anyway.

My menu-background colour is grey so commenting out that line makes no difference here :) I like the grey menu background and an alternative toolkit.jar (or chrome.jar depending on version) could always be posted somewhere for people to change to.

Comment 10

[Walter Meinl]

Attachment: better patch

Finally I disliked the menu background-color being grey while the background-color in the menus themselves is another one chosen by the default system theme. Moreover I found another quirk in the last patch: Dropdowns in the option menus were grey but should be white. Thus, this patch removes two lines in menu.css compared to the last one. But I think, now it should be ok.

Comment 11

[Dave Yeo]

Comment on attachment 429252 [details] [diff] [review]
better patch

After using the scheme palette to create a custom scheme without grey menus etc I have to agree that the background colour is better following the system default.
Once again looks good and works well on trunk.

Comment 12

[Walter Meinl]

Comment on attachment 429252 [details] [diff] [review]
better patch

Dao, seeking here for a 2nd review from a css expert, thanks

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/9ca6b7770658

Comment 14

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/33e3b6275b39

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Can someone using OS/2 verify that this bug is fixed for Firefox 3.6.2?