593944 - No icons in alltabs menuitems

Status: RESOLVED FIXED

(SeaMonkey :: Themes, defect)

Description

[Stefan [:stefanh]]

Menuitems in the alltabs menupopup has room for icons - right now there's an empty space where the icons should be. Also, the font is a bit too small in mac classic.

This could all be fixed by some styling, I do wonder one thing, though:
--------------------------------------------------------------
1062                 <xul:menupopup class="menulist-menupopup tabs-alltabs-popup"
1063                                anonid="alltabs-popup"
1064                                position="after_end"/>
--------------------------------------------------------------

The "menulist-menupopup" class seems a bit redundant, since the binding gets overridden by the tabs-alltabs-popup binding. What we have here is:
1) -moz-binding: url("chrome://global/content/bindings/popup.xml#popup-scrollbars");
2) -moz-binding: url("chrome://messenger/content/tabmail.xml#tabmail-alltabs-popup");

I would suggest removing the "menulist-menupopup" class from the xul, that would also take care of the font issue.

Comment 1

[Stefan [:stefanh]]

Attachment: Screenshot, showing empty space and large checkmark

As you can see, the checkmark is also dimensioned for a larger font.

Comment 2

[Stefan [:stefanh]]

As we don't currently set any icon on the menuitem, I can see 2 ways here (before I talked to Neil I didn't saw the 2B option):

1) Be happy with a general icon for all menuitems
2) Set the same icon as we have on the tab, which means either:
 A: "menuItem.style.listStyleImage = getComputedStyle(aTabNode, null).getPropertyValue("list-style-image");" (or something like that)
B: Copy all the needed attributes so we can use the same css as we do for the .tabmail-tab (12 attributes and a third class).

Comment 3

[Stefan [:stefanh]]

Note: for 2A and 2B I'm talking about tabmail.xml (createTabMenuItem method). 

Also for removing the menulist-menupopup class mentioned in comment #0 (line 1062 in the tabmail-tabs binding).

Comment 4

[Stefan [:stefanh]]

Doing 2) will possibly require some icon work, since it looks like message/folder icons are not always vertically centered (and they have sometimes different size)

Comment 5

[Stefan [:stefanh]]

Attachment: WIP1

Comment 6

[Stefan [:stefanh]]

Attachment: Icons in menuitems

OK, so I think this is it. The 2 added observers seems reasonable to me, but could be considered nits, of course. I have made all affected icons 16x16 and also adjusted the position of the icons in order to make them line up better in the menu. I'd probably want to adjust the original 16x16 icons (folder ones) in a follow-up.

Comment 7

[[e-mail]]

Comment on attachment 475946 [details] [diff] [review]
Icons in menuitems

