1636775 - Fix feed folder/item icon regression

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[alta88]

There were 3 icons for feeds for a reason, to differentiate an account/server from a folder with subscriptions from a subscription item url.
To elaborate: most/many folderpane feed account folders display the feed url's favicon but not all; this is even more evident in subscribe dialog where all 3 types are shown and an opened folder displays the folder icon in order to show the user multiple feed items can be subscribed.

Comment 1

[alta88]

Attachment: feedIcons.patch

Comment 2

[Richard Marti (:Paenglab)]

Comment on attachment 9147108 [details] [diff] [review]
feedIcons.patch

The changes makes sense but this patch doesn't apply because of bug 1635370. Please update to tip.

Comment 3

[Richard Marti (:Paenglab)]

Comment on attachment 9147108 [details] [diff] [review]
feedIcons.patch

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

::: mail/themes/shared/mail/folderPane.css
@@ +123,4 @@
>  .tabmail-tab[type="folder"][IsFeedFolder="true"],
>  treechildren::-moz-tree-image(folderNameCol, isFeedFolder-true) {
> +  list-style-image: url("chrome://messenger/skin/icons/feeds.svg");
> +  fill: currentColor;

This fill shouldn't be needed.

Comment 4

[Richard Marti (:Paenglab)]

And please also update https://searchfox.org/comm-central/rev/99f79dafbc912022853d27c0d78c39afc2e5afc5/mail/themes/shared/mail/accountCentral.css#110

Comment 5

[alta88]

Attachment: feedIcons.patch

updated for comments.

Comment 6

[alta88]

Attachment: feedIcons.patch

updated for account central.

Comment 7

[alta88]

Paenglab, I think it would be nicer if there were a feed-folder icon similar to search-folder, for just tree folders. Could you make one?

Comment 8

[Richard Marti (:Paenglab)]

Attachment: feed-folder.svg

Like this?

Comment 9

[alta88]

Attachment: feedFolder.patch

Exactly! Thanks. This patch goes on top of the previous patch.

Comment 10

[Richard Marti (:Paenglab)]

Comment on attachment 9147112 [details] [diff] [review]
feedIcons.patch

Thanks.

Comment 11

[alta88]

Attachment: feedMessageTab.patch

Finally, update feed message in tabs icon. (It would be nice if it were orange, but it seems the tab standard is currentColor).

Comment 12

[alta88]

Attachment: feedMessageTab.patch

In retrospect, the tab should match the feed folder, otherwise it would be inconsistent with the others.

Comment 13

[Richard Marti (:Paenglab)]

Comment on attachment 9147125 [details] [diff] [review]
feedMessageTab.patch

I had this feedMessage tabs in a other patch. But like this all belongs together.

Please update all patch commit messages wit `r=Paenglab`.

Comment 14

[alta88]

Attachment: feedIcons.patch

Comment 15

[alta88]

Attachment: feedFolder.patch

Comment 16

[alta88]

Attachment: feedMessageTab.patch

Comment 17

[alta88]

Attachment: feedFolder.patch

Comment 18

[alta88]

if it matters, landing order should be feedIcons.patch, feedFolder.patch, feedMessageTab.patch.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7ff3e6dcd86f
Fix feed folder/item icon regression, r=Paenglab
https://hg.mozilla.org/comm-central/rev/fa260ff0b744
Add a feed folder icon for trees, r=Paenglab
https://hg.mozilla.org/comm-central/rev/f82b65b87ebe
Update feed message tab icon, r=Paenglab

Comment 20

[alta88]

Attachment: feedcss2.patch

Followup:

1. height: auto is wrong, this makes no space for icons to be resolved, and shift the text when they are, plus forces layout to calculate which is way wrong in nsITree; too clever by half going on here :/
2. use feed folder icon in foldermenus for folders with subscriptions
3. due to de grid and de textbox, some css in subscribe isn't necessary. almost all of it can be shared (please check mac).

Comment 21

[Richard Marti (:Paenglab)]

Comment on attachment 9148236 [details] [diff] [review]
feedcss2.patch

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

Thanks! r+ with the comments addressed. Please update the commit message to be correct with bug number and commit text.

::: mail/themes/osx/mail/newsblog/feed-subscriptions.css
@@ +11,1 @@
>    padding: 14px;

Change this to 4px and remove the #contentPane rule below and we have together with the shared #contentPane margin the 14px spacing back.

::: mail/themes/shared/mail/feedSubscribe.css
@@ +4,5 @@
> +
> +#subscriptionsDialog {
> +  width: 40em;
> +  height: 30em;
> +  padding: 0;

This padding isn't needed anymore. The dialog is now a <window> which has no padding set. Please remove it.

Comment 22

[alta88]

Attachment: feedcss2.patch

updated for comments.

By the way, there is yet another regression in that a feed url now cannot be updated. I'm not going to spend time on it.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8570a1102c50
Fix foldermenu icon, tree-image sizing, shared/updated subscribe dialog css. r=Paenglab