Closed Bug 1729873 Opened 3 years ago Closed 3 years ago

Address book menu lists have lost their icons

Categories

(Thunderbird :: Preferences, defect)

defect

Tracking

(thunderbird_esr91 fixed, thunderbird93 fixed)

RESOLVED FIXED
94 Branch
Tracking Status
thunderbird_esr91 --- fixed
thunderbird93 --- fixed

People

(Reporter: darktrojan, Assigned: Paenglab)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

In the composition preferences are three drop-down lists of address books. These are supposed to have icons, but the CSS for the icons is in messenger.css which is no longer included in the preferences.

The same list widget (<menulist is="menulist-addrbooks">) is used elsewhere so the style needs to remain somewhere useful.

Keywords: regression
Attached patch 1729873-abMenuItem-image.patch (obsolete) (deleted) — Splinter Review

Henry, you are the set icon directly specialist. ;-)

How about this? This patch removes the need of messenger.css for this specific icons. It's not a HTML conversion as the menulists are still using XUL.

Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #9240288 - Flags: review?(henry)
Comment on attachment 9240288 [details] [diff] [review] 1729873-abMenuItem-image.patch Review of attachment 9240288 [details] [diff] [review]: ----------------------------------------------------------------- You also need to set the "image" to "chrome://messenger/skin/icons/address.svg" here https://searchfox.org/comm-central/rev/f460fb11697497d39d4a7f354c815b9187cff171/mail/components/addrbook/content/menulist-addrbooks.js#172. You can check for this menu item in the composition address side panel. As well as "AddrBook", you can remove the "IsAllAB" attribute because this isn't used anywhere. ::: mail/components/addrbook/content/menulist-addrbooks.js @@ +199,1 @@ > } This doesn't quite match the logic of the CSS selector. For example, if both `ab.isMailList` and `ab.isRemote` are true, then this will select "globe.svg" instead of "ablist.svg". However, I'm not sure if such a combination is possible. Even so, you are still potentially setting the "image" attribute three times instead of once. This should cover the behaviour with just one setting: ``` if (ab.isMailList) { listItem.setAttribute("image", "chrome://messenger/skin/icons/ablist.svg"); } else if (ab.isRemote && ab.isSecure) { listItem.setAttribute("image", "chrome://messenger/skin/icons/globe-secure.svg"); } else if (ab.isRemote) { listItem.setAttribute("image", "chrome://messenger/skin/icons/globe.svg"); } else { listItem.setAttribute("image", "chrome://messenger/skin/icons/address.svg"); } ```
Attachment #9240288 - Flags: review?(henry) → review-

Sorry, I got "address.svg" and "ablist.svg" the wrong way around in my initial comment, but I edited it the right way around now.

Comment on attachment 9240288 [details] [diff] [review] 1729873-abMenuItem-image.patch Review of attachment 9240288 [details] [diff] [review]: ----------------------------------------------------------------- I'm pretty sure you can also get rid of this "IsNone" attribute https://searchfox.org/comm-central/rev/f460fb11697497d39d4a7f354c815b9187cff171/mail/components/addrbook/content/menulist-addrbooks.js#158. You might want to check how this appears in the menu since it doesn't have an image.
Attached patch 1729873-abMenuItem-image.patch (obsolete) (deleted) — Splinter Review

Yes, your proposals are better.

Attachment #9240288 - Attachment is obsolete: true
Attachment #9240336 - Flags: review?(henry)
Attached patch 1729873-abMenuItem-image.patch (deleted) — Splinter Review

Fixed the linting.

Attachment #9240336 - Attachment is obsolete: true
Attachment #9240336 - Flags: review?(henry)
Attachment #9240338 - Flags: review?(henry)
Comment on attachment 9240338 [details] [diff] [review] 1729873-abMenuItem-image.patch Review of attachment 9240338 [details] [diff] [review]: ----------------------------------------------------------------- Looks good
Attachment #9240338 - Flags: review?(henry) → review+
Target Milestone: --- → 94 Branch

Pushed by thunderbird@calypsoblue.org:
https://hg.mozilla.org/comm-central/rev/2990d8241079
Set abMenuItem images in JS directly. r=henry

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED

Comment on attachment 9240338 [details] [diff] [review]
1729873-abMenuItem-image.patch

[Approval Request Comment]
User impact if declined: no icons in menulists in compose prefs
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): low

Attachment #9240338 - Flags: approval-comm-esr91?
Attachment #9240338 - Flags: approval-comm-beta?
Blocks: tb91found

Comment on attachment 9240338 [details] [diff] [review]
1729873-abMenuItem-image.patch

[Triage Comment]
approved for beta

Attachment #9240338 - Flags: approval-comm-beta? → approval-comm-beta+

Comment on attachment 9240338 [details] [diff] [review]
1729873-abMenuItem-image.patch

[Triage Comment]
Approved for esr91

Attachment #9240338 - Flags: approval-comm-esr91? → approval-comm-esr91+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: