364620 - Misaligned entries in Searchbar's View dropdown menu

Status: NEW

(SeaMonkey :: Themes, defect)

Description

[Bruno 'Aqualon' Escherl]

There are a few issues with the View dropdown menu. With Classic theme, "Tags" and "Custom Views" are aligned to the right of the menu while all other entries are aligned to the left (on Mac this was fixed by bug 349991 according to Karsten). With Modern theme the line-height is too small for "Tags", "Custom Views" and the entries in both submenus. I'll attach screenshots for both cases.

Comment 1

[Bruno 'Aqualon' Escherl]

Attachment: Issue with Classic Theme

Comment 2

[Bruno 'Aqualon' Escherl]

Attachment: Issue with Modern theme

Comment 3

[Karsten Düsterloh]

Hm, now that i think of it, the Mac fix in bug 349991 was about a wrong font size; and indeed Mac Classic and Modern both show the same issue as shown here for XP Modern on trunk and SM1.1 branch...

Comment 4

[Stefan [:stefanh]]

In mac classic, the arrows are far to left as well. I don't know if the arrows should be that far to the left on other systems. Anyway, for mac classic I think this change caused it:
 menulist > menupopup > menuitem,
-.menulist-menupopup > menuitem {
+menulist > menupopup > menu,
+.menulist-menupopup > menuitem,
+.menulist-menupopup > menu {
   max-width: none;
   padding-right: 20px !important;
   font: message-box;
 }

Also, I don't think the styles rules assume that you can have a sub-menu in a menulist. Iirc menuitems in a menulist are always iconic and the height is affected of .menu-iconic-icon, .menu-iconic-left etc. The reason for the menuitems being iconic is that we provide a checkmark in the menuitems.

Actually, you should probably not have sub-menus in menulists since the selected menuitem will appear as the "default" one at the top. Selecting a menuitem from a sub-menu will then result in the sub-menu item appear as the default one. But the user won't see his/hers choice in the menu - it's hidden in the sub-menu.

Comment 5

[Stefan [:stefanh]]

Attachment: Branch fix for mac classic

This will fix the problem with "arrows too far left" on mac classic. We don't want the padding-right on menus in menulists. I made a branch patch, no need to fix it on trunk (going to change anyway).

Comment 6

[[e-mail]]

So what effect (if any) does this have on the Copies and Folders menulists (in Account Manager)? (The second menulist of each pair is a cascading menulist.)

Comment 7

[Stefan [:stefanh]]

Attachment: screenshot, non-patched vs patched

(In reply to comment #6)
> So what effect (if any) does this have on the Copies and Folders menulists (in
> Account Manager)? (The second menulist of each pair is a cascading menulist.)

I see what you mean (see screenshot). Probably better without my changes. What's worse is that menuitems in submenus doesn't have the same padding/margin as the menus... Iirc the reason they're not indented is that they're supposed to get their left margin/padding from the icon. Hmm, not good - and it might even be my fault :-/

Comment 8

[Stefan [:stefanh]]

> What's worse is that menuitems in submenus doesn't have the same padding/margin
> as the menus...

Well, I suppose I can do the same with menus in menulists as I do with menuitems. Will probably not have any time before New Year, though.

Comment 9

[Stefan [:stefanh]]

(In reply to comment #8)
> > What's worse is that menuitems in submenus doesn't have the same padding/margin
> > as the menus...
> 
> Well, I suppose I can do the same with menus in menulists as I do with
> menuitems. Will probably not have any time before New Year, though.
> 

Filed bug 365039 for that issue.

Comment 10

[Bruno 'Aqualon' Escherl]

Attachment: Fix the issue with Modern theme

Comment 11

[[e-mail]]

Comment on attachment 250027 [details] [diff] [review]
Fix the issue with Modern theme

The difference in height is caused by the fact that immediate menulist menupopup menuitem children are given the menuitem-iconic-noaccel binding so that they can display a checkmark (except in a couple of cases where they display custom icons). However the menus and their child menuitems have no such style; the folder pickers work around this by giving them all the appropriate iconic class.

Comment 12

[Bruno 'Aqualon' Escherl]

I don't have time to really think about this issue in the next weeks. So if somebody wants to take it, feel free to do it.

Comment 13

[Bruno 'Aqualon' Escherl]

I had another look at this issue. Is it correct, that I have to add the menuitem-iconic-noaccel binding to menulist > menupopup > menu > menupopup > menuitem in xul.css? That seems to fix the height of the menuitems in the tags and custom views submenus.

What I still haven't figured out is, how to properly fix the menu thing. When I add the menu-iconic binding to menulist > menupopup > menu the tags and custom views submenus are aligned correctly in Classic. So Classic would be fixed this way.

In Modern there are some issues left. First there's the new problem, that both submenus are aligned to the left of the dropdown menu, so the margin to the left compared to the other menuitems is missing. Should this be fixed with a css-Rule in menu.css or is menu-iconic the wrong binding?

The second issue is the height of the submenus. That's not fixed, even with the menu-iconic binding. When I set border: 1px solid transparent; in http://lxr.mozilla.org/seamonkey/source/themes/modern/global/menu.css#166 like it's done in Classic's windows/menu.css the submenus have the same height as in Classic, but that's still too small compared to the spacing between the menuitems (the same problem like in the View-Messages menu).

And the last showstopper for this patch, if adding the binding mentioned above is necessary, the change would have to be done in /toolkit/content/xul.css for suiterunner (assumed I didn't make a fals assumption here). Is there a chance at all to get such a change into toolkit's xul.css?

Comment 14

[Stefan [:stefanh]]

If the folder pickers are ok, can't you do the same here (adding the appropiate class)?

Comment 15

[Bruno 'Aqualon' Escherl]

(In reply to comment #14)
> If the folder pickers are ok, can't you do the same here (adding the appropiate
> class)?
I don't know, I'm still trying to figure out, how that's working...

But it's probably a better idea than what I thought about.

Comment 16

[Stefan [:stefanh]]

Hmm, looking at msgViewPickerOverlay.js#348 those menuitems already seem to be menuitem-iconic ones.

Comment 17

[Bruno 'Aqualon' Escherl]

Attachment: Add missing bindings to menu and menuitem

The patch does, what Stefan forsaw in comment #16 ;)

Classic seems fine so far, Modern still needs some work. Do we want to have the same View dropdown menu as in Classic (I have a nearly finished patch for that) or should it stay the way it's now (with problems like e.g. All stays selected even if a tag was selected afterwards)?

Comment 18

[Bruno 'Aqualon' Escherl]

Comment on attachment 250396 [details] [diff] [review]
Add missing bindings to menu and menuitem

I found some issues with this patch, new version tomorrow.

Comment 19

[[e-mail]]

Comment on attachment 250396 [details] [diff] [review]
Add missing bindings to menu and menuitem

>+    menuitem.setAttribute("class", "menuitem-iconic-accel");    
The binding might be menuitem-iconic-accel but I thought the class was simply menuitem-iconic?