Closed Bug 599735 Opened 12 years ago Closed 12 years ago

Add feed icon to the new Subscribe entry in Bookmarks menu

Categories

(Firefox :: Menus, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 4.0b8

People

(Reporter: contact, Assigned: tymerkaev)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

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
Version: unspecified → Trunk
OS: Linux → All
Hardware: x86 → All
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: Menus → RSS Discovery and Preview
Depends on: 578967
QA Contact: menus → rss.preview
Attached patch patch (obsolete) — Splinter Review
Attachment #480613 - Flags: review?(dao)
Component: RSS Discovery and Preview → Menus
QA Contact: rss.preview → menus
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?
Attachment #480613 - Flags: review?(dao) → review-
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #480613 - Attachment is obsolete: true
Attachment #480891 - Flags: review?(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.
Attachment #480891 - Flags: review?(dao) → review+
(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.
The first quoted part isn't app menu specific and can be moved.
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #480891 - Attachment is obsolete: true
Attachment #481182 - Flags: review?(dao)
Attachment #481182 - Flags: review?(dao)
Attachment #481182 - Flags: review+
Attachment #481182 - Flags: approval2.0?
Assignee: nobody → tymerkaev
Attachment #481182 - Flags: approval2.0?
Attached patch patch v4 (obsolete) — Splinter Review
Attachment #481182 - Attachment is obsolete: true
Attachment #482578 - Flags: review?(dao)
Attachment #482578 - Flags: review?(dao) → review+
Status: NEW → ASSIGNED
Attachment #482578 - Flags: approval2.0?
Attachment #482578 - Flags: ui-review?(faaborg)
Attachment #482578 - Flags: ui-review?(faaborg) → ui-review+
This patch doesn't seem to have any effect on Mac.
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?
(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).
Yeah, Azat pointed me to comments in bug 603605 that make it pretty clear that we should just not add this for Mac.
I'm late it replying, but yeah no icons on mac.
Attachment #482578 - Attachment description: updated to tip → patch v4
Attachment #482578 - Attachment is obsolete: true
Attached patch patch v5Splinter Review
Same as previous patch, but without icon on Mac.
Attachment #486141 - Flags: review?(gavin.sharp)
Attachment #486141 - Flags: review?(gavin.sharp) → review?(dao)
Attachment #486141 - Flags: review?(dao) → review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/cf8ef9c157d5
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b8
I still don't see the icon with the latest version of Minefield on Linux. Can anyone confirm?
Flags: in-testsuite-
Flags: in-litmus-
(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
As usual, icons in menus depend on the corresponding Gtk setting.
OK, I see what you mean. So, with menus_have_icons=true in gconf, I see the RSS icons showing as expected.
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
Status: RESOLVED → VERIFIED
Duplicate of this bug: 604459
You need to log in before you can comment on or make changes to this bug.