Last Comment Bug 637080 - "Subscribe to This Page" in Bookmarks button menu always disabled and lacks icon
: "Subscribe to This Page" in Bookmarks button menu always disabled and lacks icon
Status: VERIFIED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Feed Discovery and Preview (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1b3
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
Depends on: 580660
Blocks: 643323
  Show dependency treegraph
 
Reported: 2011-02-26 15:45 PST by Stefan [:stefanh]
Modified: 2011-03-23 12:50 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed patch (7.53 KB, patch)
2011-03-20 14:33 PDT, Stefan [:stefanh]
no flags Details | Diff | Splinter Review
Alternative approach [Checked in: See comment 12] (6.35 KB, patch)
2011-03-20 15:06 PDT, neil@parkwaycc.co.uk
stefanh: review+
bugzillamozillaorg_serge_20140323: feedback+
Details | Diff | Splinter Review

Description Stefan [:stefanh] 2011-02-26 15:45:29 PST
Recent trunk, all OS (Callek confirmed Windows)

STR:

1) Load planet.m.o and open the Bookmarks menu in the Bookmarks toolbar (note that it's the BM toolbar menu)
2) Look at the "Subscribe to This Page" menu

Expected result: Enabled menu with a feeds icon
Actual result: Disabled menu without feeds icon
Comment 1 Stefan [:stefanh] 2011-03-20 07:36:43 PDT
I think this should block, having a menitem that doesn't work seems to me like really bad ui. We should either remove it (it's not there in 2.0.x) or make it work.
Comment 2 Serge Gautherie (:sgautherie) 2011-03-20 10:29:05 PDT
Confirmed (again) on Windows.

SM 2.0 and 2.1:
window Bookmarks menu has this menuitem;
menuitem has the "feeds" icon;
menuitem is enabled on http://planet.mozilla.org/ page.
The feed icon in the Location Bar works too.

SM 2.1 only:
personal toolbar Bookmarks menu has this menuitem;
and this bug.

http://mxr.mozilla.org/comm-central/search?string=populateFeeds&case=1&find=%2Fsuite%2F
The 3 occurrences

***

http://mxr.mozilla.org/comm-central/search?string=feedsMenu&case=1&find=%2Fsuite%2F

Iiuc, nsBrowserStatusHandler.js handles (the) 1 'id=feedsMenu' only :-|
Comment 3 Serge Gautherie (:sgautherie) 2011-03-20 10:32:52 PDT
(In reply to comment #2)
> http://mxr.mozilla.org/comm-central/search?string=feedsMenu&case=1&find=%2Fsuite%2F
> 
> (the) 1 'id=feedsMenu' only :-|

And, because of the same lack of id, css rules don't apply the icon there :-|
Comment 4 Serge Gautherie (:sgautherie) 2011-03-20 10:39:12 PDT
(In reply to comment #2)
> SM 2.1 only:
> personal toolbar Bookmarks menu has this menuitem;
> and this bug.

Ftr, adding this occurrence is blamed to bug 580660.
Comment 5 Stefan [:stefanh] 2011-03-20 12:56:23 PDT
Maybe having a BMB_feedsmenu id and then fix nsBrowserStatusHandler.js to handle it would be ok.
Comment 6 Stefan [:stefanh] 2011-03-20 14:33:46 PDT
Created attachment 520554 [details] [diff] [review]
Proposed patch

I went with 'bmbFeedsMenu' since that fits better with the style in the js file.
Comment 7 Stefan [:stefanh] 2011-03-20 14:36:35 PDT
Comment on attachment 520554 [details] [diff] [review]
Proposed patch

Forgot to say that the "!important" didn't seemed necessary, but it's not tested on non-mac.
Comment 8 Stefan [:stefanh] 2011-03-20 14:41:56 PDT
I will file a follow-up, since on Mac, I need an icon for the disabled state (the native menu handles this for free, so I don't have a ready one and the windows one is most likely too washed-out).
Comment 9 neil@parkwaycc.co.uk 2011-03-20 15:06:14 PDT
Created attachment 520557 [details] [diff] [review]
Alternative approach
[Checked in: See comment 12]
Comment 10 Serge Gautherie (:sgautherie) 2011-03-20 17:51:53 PDT
Comment on attachment 520557 [details] [diff] [review]
Alternative approach
[Checked in: See comment 12]

Just what I was hoping for: class and command ;-)

Should '!important' be removed from the 2 non-Mac files too?
Comment 11 Stefan [:stefanh] 2011-03-21 12:33:42 PDT
Comment on attachment 520557 [details] [diff] [review]
Alternative approach
[Checked in: See comment 12]

This is much better, thanks :-) Should we have separate names for class and id?
Comment 12 neil@parkwaycc.co.uk 2011-03-22 03:04:39 PDT
Pushed changeset 73a13310cbed to comm-central, removing all the !important
Comment 13 Serge Gautherie (:sgautherie) 2011-03-23 12:50:04 PDT
[Mozilla/5.0 (Windows NT 5.0; rv:2.2a1pre) Gecko/20110323 Firefox/4.0b13pre SeaMonkey/2.1b3pre] (comm-central-trunk-win32/...)

Icon is there and item activates. (at the 3 places)

V.Fixed

Note You need to log in before you can comment on or make changes to this bug.