Last Comment Bug 593944 - No icons in alltabs menuitems
: No icons in alltabs menuitems
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Themes (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1b1
Assigned To: Stefan [:stefanh] (away until May 28)
:
Mentors:
Depends on:
Blocks: 593840
  Show dependency treegraph
 
Reported: 2010-09-07 03:08 PDT by Stefan [:stefanh] (away until May 28)
Modified: 2010-10-06 10:28 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Screenshot, showing empty space and large checkmark (8.63 KB, image/png)
2010-09-07 03:13 PDT, Stefan [:stefanh] (away until May 28)
no flags Details
WIP1 (53.92 KB, patch)
2010-09-12 17:50 PDT, Stefan [:stefanh] (away until May 28)
no flags Details | Diff | Review
Icons in menuitems (54.36 KB, patch)
2010-09-16 12:13 PDT, Stefan [:stefanh] (away until May 28)
no flags Details | Diff | Review
Something like this? (55.96 KB, patch)
2010-09-17 10:49 PDT, Stefan [:stefanh] (away until May 28)
no flags Details | Diff | Review
New version (56.84 KB, patch)
2010-09-18 06:33 PDT, Stefan [:stefanh] (away until May 28)
mnyromyr: review-
neil: superreview+
Details | Diff | Review
Another version (58.62 KB, patch)
2010-09-24 16:21 PDT, Stefan [:stefanh] (away until May 28)
mnyromyr: review+
kairo: approval‑seamonkey2.1b1+
Details | Diff | Review

Description Stefan [:stefanh] (away until May 28) 2010-09-07 03:08:37 PDT
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] (away until May 28) 2010-09-07 03:13:08 PDT
Created attachment 472592 [details]
Screenshot, showing empty space and large checkmark

As you can see, the checkmark is also dimensioned for a larger font.
Comment 2 Stefan [:stefanh] (away until May 28) 2010-09-07 16:59:19 PDT
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] (away until May 28) 2010-09-08 11:58:11 PDT
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] (away until May 28) 2010-09-10 08:01:02 PDT
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] (away until May 28) 2010-09-12 17:50:46 PDT
Created attachment 474597 [details] [diff] [review]
WIP1
Comment 6 Stefan [:stefanh] (away until May 28) 2010-09-16 12:13:57 PDT
Created attachment 475946 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2010-09-16 13:27:58 PDT
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] (away until May 28) 2010-09-16 13:48:32 PDT
(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] (away until May 28) 2010-09-17 10:49:06 PDT
Created attachment 476299 [details] [diff] [review]
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] (away until May 28) 2010-09-17 11:02:21 PDT
(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 neil@parkwaycc.co.uk 2010-09-17 11:33:56 PDT
(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 neil@parkwaycc.co.uk 2010-09-17 11:39:40 PDT
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] (away until May 28) 2010-09-17 13:29:05 PDT
(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] (away until May 28) 2010-09-17 13:30:07 PDT
Comment on attachment 475946 [details] [diff] [review]
Icons in menuitems

Will have a new patch up tomorrow.
Comment 15 Stefan [:stefanh] (away until May 28) 2010-09-18 06:33:43 PDT
Created attachment 476520 [details] [diff] [review]
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 2010-09-21 15:24:43 PDT
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] (away until May 28) 2010-09-21 23:31:26 PDT
(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 2010-09-22 01:26:43 PDT
(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] (away until May 28) 2010-09-22 03:55:43 PDT
(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] (away until May 28) 2010-09-24 16:21:54 PDT
Created attachment 478440 [details] [diff] [review]
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 2010-10-05 16:09:23 PDT
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] (away until May 28) 2010-10-06 10:18:17 PDT
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 (not working on stability any more) 2010-10-06 10:21:30 PDT
Comment on attachment 478440 [details] [diff] [review]
Another version

Yes, let's do it - but please land it fast. ;-)
Comment 24 Stefan [:stefanh] (away until May 28) 2010-10-06 10:28:13 PDT
http://hg.mozilla.org/comm-central/rev/8c523f3c00fb

Note You need to log in before you can comment on or make changes to this bug.