416136 - [Mac] Feed icon in Bookmarks menu stretched/broken

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Stefan [:stefanh]]

Attachment: Screenshot - stretched feed icon

Something makes the feed icon in the Bookmarks menu stretched. Bookmark folders looks OK, so it's feed-specific. Maybe the -moz-image-region rules?

Comment 1

[Robert Kaiser]

the scree shot looks like it would be missing the image region... of course, that could easily be a mac-specific problem, as mac menus are not XUL :(

Comment 2

[Stefan [:stefanh]]

Yeah. Dunno if this is known, but I haven't seen any examples of -moz-image-region in menus other than this.

Comment 3

[Stefan [:stefanh]]

I'm pretty sure that the solution for this is to not use -moz-image-region. Note also that the icon won't change state when you hover it in the menu. I'm cc:ing Mark, so he can enlighten us ;)

Comment 4

[Stefan [:stefanh]]

fwiw, see bug 46177, comment #69. This was later "ported" to cocoa, but I assume that no support for the -moz-image-region was added.

Comment 5

[Stefan [:stefanh]]

Hmm, I'm also not 100% convinced that the disabled icon will work. If you have your menu opened while a page with feeds load, it might be that the icon doesn't change.

Comment 6

[Justin Wood (:Callek)]

Attachment: Simply remove icon in menu

Remove the menu icon for all platforms as a simple fix, neither of these .css files are currently preprocessed, and I'm not sure that it would be worth doing.  Neil, is this ok?

(If you would rather preprocess for removal on just mac, I'll attach a new patch)

Comment 7

[[e-mail]]

Comment on attachment 303956 [details] [diff] [review]
Simply remove icon in menu

KaiRo needs a standalone feed image for his helper applicates pref pane, so maybe just we'll be able to use that for the "normal" state instead.

Comment 8

[Philip Chee]

For a possible solution how about using chrome.manifest OS specific overrides. See:
Bug 397073 – Allow runtime overrides by OS version for chrome
Bug 416531 – Use chrome overrides to display Aero style icons on Vista

Comment 9

[Stefan [:stefanh]]

(In reply to comment #8)
> For a possible solution how about using chrome.manifest OS specific overrides.
> See:
> Bug 397073 – Allow runtime overrides by OS version for chrome
> Bug 416531 – Use chrome overrides to display Aero style icons on Vista
> 

Well, then I think it might be better to just package a different file/icon for mac.

Comment 10

[Robert Kaiser]

I added a stand-alone feed icon in bug 417590 - the only question now is how to use that on Mac while keeping the disable/hover/etc dynamic on the other platforms.

Comment 11

[Philip Chee]

You mean in modern, right? Classic already has navigator/{mac|win} so you could move the feed icon styles in there.

Comment 12

[Stefan [:stefanh]]

If we're hiding feeds in the 2.0a2 release, I think we should wait with a workaround and hope that bug 418774 gets fixed.

Comment 13

[Robert Kaiser]

I doubt bug 418774 gets fixed for 1.9.1 so we need to go and do split icons there.

Comment 14

[Stefan [:stefanh]]

On mac, you just need the normal and disabled states. But I think it's no harm if you use all states, it's just that hover and hover:active won't work in the native menu.

Comment 15

[Stefan [:stefanh]]

Actually, I just tried with 4 different icons and it appears that you can only have one icon on mac. If you launch a page that doesn't have feeds, then navigate to a page with feeds you'll still have the disabled icon. On the other hand, just having the "normal" icon will work, because it'll look disabled when the menuitem is disabled. I'm going to try another solution... stay tuned...

Comment 16

[Stefan [:stefanh]]

OK, so I just sliced out the "normal" icon from feeds.png, dumped it in navigator/mac as "feeds.png" and packaged it for mac. Since the native menu doesn't care about -moz-image-region, it seems to work - I just get the "normal" icon in the menu. Quite hacky, though - but if we're desperate for a2, I guess it'll work.

Comment 17

[Stefan [:stefanh]]

Attachment: patch for classic

Here's a patch for classic that does the trick. Of course, this does feels like a temporary solution... For classic, I'd rather have a separate navigator.css since that'd probably be necessary later anyway. Dunno what's acceptable for Modern, but we could probably do the same. Comments are welcome.

Comment 18

[Robert Kaiser]

This is the same icon as communicator/icons/feedIcon16.png, right? Why do we need the same icon twice in our theme?

Comment 19

[Stefan [:stefanh]]

(In reply to comment #18)
> This is the same icon as communicator/icons/feedIcon16.png, right? Why do we
> need the same icon twice in our theme?

Can you use feedIcon16.png without the need to change references in navigator.css? In that case, there's no need for a new icon.

Comment 20

[Stefan [:stefanh]]

Or do you suggest that we re-name "feedIcon16.png" to "feeds.png"? That's also an option.

Comment 21

[Robert Kaiser]

Renaming is no option IMHO, and I don't particularly like your hack either, actually. I'd rather use feedIcon16.png in navigator.css as the primary icon for the menu, and only take the others from feeds.png, and also add a comment there why we need to do it that way.

Comment 22

[Stefan [:stefanh]]

(In reply to comment #21)
> and only take the others from feeds.png, and also add a comment
> there why we need to do it that way.

That would mean something like this, I suppose:

#feedsMenu[disabled="true"] {
  list-style-image: url("chrome://communicator/skin/btn1/feeds.png");
  -moz-image-region: rect(32px 16px 48px 0px);
}
etc

Since list-style-image is understood by the native menu, you'll most likely have a garbled icon in the menu if the first thing you do is visiting a page with no feeds...

Comment 23

[Robert Kaiser]

OK, didn't know it would load the disabled icon in that case and never update to the correct one. In this case, I vote for adding !important rules pointing to feedIcon16.png and setting |-moz-image-region: auto| in a Mac-specific overlay CSS. (The second rule is for not breaking in the case when bug 418774 will ever be solved.)

Comment 25

[Stefan [:stefanh]]

Should be fixed by Benjamin's patch in bug 418774.