599735 - Add feed icon to the new Subscribe entry in Bookmarks menu

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[nicolas m]

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0b7pre) Gecko/20100925 Firefox/4.0b7pre
Build Identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0b7pre) Gecko/20100925 Firefox/4.0b7pre

Now that the Subscribe to RSS/Atom feature is only on the Bookmarks menu (moved from the location bar), the orange feed icon should be displayed in the menu, at the left of the text (in the way there is already icons close to the main menu entries).

Reproducible: Always

Comment 1

[tymerkaev]

Attachment: patch

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 480613 [details] [diff] [review]
patch

>--- a/browser/themes/winstripe/browser/browser.css
>+++ b/browser/themes/winstripe/browser/browser.css

>+#subscribeToPageMenuitem,
>+#subscribeToPageMenupopup,
>+#BMB_subscribeToPageMenuitem,
>+#BMB_subscribeToPageMenupopup {
>+  list-style-image: url("chrome://browser/skin/feeds/feedIcon16.png");
>+  -moz-image-region: auto;
>+}

-moz-image-region: auto; isn't needed here, is it?

Shouldn't the icon only be there if there's a feed?

What about appmenu_subscribeToPage and appmenu_subscribeToPageMenu?

Comment 3

[tymerkaev]

Attachment: patch v2

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 480891 [details] [diff] [review]
patch v2

>--- a/browser/themes/winstripe/browser/browser.css
>+++ b/browser/themes/winstripe/browser/browser.css
>@@ -284,8 +284,7 @@
> 
> #appmenu_subscribeToPage:not([disabled]),
> #appmenu_subscribeToPageMenu {
>-  list-style-image: url("chrome://browser/skin/feeds/feed-icons-16.png");
>-  -moz-image-region: rect(0px 16px 16px 0px);
>+  list-style-image: url("chrome://browser/skin/feeds/feedIcon16.png");
> }

>+#subscribeToPageMenuitem:not([disabled]),
>+#subscribeToPageMenupopup,
>+#BMB_subscribeToPageMenuitem:not([disabled]),
>+#BMB_subscribeToPageMenupopup {
>+  list-style-image: url("chrome://browser/skin/feeds/feedIcon16.png");
>+}

Can you combine these selectors?

You should probably get this ui-reviewed as well.

Comment 5

[tymerkaev]

(In reply to comment #4)

> Can you combine these selectors?

I'm not sure that they're should be combined because winstripe's browser.css contains specific part for app menu button styling, and moving something from / adding to it may be confused.

Comment 6

[Dão Gottwald [:dao]]

The first quoted part isn't app menu specific and can be moved.

Comment 7

[tymerkaev]

Attachment: patch v3

Comment 8

[tymerkaev]

Attachment: patch v4

Comment 9

[:Gavin Sharp [email: [e-mail]]]

This patch doesn't seem to have any effect on Mac.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

The problem is the :not([disabled]). The native Mac menu code only reads the CSS to determine whether or not to show an icon once, on menu initialization, so if you first open the menu when the menuitem is disabled, you won't ever get an icon, and if you open it when it's enabled, you'll always have one.

I guess you can fix it by just removing the :not(), but then the menu would always have an icon, even when disabled, which looks a bit odd. Should we just not add an icon on Mac? Alex?

Comment 11

[:Margaret Leibovic]

(In reply to comment #10)

> I guess you can fix it by just removing the :not(), but then the menu would
> always have an icon, even when disabled, which looks a bit odd. Should we just
> not add an icon on Mac? Alex?

I don't think there are supposed to be icons in the menus on Mac (https://bugzilla.mozilla.org/show_bug.cgi?id=603605#c16).

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Yeah, Azat pointed me to comments in bug 603605 that make it pretty clear that we should just not add this for Mac.

Comment 13

[Alex Faaborg [:faaborg] (Firefox UX)]

I'm late it replying, but yeah no icons on mac.

Comment 14

[tymerkaev]

Attachment: patch v5

Same as previous patch, but without icon on Mac.

Comment 15

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/cf8ef9c157d5

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I still don't see the icon with the latest version of Minefield on Linux. Can anyone confirm?

Comment 17

[mcdavis941 (sporadically reading bugmail)]

(In reply to comment #16)
> I still don't see the icon with the latest version of Minefield on Linux. Can
> anyone confirm?

I don't see the RSS icon in the Bookmarks menu either, on Linux.  Also not there for the Bookmarks Menu toolbar button.

Ubuntu 10.04 LTS
Mozilla/5.0 (X11; Linux i686; rv:2.0b8pre) Gecko/20101113 Firefox/4.0b8pre
Default Firefox theme
Default desktop theme

Comment 18

[Dão Gottwald [:dao]]

As usual, icons in menus depend on the corresponding Gtk setting.

Comment 19

[mcdavis941 (sporadically reading bugmail)]

OK, I see what you mean. So, with menus_have_icons=true in gconf, I see the RSS icons showing as expected.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Somehow this has been reset on our testing box in the lab. Restoring the setting shows up the icon now. Verified fixed with Mozilla/5.0 (X11; Linux i686; rv:2.0b8pre) Gecko/20101109 Firefox/4.0b8pre