Last Comment Bug 464524 - List all tabs menupopup should use the favicons of the Tabs
: List all tabs menupopup should use the favicons of the Tabs
Status: RESOLVED FIXED
: polish
Product: Thunderbird
Classification: Client Software
Component: Mail Window Front End (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: Thunderbird 17.0
Assigned To: Prasad Sunkari [:prasad]
:
Mentors:
: 530772 725708 (view as bug list)
Depends on:
Blocks: tabsmeta 502616 785580
  Show dependency treegraph
 
Reported: 2008-11-12 12:22 PST by Richard Marti (:Paenglab)
Modified: 2012-09-26 04:02 PDT (History)
16 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
screenshot (11.29 KB, image/png)
2009-05-26 04:14 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
Uses computed style from the tabs. (32.80 KB, patch)
2009-10-14 14:48 PDT, Prasad Sunkari [:prasad]
no flags Details | Diff | Review
Version 2. Fixed the -moz-image-region on close button. (33.21 KB, patch)
2009-10-15 10:50 PDT, Prasad Sunkari [:prasad]
clarkbw: ui‑review+
Details | Diff | Review
v2 de-bitrotted for review (26.50 KB, patch)
2010-02-12 23:53 PST, Andrew Sutherland [:asuth]
bugmail: ui‑review+
Details | Diff | Review
v2 de-bitrotted again, plus calendar (35.76 KB, patch)
2010-02-28 23:08 PST, Philipp Kewisch [:Fallen]
bugmail: review+
philipp: ui‑review+
Details | Diff | Review
New patch (2.25 KB, patch)
2012-08-12 06:56 PDT, Richard Marti (:Paenglab)
mconley: review-
Details | Diff | Review
Alltabs patch (2.25 KB, patch)
2012-08-20 14:08 PDT, Richard Marti (:Paenglab)
mconley: review+
Details | Diff | Review

Description Richard Marti (:Paenglab) 2008-11-12 12:22:24 PST
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, NI for questions) 2009-01-15 19:30:14 PST
indeed, would give TB3 a more polished look.
Comment 2 Dan Mosedale (:dmose) 2009-03-02 14:16:13 PST
In currently nightlies on Mac, I don't see any favicon at all.
Comment 3 Gary Kwong [:gkw] [:nth10sd] 2009-05-26 04:14:25 PDT
Created attachment 379668 [details]
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] 2009-05-26 13:33:19 PDT
yep, we want this bit of polish
Comment 5 Bryan Clark (DevTools PM) [@clarkbw] 2009-10-13 11:06:03 PDT
giving this to andreas if there is time
Comment 6 Prasad Sunkari [:prasad] 2009-10-14 14:48:52 PDT
Created attachment 406305 [details] [diff] [review]
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] 2009-10-14 14:52:17 PDT
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) 2009-10-14 14:56:55 PDT
(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] 2009-10-14 15:11:24 PDT
(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) 2009-10-15 01:05:38 PDT
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) 2009-10-15 09:13:25 PDT
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) 2009-10-15 09:22:22 PDT
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] 2009-10-15 09:56:19 PDT
Oops!
You are right.  Will take a look at it.
Comment 14 Prasad Sunkari [:prasad] 2009-10-15 10:50:47 PDT
Created attachment 406473 [details] [diff] [review]
Version 2.  Fixed the -moz-image-region on close button.

Fixed the problem reported by Andreas
Comment 15 Bryan Clark (DevTools PM) [@clarkbw] 2009-10-30 15:04:49 PDT
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) 2009-10-31 02:09:30 PDT
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) 2009-10-31 02:14:00 PDT
(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 (:dmose) 2009-10-31 07:56:15 PDT
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) 2010-02-12 20:26:57 PST
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] 2010-02-12 23:53:43 PST
Created attachment 426808 [details] [diff] [review]
v2 de-bitrotted for review
Comment 21 Philipp Kewisch [:Fallen] 2010-02-28 23:08:17 PST
Created attachment 429478 [details] [diff] [review]
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] 2010-02-28 23:28:55 PST
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 23 Philipp Kewisch [:Fallen] 2010-02-28 23:31:16 PST
*** Bug 530772 has been marked as a duplicate of this bug. ***
Comment 24 Andrew Sutherland [:asuth] 2010-04-09 00:01:09 PDT
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 2010-04-29 23:31:19 PDT
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] 2010-04-30 00:03:32 PDT
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) 2012-08-12 06:56:01 PDT
Created attachment 651194 [details] [diff] [review]
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 28 Ludovic Hirlimann [:Usul] 2012-08-14 04:39:21 PDT
*** Bug 725708 has been marked as a duplicate of this bug. ***
Comment 29 Mike Conley (:mconley) - (needinfo me!) 2012-08-20 11:51:26 PDT
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) 2012-08-20 14:08:39 PDT
Created attachment 653514 [details] [diff] [review]
Alltabs patch

patch changed like in review comment proposed.
Comment 31 Mike Conley (:mconley) - (needinfo me!) 2012-08-21 07:42:26 PDT
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] 2012-08-21 16:46:49 PDT
https://hg.mozilla.org/comm-central/rev/3dbaf4253214

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