464524 - List all tabs menupopup should use the favicons of the Tabs

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, enhancement)

Description

[Richard Marti (:Paenglab)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081112 Minefield/3.1b2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081112 Lightning/1.0pre Shredder/3.0b1pre 

The List all tabs menupopup shows always a letter as favicon. This favicon should represent the favicon of the tab like calendar, task, inbox etc.

Reproducible: Always

Comment 1

[Wayne Mery (:wsmwk)]

indeed, would give TB3 a more polished look.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

In currently nightlies on Mac, I don't see any favicon at all.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: screenshot

Confirming on debug Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090523 Lightning/1.0pre Shredder/3.0b3pre

Comment 4

[Bryan Clark (DevTools PM) [:clarkbw]]

yep, we want this bit of polish

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

giving this to andreas if there is time

Comment 6

[Prasad Sunkari [:prasad]]

Attachment: Uses computed style from the tabs.

This patch uses the computed style from the tabs to set icons on the menuitems.  It also observes the changes to the "busy" status and updates icons accordingly.

The patch also changes how the icons are applied to tasks.  Instead of setting the icons to "tabmail-tab .tab-icon-image" we can directly set the styles to tabmail-tab and let .tab-icon-image inherit the icons.  This is both more efficient (not that it would matter much in this case) and also more consistent with other elements of Mozilla (like the menuitems).

Please note that the second change is mainly to make the first change possible in a slightly cleaner way (otherwise I will have to get the anonymous element from the tabNode and then retrieve its style).

On Linux it does fix another silly problem of not being able to see the "loading" icon on special folders.  The style for special folders sets a "-moz-image-region" to values out of the first 16x16 pixels - but the busy icon is not defined in those regions ;)

Comment 7

[Prasad Sunkari [:prasad]]

Please read the first line of the second paragraph as "The patch also changes how the icons are applied to tabs"

Comment 8

[Mark Banner (:standard8)]

(In reply to comment #6)
> On Linux it does fix another silly problem of not being able to see the
> "loading" icon on special folders.  The style for special folders sets a
> "-moz-image-region" to values out of the first 16x16 pixels - but the busy icon
> is not defined in those regions ;)

Aha, that would actually fix the remainder of bug 502616 for me :-)

Comment 9

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #8)
> Aha, that would actually fix the remainder of bug 502616 for me :-)

Mark, you should take review on this. :)

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 406305 [details] [diff] [review]
Uses computed style from the tabs.

Actually I'll defer to Phil as he likes themes ;-)

Comment 11

[Andreas Nilsson (:andreasn)]

When trying this patch on Linux with the gconf key buttons_have_icons set to false (now default on GNOME since 2.28 [1]), I seem to loose the close buttons on the tabs.

1. https://bugzilla.gnome.org/show_bug.cgi?id=583352

Comment 12

[Andreas Nilsson (:andreasn)]

Err...my error. It happens regardless what the gconf key says. No idea where I got that idea from. :)

I seem to be missing close button icons on Inbox, Drafts etc. but not on regular messages opened in a new tab.

Comment 13

[Prasad Sunkari [:prasad]]

Oops!
You are right.  Will take a look at it.

Comment 14

[Prasad Sunkari [:prasad]]

Attachment: Version 2. Fixed the -moz-image-region on close button.

Fixed the problem reported by Andreas

Comment 15

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 406473 [details] [diff] [review]
Version 2.  Fixed the -moz-image-region on close button.

this looks good and works for me on the mac.  will try the linux build later but it seems like it should work similarly.

Comment 16

[Richard Marti (:Paenglab)]

This looks also good for me on Win 7.

Will this come in to 3.0 if review+ or is it to late?

Comment 17

[Mark Banner (:standard8)]

