637080 - "Subscribe to This Page" in Bookmarks button menu always disabled and lacks icon

Status: VERIFIED FIXED

(SeaMonkey :: Feed Discovery and Preview, defect)

Description

[Stefan [:stefanh]]

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]]

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)]

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)]

(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)]

(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]]

Maybe having a BMB_feedsmenu id and then fix nsBrowserStatusHandler.js to handle it would be ok.

Comment 6

[Stefan [:stefanh]]

Attachment: Proposed patch

I went with 'bmbFeedsMenu' since that fits better with the style in the js file.

Comment 7

[Stefan [:stefanh]]

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]]

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

[[e-mail]]

Attachment: Alternative approach [Checked in: See comment 12]

Comment 10

[Serge Gautherie (:sgautherie)]

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]]

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

[[e-mail]]

Pushed changeset 73a13310cbed to comm-central, removing all the !important

Comment 13

[Serge Gautherie (:sgautherie)]

[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