>+            attributes.forEach (
forEach is a function, not a keyword, so no space before the (

>+                if (aTabNode.hasAttribute(attribute))
>+                {
>+                  menuItem.setAttribute(attribute, aTabNode.getAttribute(attribute));
What if the tab node doesn't have that attribute?

> .tabmail-tab[type="folder"],
>+.alltabs-item[type="folder"],
This is annoying. Maybe we should have a class whose name is sufficiently generic to use for both a tabmail tab and an alltabs menuitem.

>diff --git a/suite/themes/classic/messenger/icons/message-mail-attach-offl.png b/suite/themes/classic/messenger/icons/message-mail-attach-offl.png
Can you describe the binary changes? (I don't have an easy way to revert a patch that changes images so I tried to avoid applying them.)

>-.tabmail-tab[type="message"] .tab-icon {
>-  margin-top: -4px;
This is because all the icons are now the same size?

>-/* the news icons are only 14px high, unfortunately */
>-.tabmail-tab[type="message"][MessageType="rss"] .tab-icon,
>-.tabmail-tab[type="message"][MessageType="nntp"] .tab-icon {
>-  height: 14px;
And presumably these also?

>diff --git a/suite/themes/modern/messenger/icons/message-junk-other.gif b/suite/themes/modern/messenger/icons/message-junk-other.gif
I didn't spot any CSS size changes, did I just scroll too quickly? Or what are the images changes for?

Comment 8

[Stefan [:stefanh]]

(In reply to comment #7)
> Comment on attachment 475946 [details] [diff] [review]
> Icons in menuitems
> 
> >+            attributes.forEach (
> forEach is a function, not a keyword, so no space before the (
> 
> >+                if (aTabNode.hasAttribute(attribute))
> >+                {
> >+                  menuItem.setAttribute(attribute, aTabNode.getAttribute(attribute));
> What if the tab node doesn't have that attribute?

Then it doesn't get set on the menuitem. That's what the 'if (aTabNode.hasAttribute(attribute))' is for.

> 
> > .tabmail-tab[type="folder"],
> >+.alltabs-item[type="folder"],
> This is annoying. Maybe we should have a class whose name is sufficiently
> generic to use for both a tabmail tab and an alltabs menuitem.

Hmm, yes, perhaps.

> >diff --git a/suite/themes/classic/messenger/icons/message-mail-attach-offl.png b/suite/themes/classic/messenger/icons/message-mail-attach-offl.png
> Can you describe the binary changes? (I don't have an easy way to revert a
> patch that changes images so I tried to avoid applying them.)
> 
> >-.tabmail-tab[type="message"] .tab-icon {
> >-  margin-top: -4px;
> This is because all the icons are now the same size?

Yes, well - I also adjsted the icons vertical position, so it's as hight up as it can be. There might be a slight difference from before since I think I miss 1-2px, so maybe I should add it back but with a different negative value.
 
> >-/* the news icons are only 14px high, unfortunately */
> >-.tabmail-tab[type="message"][MessageType="rss"] .tab-icon,
> >-.tabmail-tab[type="message"][MessageType="nntp"] .tab-icon {
> >-  height: 14px;
> And presumably these also?

Yes!

> 
> >diff --git a/suite/themes/modern/messenger/icons/message-junk-other.gif b/suite/themes/modern/messenger/icons/message-junk-other.gif
> I didn't spot any CSS size changes, did I just scroll too quickly? Or what are
> the images changes for?

That's a mistake, actually - I made it 16x16 even though it's not used in the menu.

Comment 9

[Stefan [:stefanh]]

Attachment: Something like this?

Regarding the class for the icons, I don't think we can get by that without using 3 classes.

Comment 10

[Stefan [:stefanh]]

(In reply to comment #7)
> Can you describe the binary changes? (I don't have an easy way to revert a
> patch that changes images so I tried to avoid applying them.)

Sorry, I noticed I wasn't very clear on this. The binary changes are basically making the icons that will appear in alltabs menuitems 16x16 in size. That also means vertically/horisontally re-position them. I can attach the icons here, if that will help you ('hg revert path/to/icon' doesn't work for you?)

Comment 11

[[e-mail]]

(In reply to comment #8)
>(In reply to comment #7)
>>(From update of attachment 475946 [details] [diff] [review])
>>>+                if (aTabNode.hasAttribute(attribute))
>>>+                {
>>>+                  menuItem.setAttribute(attribute, aTabNode.getAttribute(attribute));
>>What if the tab node doesn't have that attribute?
>Then it doesn't get set on the menuitem. That's what the 'if
>(aTabNode.hasAttribute(attribute))' is for.
Sorry, I was confusing myself there; each menuitem maps to exactly one tab node, so it doesn't matter if the attribute is there or not.

>>>-.tabmail-tab[type="message"] .tab-icon {
>>>-  margin-top: -4px;
>>This is because all the icons are now the same size?
>Yes, well - I also adjsted the icons vertical position, so it's as hight up as
>it can be. There might be a slight difference from before since I think I miss
>1-2px, so maybe I should add it back but with a different negative value.
I'll let Mnyromyr decide that.

>>>diff --git a/suite/themes/modern/messenger/icons/message-junk-other.gif b/suite/themes/modern/messenger/icons/message-junk-other.gif
>>I didn't spot any CSS size changes, did I just scroll too quickly? Or what are
>>the images changes for?
>That's a mistake, actually - I made it 16x16 even though it's not used in the
>menu.
I asked about "images" - I just didn't want to quote the whole diff ;-)

(In reply to comment #10)
>('hg revert path/to/icon' doesn't work for you?)
It would, but it's a hassle with so many affected files ;-)

Comment 12

[[e-mail]]

Comment on attachment 476299 [details] [diff] [review]
Something like this?

This looks reasonable from the point of view of the class changes.

Comment 13

[Stefan [:stefanh]]

(In reply to comment #11)

> >>>diff --git a/suite/themes/modern/messenger/icons/message-junk-other.gif b/suite/themes/modern/messenger/icons/message-junk-other.gif
> >>I didn't spot any CSS size changes, did I just scroll too quickly? Or what are
> >>the images changes for?
> >That's a mistake, actually - I made it 16x16 even though it's not used in the
> >menu.
> I asked about "images" - I just didn't want to quote the whole diff ;-)

Ah, ok - you probably scrolled too quickly then ;-)

Comment 14

[Stefan [:stefanh]]

Comment on attachment 475946 [details] [diff] [review]
Icons in menuitems

Will have a new patch up tomorrow.

Comment 15

[Stefan [:stefanh]]

Attachment: New version

* Use a shared class 'icon-holder' to style both tab and menuitem icons
* Keep a adjusted negative margin on message icons, but move the rules to mailWindow1.css (that way we have no tabmail-specific rules in the other files)

Comment 16

[Karsten Düsterloh]

Comment on attachment 476520 [details] [diff] [review]
New version

The problem with this approach is that folder pane styles (like boldness of text, etc.) get inflected into the popup, which is wrong. Only the styles for the icon, the checkmark (if visible at all) and the marking of the current tab should have effect here. 
Only the current tab should be bold, which was broken in Linux anyway. 
(Furthermore, I noticed that the mail drop down buttons on Mac don't bold their default action - is that on purpose (why?!) or just broken?)

Oh, and the Local Folders icon isn't correctly centered vertically everwhere, both on Mac and Linux.

Comment 17

[Stefan [:stefanh]]

(In reply to comment #16)
> Comment on attachment 476520 [details] [diff] [review]
> New version
> The problem with this approach is that folder pane styles (like boldness of
> text, etc.) get inflected into the popup, which is wrong. Only the styles for
> the icon, the checkmark (if visible at all) and the marking of the current tab
> should have effect here.

The patch only apply the correct icons in the menuitems. Any other styling you see (boldness etc) can also be seen without the patch.

Comment 18

[Karsten Düsterloh]

(In reply to comment #17)
> The patch only apply the correct icons in the menuitems. Any other styling you
> see (boldness etc) can also be seen without the patch.

Sorry to say that, but: Wrong!

These changes propagate the boldness into the popup:

| -.tabmail-tab[type="folder"][NewMessages="true"],
| +.icon-holder[type="folder"][NewMessages="true"],
|  treechildren::-moz-tree-cell-text(folderNameCol, newMessages-true) {
|    font-weight: bold;
|  }

Comment 19

[Stefan [:stefanh]]

(In reply to comment #18)
> (In reply to comment #17)
> > The patch only apply the correct icons in the menuitems. Any other styling you
> > see (boldness etc) can also be seen without the patch.
> Sorry to say that, but: Wrong!
> These changes propagate the boldness into the popup:
> | -.tabmail-tab[type="folder"][NewMessages="true"],
> | +.icon-holder[type="folder"][NewMessages="true"],
> |  treechildren::-moz-tree-cell-text(folderNameCol, newMessages-true) {
> |    font-weight: bold;
> |  }

Ahh, you're right - sorry, I missed that. I guess I should undo the change and then move the .tabmail-tab rule to mailWindow1.css.

Comment 20

[Stefan [:stefanh]]

Attachment: Another version

* Fixed the Local folders icon by using the local-mailhost icon saved as server-local (they're identical, except that local-mailhost is vertically centered)

*Reverted the bold styling per comment #19 and made selected alltabs-item's bold, except for mac classic

Comment 21

[Karsten Düsterloh]

Comment on attachment 478440 [details] [diff] [review]
Another version

r=me based upon the diff to the prior version and application of the patch under Linux - I don't have a working Mac/PPC build atm.

Comment 22

[Stefan [:stefanh]]

Comment on attachment 478440 [details] [diff] [review]
Another version

I think we should take this for beta - it's mostly theme/ui changes and I would consider it safe.

Comment 23

[Robert Kaiser]

Comment on attachment 478440 [details] [diff] [review]
Another version

Yes, let's do it - but please land it fast. ;-)

Comment 24

[Stefan [:stefanh]]

http://hg.mozilla.org/comm-central/rev/8c523f3c00fb