(In reply to comment #16)
> This looks also good for me on Win 7.
> 
> Will this come in to 3.0 if review+ or is it to late?

It has missed code freeze for RC1 so we won't be taking it for TB 3.0. Not sure yet if we'd take it in a point release, but possibly not.

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Any resources that we apply to point releases are effectively stolen from core development, which already has fewer resources than we might like.  So my take is that our 2.0.0.x policy of being very strict about security and stability changes only on the branch is likely to continue to be a good fit for the Thunderbird team in the Tb3 timeframe.  As such, I think this should wait for 3.1 (which, happily, should be a small number of months after 3.0).

Comment 19

[Phil Ringnalda (:philor)]

Comment on attachment 406473 [details] [diff] [review]
Version 2.  Fixed the -moz-image-region on close button.

Since I've clearly failed you, let's see how the tab UI owner does instead.

Comment 20

[Andrew Sutherland [:asuth] (he/him)]

Attachment: v2 de-bitrotted for review

Comment 21

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: v2 de-bitrotted again, plus calendar

Thanks for telling me about this patch, I should have looked. I hope you don't mind, I de-bitrotted the gnomestripe conflict and added a few lines for calendar. Could you review them too?

Comment 22

[Philipp Kewisch [:Fallen][📱📆]]

I found an issue on linux. It seems I have a system setting for images in menus disabled. While this makes sense for normal menus, I'm not sure it should also be the case for the all tabs dropdown. The following xul.css rule hides the images:

menuitem:not([type]):not(:-moz-system-metric(images-in-menus)) > .menu-iconic-left {
  visibility: hidden;
}

Comment 24

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 429478 [details] [diff] [review]
v2 de-bitrotted again, plus calendar

Thanks for the patch and revised patch and sorry about the delay!  The holdup was my desire to test on all 3 platforms.  That problem was resolved... let me know if this breaks anything :)  (I'm declaring this low risk by inspection.)

I had to de-bitrot and made the following 2 changes:
- There was a margin thing on linux that was intended only for the tree icon case, but because the rule was used for both the tab and the tree icon, it was now affecting the tabs.  I broke things up so the tree gets its own rule with the margin.
- I added an explicit rule to deal with the icon problem Fallen noted.

pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/bd72fc3badc3

Comment 25

[Stefan Sitter [:ssitter]]

How is this supposed to work? I only see a generic paper icon instead of e.g. the trash can icon, the calendar icon or the task icon in the tab dropdown menu.

Tested Lightning/1.0b2pre with Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.5pre) Gecko/20100429 Lanikai/3.1pre

Tested Lightning/1.1a1pre with Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100428 Shredder/3.2a1pre

Comment 26

[Andrew Sutherland [:asuth] (he/him)]

Hm, yes, it does not seem to be working correctly right now.  I'd like to think I would have noticed that all the icons looked wrong with my initial patch, in which case I'll claim someone else regressed it.  I seem to recall some time after landing this patch that my tab icons broke too... maybe whatever broke that and then fixed it only fixed the tabs?

This is annoying to debug because of the part where it likes to tear stuff down when the popup closes and DOM inspector therefore has a hard time of ever getting to peak at what is going on.

Maybe Standard8 or bwinton or clarkbw or andreasn remember something?

Comment 27

[Richard Marti (:Paenglab)]

Attachment: New patch

Bug 545955 removed the previous patch some weeks later again with his check-in.

Unbitrotted and added a menuItem.removeAttribute("image") to show the throbber also on menuitems with the image attribute.

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 651194 [details] [diff] [review]
New patch

Review of attachment 651194 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Richard - just one suggestion. See below.

::: mail/base/content/tabmail.xml
@@ +2390,4 @@
>                  else
>                    menuItem.setAttribute(attrName, aEvent.newValue);
>              }
> +            if (attrName == "busy") {

Instead of putting a fresh conditional at the end of the function, why don't we add this to the switch statement above?

Comment 30

[Richard Marti (:Paenglab)]

Attachment: Alltabs patch

patch changed like in review comment proposed.

Comment 31

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 653514 [details] [diff] [review]
Alltabs patch

Review of attachment 653514 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me. Thanks Richard!

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/3dbaf4253214