The default bug view has changed. See this FAQ.

List all tabs menupopup should use the favicons of the Tabs

RESOLVED FIXED in Thunderbird 17.0

Status

Thunderbird
Mail Window Front End
--
enhancement
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: Paenglab, Assigned: prasad)

Tracking

(Blocks: 1 bug, {polish})

Trunk
Thunderbird 17.0
polish
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

9 years ago
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
indeed, would give TB3 a more polished look.
Blocks: 392328
Flags: wanted-thunderbird3?
Keywords: polish
In currently nightlies on Mac, I don't see any favicon at all.
Keywords: qawanted

Updated

8 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
Keywords: qawanted
Version: unspecified → Trunk
yep, we want this bit of polish
Flags: wanted-thunderbird3? → wanted-thunderbird3+
giving this to andreas if there is time
Assignee: nobody → nisses.mail
(Assignee)

Comment 6

8 years ago
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 ;)
Attachment #406305 - Flags: review?(clarkbw)
(Assignee)

Comment 7

8 years ago
Please read the first line of the second paragraph as "The patch also changes how the icons are applied to tabs"
(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 :-)
Blocks: 502616
(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 on attachment 406305 [details] [diff] [review]
Uses computed style from the tabs.

Actually I'll defer to Phil as he likes themes ;-)
Attachment #406305 - Flags: ui-review?(clarkbw)
Attachment #406305 - Flags: review?(philringnalda)
Attachment #406305 - Flags: review?(clarkbw)
(Assignee)

Updated

8 years ago
Assignee: nisses.mail → prasad
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
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.
(Assignee)

Comment 13

8 years ago
Oops!
You are right.  Will take a look at it.
(Assignee)

Comment 14

8 years ago
Created attachment 406473 [details] [diff] [review]
Version 2.  Fixed the -moz-image-region on close button.

Fixed the problem reported by Andreas
Attachment #406305 - Attachment is obsolete: true
Attachment #406473 - Flags: ui-review?(clarkbw)
Attachment #406473 - Flags: review?(philringnalda)
Attachment #406305 - Flags: ui-review?(clarkbw)
Attachment #406305 - Flags: review?(philringnalda)
Attachment #406473 - Flags: ui-review?(clarkbw) → ui-review+
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.
(Reporter)

Comment 16

8 years ago
This looks also good for me on Win 7.

Will this come in to 3.0 if review+ or is it to late?
(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.
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 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.
Attachment #406473 - Flags: review?(philringnalda) → review?(bugmail)
Created attachment 426808 [details] [diff] [review]
v2 de-bitrotted for review
Attachment #406473 - Attachment is obsolete: true
Attachment #426808 - Flags: ui-review+
Attachment #426808 - Flags: review?(bugmail)
Attachment #406473 - Flags: review?(bugmail)
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?
Attachment #426808 - Attachment is obsolete: true
Attachment #429478 - Flags: ui-review+
Attachment #429478 - Flags: review?(bugmail)
Attachment #426808 - Flags: review?(bugmail)
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;
}
Duplicate of this bug: 530772
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
Attachment #429478 - Flags: review?(bugmail) → review+
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 25

7 years ago
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
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 27

5 years ago
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.
Attachment #651194 - Flags: review?(mconley)
Duplicate of this bug: 725708
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?
Attachment #651194 - Flags: review?(mconley) → review-
(Reporter)

Comment 30

5 years ago
Created attachment 653514 [details] [diff] [review]
Alltabs patch

patch changed like in review comment proposed.
Attachment #651194 - Attachment is obsolete: true
Attachment #653514 - Flags: review?(mconley)
Comment on attachment 653514 [details] [diff] [review]
Alltabs patch

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

This looks good to me. Thanks Richard!
Attachment #653514 - Flags: review?(mconley) → review+
(Reporter)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/3dbaf4253214
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago5 years ago
Flags: wanted-thunderbird3+ → in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

5 years ago
Blocks: 785580
Target Milestone: --- → Thunderbird 17.0
You need to log in before you can comment on or make changes to this bug.