398610 - OS/2 meta bug follow-ups of UI changes

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Walter Meinl]

User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9a9pre) Gecko/2007100421 Minefield/3.0a9pre
Build Identifier: Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9a9pre) Gecko/2007100421 Minefield/3.0a9pre

Currently a lot of polishing starts as FF-3.0 comes closer. Not every checkin for winstripe is usable/visible for OS/2. Here patches for css files in toolkit/pmstripe/global/ should be posted and discussed if they are needed/useful for the UI on OS/2. This bug should beleft open and every issue preferably with patch simply added until the final version is released (or the bug becomes too crowded ;-)

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Walter Meinl]

Attachment: follow-up to bug279703

see http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=popup.css&branch=&root=/cvsroot&subdir=/mozilla/toolkit/themes/winstripe/global&command=DIFF_FRAMESET&rev1=1.9&rev2=1.10
from http://mxr.mozilla.org/mozilla/source/toolkit/content/xul.css#328 popup is deprecated now panel should be used.
To be honest, I don't see a difference in behaviour (hoped that would help against the z-index problem but it doesn't). Therefore, I don't go for review now

Comment 2

[Walter Meinl]

Attachment: [checked in] follow-up to bug363130

This helps against icons in the bookmark menu or bookmark bar to be displayed bigger than 16x16 (which some do after the movements in bug363130). When I use exactly the same patch as for winstripe/global/menu.css our menu-check mark grows to 16px (that's not so good looking ;-). Therefore the max- prefix. Alternatively, one could enter a fixed value fore the size of menu-check.png

Comment 3

[Peter Weilbacher]

Comment on attachment 283609 [details] [diff] [review]
[checked in] follow-up to bug363130

The patch seems alright, but I cannot reproduce the problem that this is fixing with the URL given in bug 363130 (http://www.idealo.de/). Do I have to do anything special?

Comment 4

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

(In reply to comment #3)
> (From update of attachment 283609 [details] [diff] [review])
> The patch seems alright, but I cannot reproduce the problem that this is fixing
> with the URL given in bug 363130 (http://www.idealo.de/). Do I have to do
> anything special?

That site's changed their icons. You can use the engine at http://mycroft.mozdev.org/download.html?name=btjunkie&opensearch=yes&skipcache=yes&submitform=Search for the moment, I think, but any engine with an icon that's larger than 16x16px should work.

Comment 5

[Walter Meinl]

(In reply to comment #3)
> (From update of attachment 283609 [details] [diff] [review])
> The patch seems alright, but I cannot reproduce the problem that this is fixing
> with the URL given in bug 363130 (http://www.idealo.de/). Do I have to do
> anything special?
> 
Actually, I didn t see the searchbar problem (and shouldn t this be fixed by now?) but I had some big icons in my bookmarks-menu and -bar. 
here a link save it to your bookmarks
http://www.kuebelpflanzeninfo.de/exot/mangobaum.htm
the size restriction was in browser.css before (as I could find in the patch of that bug) menu.css for pinstripe, winstripe even gnomestripe were changed accordingly.

Comment 6

[Peter Weilbacher]

Comment on attachment 283609 [details] [diff] [review]
[checked in] follow-up to bug363130

OK, yes, this patch helps indeed. Thanks for the links.

Comment 7

[Peter Weilbacher]

Comment on attachment 283609 [details] [diff] [review]
[checked in] follow-up to bug363130

OK, checked this patch into trunk.

Comment 8

[Walter Meinl]

Comment on attachment 283607 [details] [diff] [review]
follow-up to bug279703

We should probably check this follow up in to keep in-line with the other platforms. See also comment#1. I have the patched file in my tree since several months and didn't see a regression

Comment 9

[Walter Meinl]

New small glitch: Open the places "Library". Press any of the menu styled "Organize" "View" or "Import and Backup" fields. They get the background color of a menu but the letters stay black. I guess
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=organizer.css&branch=&root=/cvsroot&subdir=/mozilla/browser/themes/winstripe/browser/places&command=DIFF_FRAMESET&rev1=1.2&rev2=1.3
could be the relevant checkin.

Comment 10

[Peter Weilbacher]

Comment on attachment 283607 [details] [diff] [review]
follow-up to bug279703

It doesn't really make sense to add panel to both sections.

The new bookmark panel (the one that opens when clicking on the URL bar star) is such a panel. I think it looks nicer when adding it to the popup so that it gets the 2px top border.

Btw, should we not be able to merge the two blocks into one and add a small new block just for the two lines of 1px top border of menupopups?

Comment 11

[Walter Meinl]

Attachment: 2nd try follow-up bug279703

(In reply to comment #10)
> (From update of attachment 283607 [details] [diff] [review])
> It doesn't really make sense to add panel to both sections.
> 
> The new bookmark panel (the one that opens when clicking on the URL bar star)
> is such a panel. I think it looks nicer when adding it to the popup so that it
> gets the 2px top border.
Ok, slowly I understand, what this is about ;-)
> 
> Btw, should we not be able to merge the two blocks into one and add a small new
> block just for the two lines of 1px top border of menupopups?
> 
I merged it, and so far it seems to work.
What I still don't like is that the top border of the add-bookmark panel with this patch comes in line with the top border of the lower location bar. (IIRC that is however the same in windows)

Comment 12

[Peter Weilbacher]

Comment on attachment 310105 [details] [diff] [review]
2nd try follow-up bug279703

>-menupopup {
>+menupopup,
>+popup,
>+panel {
>   -moz-appearance: menupopup;
>   border: 2px solid;
>-  border-top: 1px solid;
>   -moz-border-top-colors: ThreeDHighlight;

I think this should be used only for the 1px border case. So use this one

>-  -moz-border-top-colors: grey ThreeDHighlight;

here, and the one with only ThreeDHighlight for the new menupopup section. Gives a small but noticeable difference.

Comment 13

[Walter Meinl]

Attachment: [checked in] wipe tomatos from my eye

> 
> I think this should be used only for the 1px border case. So use this one
> 
> >-  -moz-border-top-colors: grey ThreeDHighlight;
> 
> here, and the one with only ThreeDHighlight for the new menupopup section.
Your're right, I've should have noticed the difference in the wiped-out code. This looks nice now

Comment 14

[Walter Meinl]

(In reply to comment #9)
> New small glitch: Open the places "Library". Press any of the menu styled
> "Organize" "View" or "Import and Backup" fields. They get the background color
> of a menu but the letters stay black. I guess
> http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=organizer.css&branch=&root=/cvsroot&subdir=/mozilla/browser/themes/winstripe/browser/places&command=DIFF_FRAMESET&rev1=1.2&rev2=1.3
> could be the relevant checkin.
> 

Ok these should have the look like the toplevel in bookmarks, there should not be a highlight on the toolbar and the text color should not change.
I cannot change the assignee. Peter, could you please assign it to me?

Comment 15

[Peter Weilbacher]

Comment on attachment 310316 [details] [diff] [review]
[checked in] wipe tomatos from my eye

Checked this in to trunk.

Comment 16

[Peter Weilbacher]

(In reply to comment #14)
> Peter, could you please assign it to me?

Hmm, you don't have a check-box that says "take bug" when you upload an attachment? I didn't know that one needs privileges to get that.

Comment 17

[Walter Meinl]

(In reply to comment #16)
> (In reply to comment #14)
> > Peter, could you please assign it to me?
> 
> Hmm, you don't have a check-box that says "take bug" when you upload an
> attachment? I didn't know that one needs privileges to get that.
> 
Yeah, I don't see this checkbox. I can change priority and target milestones and such things but not assigned to
(In reply to comment #14)
> > New small glitch: Open the places "Library". Press any of the menu styled
> > "Organize" "View" or "Import and Backup" fields. They get the background color
> > of a menu but the letters stay black. I guess
> > http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=organizer.css&branch=&root=/cvsroot&subdir=/mozilla/browser/themes/winstripe/browser/places&command=DIFF_FRAMESET&rev1=1.2&rev2=1.3
> > could be the relevant checkin.
> > 
> 
> Ok these should have the look like the toplevel in bookmarks, there should not
> be a highlight on the toolbar and the text color should not change.
looking deeper at the patch for bug400703 not the organizer.css changes seem to be crucial but http://bonsai.mozilla.org/cvsview2.cgi?command=DIFF&subdir=%2Fmozilla%2Fbrowser%2Fcomponents%2Fplaces%2Fcontent&file=places.xul&rev1=1.104&rev2=1.105&whitespace_mode=ignore&diff_mode=context (removed a bunch of whitespace fixes) where the places bar was changed from separate toolbarbuttons to menus in a menubar
<menubar id="placesMenu" chromedir="&locale.dir;">                        
<menu accesskey="&organize.accesskey;" class="menu-iconic"
Obviously on native themes that makes no visible difference but on OS/2 it does.

Comment 18

[Walter Meinl]

Attachment: [checked in] follow-up of https://bugzilla.mozilla.org/attachment.cgi?id=283609 (finetuning of menu text position)

This patch is in part a correction to https://bugzilla.mozilla.org/attachment.cgi?id=283609. In the first hunk we have to move the start of non-iconic menu entries 1px to the right (open e.g. the bookmarks or history menu and you'll see that the non-iconic entries are 1px shifted to the left compared to the iconic entries). The second part is correcting a problem with the new dropdown panel for the unified forward/back button in firefox. When this list contains an URL without icon and one hovers over the entries the menubar seems to 'jump'. Moreover, when the list entries are short, and one hovers over the entry without icon the menubar expands the whole panel to the right. Testcase for this start,  ff, load os2.org (with favicon), load netlabs.org (no favicon) and again os2.org. Then dropdown the list and move the mouse over it. The panel expands to the right when hovering over netlabs.org

Comment 19

[Walter Meinl]

Attachment: Should we make the new forward back arrows look more OS/2 alike?

Peter, I attached a screenshot how we could make the new forward / back arrows look more OS/2 alike. I mirrored and then changed the black/white borders of the Menu-arrow.png from toolkit\themes\pmstripe\global\menu. We would need to patch browser/themes/winstripe/browser/browser.css with %ifdef XP_OS2... I'm having a patch for it, but wanted to get your opinion about it.

Comment 20

[Peter Weilbacher]

Comment on attachment 311375 [details]
Should we make the new forward back arrows look more OS/2 alike?

I have to say that I find it rather confusing to see this arrow on the left of the menu. As this is not a standard OS/2 control I don't think we have to do it. (And I don't think it's a good idea to start ifdefing in the winstripe subdirectory.)

Comment 21

[Peter Weilbacher]

Comment on attachment 311371 [details] [diff] [review]
[checked in] follow-up of https://bugzilla.mozilla.org/attachment.cgi?id=283609 (finetuning of menu text position)

Yes, this seems OK to me. If you want this in beta 5 I think you should request 1.9b5 approval, otherwise I hope I will think about checking it in after the freeze (otherwise please remind me).

Comment 22

[Walter Meinl]

Comment on attachment 311371 [details] [diff] [review]
[checked in] follow-up of https://bugzilla.mozilla.org/attachment.cgi?id=283609 (finetuning of menu text position)

This is OS/2 only in an OS/2 only directory, doesn't affect the main build platforms. The risk (even for OS/2) should be closest to zero

Comment 23

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

Comment on attachment 311371 [details] [diff] [review]
[checked in] follow-up of https://bugzilla.mozilla.org/attachment.cgi?id=283609 (finetuning of menu text position)

beta 5 is closed, moving approval flag ...

Comment 24

[Peter Weilbacher]

Comment on attachment 311371 [details] [diff] [review]
[checked in] follow-up of https://bugzilla.mozilla.org/attachment.cgi?id=283609 (finetuning of menu text position)

I don't think we have to wait for approvals in  OS/2-only (=NPOTB) files during non-freeze periods. Checked into trunk.

Comment 25

[Walter Meinl]

Comment on attachment 311375 [details]
Should we make the new forward back arrows look more OS/2 alike?

(In reply to comment #20)
> (From update of attachment 311375 [details])
> I have to say that I find it rather confusing to see this arrow on the left of
> the menu. As this is not a standard OS/2 control I don't think we have to do
> it. (And I don't think it's a good idea to start ifdefing in the winstripe
> subdirectory.)
> 
This came just to my mind when puzzling at the menu things. I was myself not 100% convinced (for the reasons you mentioned) but thought it would be good to get another oppinion

Comment 26

[Walter Meinl]

(In reply to comment #14)
> > > New small glitch: Open the places "Library". Press any of the menu styled
> > > "Organize" "View" or "Import and Backup" fields. They get the background color
> > > of a menu but the letters stay black. I guess
> > > http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=organizer.css&branch=&root=/cvsroot&subdir=/mozilla/browser/themes/winstripe/browser/places&command=DIFF_FRAMESET&rev1=1.2&rev2=1.3
> > > could be the relevant checkin.
> > > 
> > 
> > Ok these should have the look like the toplevel in bookmarks, there should not
> > be a highlight on the toolbar and the text color should not change.
> looking deeper at the patch for bug400703 not the organizer.css changes seem to
#placesMenu > menu {                                                            
  -moz-padding-start: 4px;                                                      
  -moz-appearance: toolbarbutton;                                               
  color: -moz-DialogText;                                                       
}                                                                               
So, the menu should have the appearance of a toolbarbutton, this works on windows (xp and vista) but not on OS/2. I played around with moz-apperance: whatever I entered after the colon wasn't respected.
Any ideas why this don't work are appreciated

Comment 27

[Peter Weilbacher]

Could it be that most or all of the -moz-appearance properties are ignored on OS/2 because we never implemented nsNativeThemeOS2? At some point I understood how these things worked but now I don't find it in the code any more.

Comment 28

[Walter Meinl]

(In reply to comment #27)
> Could it be that most or all of the -moz-appearance properties are ignored on
> OS/2 because we never implemented nsNativeThemeOS2? 
That's what I suspected, but wasn't sure about. -moz-appearance is also ignored when you start WindowsXP with the classic theme. In classic theme under XP the main menus of the "library" don't look like buttons. So, I'm afraid we can't do anything else than hoping that there's gonna be a change in organizer.css.

Comment 29

[Peter Weilbacher]

I still haven't even looked at the places menu thing or organizer.css... 
Reminder to myself!

Two other things, though: I looked at bug 426732 and I think we need to do something about the OS/2 colors, too, like adding a few lines in nsLookAndFeel.cpp. Not sure if bug 427979 also calls for a similar solution for OS/2.

Comment 30

[Walter Meinl]

(In reply to comment #29)
> Not sure if bug 427979 also calls for a similar solution for
> OS/2.
> 
Today bug429188 was checked in, that changed some of the checkins of bug 427979 and implemented something in http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/widget/src/os2&command=DIFF_FRAMESET&root=/cvsroot&file=nsLookAndFeel.cpp&rev1=1.45&rev2=1.46
haven't built yet but maybe that's already what you're looking for (or something to start)

Comment 31

[Walter Meinl]

(In reply to comment #30)
> (In reply to comment #29)
> > Not sure if bug 427979 also calls for a similar solution for
> > OS/2.
> > 
> Today bug429188 was checked in, that changed some of the checkins of bug 427979
> and implemented something in widget/src/os2/nsLookAndFeel.cpp
> but maybe that's already what you're looking for (or something to start)
> 
Built it and in principle this is indeed implemented. The only consumer so far is the downloadmanager. But it doesn't look different after the checkin for two reasons: first ecolor_moz_eventreerow and ecolor_moz_oddtreerow have the same value (SYSCLR_ENTRYFIELD). Secondly and more important before bug 429188 was checked in it was decided that winstripe (windows) shouldn't use alternate colors in downloadmanager
toolkit/themes/winstripe/mozapps/downloads/downloads.css
1.30 reed%reedloden.com 2008-04-04 02:06 Bug 426007 - "Remove alternating row colors in the download manager"
For testing if the checkin in nsLookAndFeel.cpp would work in principle I moved ecolor_moz_oddtreerow to have SYSCLR_MENU and reimplemented the alternate backgroundcolors in downloadmanager.css and it shows now for odd rows a grey background. So, as said in principle it would work but no consumer at the moment and if we really want to have different even and odd row colors we would have to move ecolor_moz_oddtreerow to another place in nsLookAndFeel.cpp

Comment 32

[Walter Meinl]

(In reply to comment #29)
> 
> I looked at bug 426732 and I think we need to do
> something about the OS/2 colors, too, like adding a few lines in
> nsLookAndFeel.cpp.
> 
I guess there is no native coloro for hyperlinks or an equivalent for COLOR_HOTLIGHT (used on windows) in OS/2, so we'd have to hardcode a color like mac does (aColor = NS_RGB(0x14,0x4F,0xAE);). Or what are you thinking?

Comment 33

[Peter Weilbacher]

For the link color I'm pretty happy with what the default is now (#0000ef), perhaps we should just hardcode that. The darker blue that you quoted doesn't fit as nicely on OS/2 as it probably does for MacOSX.

As for the download manager banding I'm not sure what to do. Just using a completely different color property because it gives some grayish value seems wrong to me. Is there any other (native/PM) OS/2 app that does banding somehow? I can't think of any, if you can't, either, then let's forget about it.

Comment 34

[Walter Meinl]

(In reply to comment #33)
> For the link color I'm pretty happy with what the default is now (#0000ef),
> perhaps we should just hardcode that. The darker blue that you quoted doesn't
> fit as nicely on OS/2 as it probably does for MacOSX.
>
This should just be an example, how other platforms than windows do. I would guess as long as we do not experience weird colors in hyperlinks, we should probably leave it as is, a patch for hardcoding the link color would be relatively easy to apply in case, another UI change will result in bad coloring of hyperlinks.

> As for the download manager banding I'm not sure what to do. Just using a
> completely different color property because it gives some grayish value seems
> wrong to me. Is there any other (native/PM) OS/2 app that does banding somehow?
> I can't think of any, if you can't, either, then let's forget about it.
> 
With regard to the grayish color, that was as said only a test, if the new variables do work for OS/2, and they do. So if ever we need to change something, we know now, that we only need a change in our nsLookAndFeel.cpp and wouldn't have to change other files (like for gtk in bug 427979).
With regard to the downloadmanager I just rebuilt again, now there is a faint grey band between the individual downloads and it looks quite ok (and its the same on windows), so for the moment we can let it as it is.

Comment 35

[Walter Meinl]

(In reply to comment #29)
> I still haven't even looked at the places menu thing or organizer.css... 
> Reminder to myself!
> 

(In reply to comment #28)
> when you start WindowsXP with the classic theme. In classic theme under XP the
> main menus of the "library" don't look like buttons. So, I'm afraid we can't do
> anything else than hoping that there's gonna be a change in organizer.css.
> 

As there were problems with the classic theme on windows, too, they checked in Bug 421529 - Menu items in bookmarks manager don't get a hovered look anymore with classic theme.
For us, now the button-like appearance is back again, when the menu is inactive and you hover over it. The only remainder that this is defined as a menu is the bold text and when you press such a menu you get the menu background color. So, it has gotten better, maybe not optimal but as we would have to do sth in a browser/winstripe css file, I think we should leave it for the moment as is, probably there will be other changes in the near future.

Comment 36

[Peter Weilbacher]

Attachment: organizer buttons to use menu colors on OS/2

This fixes the menu colors on OS/2 when the organizer menus are open.
As the file is already preprocessed I guess we can add this ifdef.

Comment 37

[Peter Weilbacher]

Oops, Walter, I only saw your comment after I attached the patch, I haven't CVS updated to the latest sources yet in the tree I was working with. Does the patch still work?

Comment 38

[Peter Weilbacher]

Attachment: organizer buttons to use menu colors on OS/2, updated

OK, confirmed that it is still necessary, updated for bitrot.

Comment 39

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

Comment on attachment 317865 [details] [diff] [review]
organizer buttons to use menu colors on OS/2, updated

>Index: browser/themes/winstripe/browser/places/organizer.css

> #placesMenu > menu {
>   -moz-padding-start: 4px;
>   -moz-padding-end: 1px;
>   padding-top: 2px;
>   padding-bottom: 2px;
>   -moz-appearance: toolbarbutton;
>+%ifdef XP_WIN
>   color: -moz-DialogText;
>+%endif
>   border: 1px solid transparent;
> }

I'm not sure I understand why this is needed - does OS/2's native style implementation have different semantics for "-moz-appearance: toolbarbutton" or "color: -moz-DialogText"? Either way, would be nice to add a preprocessor comment ("% " prefixed) explaining that this is a fix for OS/2.

Comment 40

[Peter Weilbacher]

Gavin, thanks. Yes, "-moz-appearance: toolbarbutton" has no effect for OS/2 because we never found time to implement nsNativeThemeOS2.

Comment 41

[Peter Weilbacher]

Attachment: [checked in] organizer buttons to use menu colors on OS/2, with comment

Comment added as suggested by Gavin, carrying over his r+. As this fix will only affect OS/2 there should be no risk for other platforms.

Comment 42

[Mike Connor [:mconnor]]

Comment on attachment 317914 [details] [diff] [review]
[checked in] organizer buttons to use menu colors on OS/2, with comment

a=mconnor on behalf of 1.9 drivers

Comment 43

[Ben Turner (not reading bugmail, use the needinfo flag!)]

All patches appear to have been checked in, resolving FIXED. Please reopen ASAP if more work needs to be done here.