In some systems, the blue star does not look good, and is not consistent with the theme. I think it would look much better if animated star it was yellow, like the starred48

Stephen, Do you think this is needed ?

I discussed this with madhava and stephen some days ago, the blue star in the animation is expected across platforms, what's wrong is the yellow star in some themes. Though I don't know if there's already a bug to change those graphics to the new version.

(In reply to Marco Bonardo [:mak] from comment #2) > I discussed this with madhava and stephen some days ago, the blue star in > the animation is expected across platforms, what's wrong is the yellow star > in some themes. Though I don't know if there's already a bug to change those > graphics to the new version. There is not AFAIK. We can just use this one.

P5, but should be a trivial image fix.

Patch updates Toolbar.png and menuPanel.png with new bookmark icons. Also updates bookmarks-notification-finish.png to more accurately match new star icons.

Also had to update the coordinates to show the right icon in the Menu Panel.

Stephen, what about star-icons.png and .*starred48.*\.png? Are those being handled elsewhere?

and bookmark.png in the windows theme (we should probably try to unify all of these star images into fewer files)

You should make sure this doesn't conflict with the new sidebar widget (bug 960198)

(In reply to Marco Bonardo [:mak] from comment #8) > Stephen, what about star-icons.png and .*starred48.*\.png? > Are those being handled elsewhere? I was going to file a follow-up and just get the stuff that's in primary UI updated. But I guess it makes sense to just go ahead and do it here :) (In reply to Marco Bonardo [:mak] from comment #9) > and bookmark.png in the windows theme (we should probably try to unify all > of these star images into fewer files) Ugh, thanks, forgot these. (In reply to Tim Nguyen [:ntim] from comment #10) > You should make sure this doesn't conflict with the new sidebar widget (bug > 960198) Thanks! Didn't realize that relanded.

You might want to wait for bug 978483 to be fixed first.

Updated starred48.png for all platforms and added starred48@2x.png for OS X.

Comment on attachment 8390210 [details] [diff] [review] update-bookmark-star-states.patch Forgot to add the other stars again…

Think I got them all this time. For real.

Comment on attachment 8390461 [details] [diff] [review] update-bookmark-star-states.patch Review of attachment 8390461 [details] [diff] [review]: ----------------------------------------------------------------- I know a new patch is coming up with an inverted Find icon for the Windows theme. - all the assets are nicely compressed and small. - various other improvements are made to the toolbar and menu-panel glyphs, which makes this a big win overall in contrast and sharper edges. - bookmark stars are nice & blue, across all platforms. So apart from the small CSS tweak, this should be ready to go! ::: browser/themes/windows/browser.css @@ +1314,5 @@ > } > > +richlistitem[selected="true"][current="true"] > .ac-title-box > .ac-result-type-bookmark, > +.autocomplete-treebody::-moz-tree-image(selected, current, bookmark, treecolAutoCompleteImage) { > + list-style-image: url("chrome://browser/skin/places/bookmark.png"); Can you move this rule just below the previous one that targets bookmarks? Also, I think you can remove this list-style-image property, because it'll already be set and correct; only the coordinates are needed.

Updated patch to fix flipped menuPanel find icons on Windows. Also fixed the selected list item rule.

Comment on attachment 8390491 [details] [diff] [review] update-bookmark-star-states.patch Review of attachment 8390491 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! I'll push this big update for you.

Landed as: https://hg.mozilla.org/integration/fx-team/rev/02be46ac2b55

(In reply to Mike de Boer [:mikedeboer] from comment #19) > Landed as: https://hg.mozilla.org/integration/fx-team/rev/02be46ac2b55 I see the patch adds a menuPanel-aero.png , but not sure if it's used anywhere in the CSS. I'm guessing the menuPanel.png is for Windows 8+ and menuPanel-aero.png for Windows 7, Vista and XP.

(In reply to Tim Nguyen [:ntim] from comment #20) > (In reply to Mike de Boer [:mikedeboer] from comment #19) > > Landed as: https://hg.mozilla.org/integration/fx-team/rev/02be46ac2b55 > > I see the patch adds a menuPanel-aero.png , but not sure if it's used > anywhere in the CSS. > I'm guessing the menuPanel.png is for Windows 8+ and menuPanel-aero.png for > Windows 7, Vista and XP. Nevermind, ignore the comment.

Comment on attachment 8390491 [details] [diff] [review] update-bookmark-star-states.patch [Approval Request Comment] Bug caused by (feature/regressing bug #): Australis User impact if declined: odd set of bookmarks icons Testing completed (on m-c, etc.): on m-c Risk to taking this patch (and alternatives if risky): low, just an asset swap String or IDL/UUID changes made by this patch: none

The "Unsorted Bookmarks"-icons in win and linux also have a yellow star.

Verified as fixed on Firefox 29 RC and latest Aurora using Windows 7 64bit, Ubuntu 14.04 32bit and Mac OS X 10.9.2.