Add feed icon to the new Subscribe entry in Bookmarks menu

VERIFIED FIXED in Firefox 4.0b8

Status

()

VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: nicolas, Assigned: tymerkaev)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 4.0b8
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

8 years ago
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
(Reporter)

Updated

8 years ago
Version: unspecified → Trunk
(Reporter)

Updated

8 years ago
OS: Linux → All
Hardware: x86 → All
(Assignee)

Updated

8 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: Menus → RSS Discovery and Preview
Depends on: 578967
QA Contact: menus → rss.preview
(Assignee)

Comment 1

8 years ago
Created attachment 480613 [details] [diff] [review]
patch
Attachment #480613 - Flags: review?(dao)

Updated

8 years ago
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-
(Assignee)

Comment 3

8 years ago
Created attachment 480891 [details] [diff] [review]
patch v2
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+
(Assignee)

Comment 5

8 years ago
(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.
(Assignee)

Comment 7

8 years ago
Created attachment 481182 [details] [diff] [review]
patch v3
Attachment #480891 - Attachment is obsolete: true
Attachment #481182 - Flags: review?(dao)

Updated

8 years ago
Attachment #481182 - Flags: review?(dao)
Attachment #481182 - Flags: review+
Attachment #481182 - Flags: approval2.0?

Updated

8 years ago
Assignee: nobody → tymerkaev
(Assignee)

Updated

8 years ago
Attachment #481182 - Flags: approval2.0?
(Assignee)

Comment 8

8 years ago
Created attachment 482578 [details] [diff] [review]
patch v4
Attachment #481182 - Attachment is obsolete: true
Attachment #482578 - Flags: review?(dao)

Updated

8 years ago
Attachment #482578 - Flags: review?(dao) → review+
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

8 years ago
Attachment #482578 - Flags: approval2.0?
(Assignee)

Updated

8 years ago
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?

Comment 11

8 years ago
(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.
(Assignee)

Updated

8 years ago
Attachment #482578 - Attachment description: updated to tip → patch v4
Attachment #482578 - Attachment is obsolete: true
(Assignee)

Comment 14

8 years ago
Created attachment 486141 [details] [diff] [review]
patch v5

Same as previous patch, but without icon on Mac.
Attachment #486141 - Flags: review?(gavin.sharp)
(Assignee)

Updated

8 years ago
Attachment #486141 - Flags: review?(gavin.sharp) → review?(dao)

Updated

8 years ago
Attachment #486141 - Flags: review?(dao) → review+

Updated

8 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/cf8ef9c157d5
Status: ASSIGNED → RESOLVED
Last Resolved: 8 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
(Assignee)

Updated

8 years ago
Duplicate of this bug: 604459
Depends on: 625410
Depends on: 625412
You need to log in before you can comment on or make changes to this bug.