969904 - Update Bookmarks Star Icons for Consistency

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[fx4waldi]

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

Comment 1

[Tim Nguyen :ntim]

Stephen, Do you think this is needed ?

Comment 2

[Marco Bonardo [:mak]]

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.

Comment 3

[Stephen Horlander [:shorlander]]

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

Comment 4

[Justin Dolske [:Dolske]]

P5, but should be a trivial image fix.

Comment 6

[Stephen Horlander [:shorlander]]

Attachment: update-bookmark-star-states.patch

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

Comment 7

[Stephen Horlander [:shorlander]]

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

Comment 8

[Marco Bonardo [:mak]]

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

Comment 9

[Marco Bonardo [:mak]]

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

Comment 10

[Tim Nguyen :ntim]

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

Comment 11

[Stephen Horlander [:shorlander]]

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

Comment 12

[Tim Nguyen :ntim]

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

Comment 13

[Stephen Horlander [:shorlander]]

Attachment: update-bookmark-star-states.patch

Updated starred48.png for all platforms and added [e-mail] for OS X.

Comment 14

[Stephen Horlander [:shorlander]]

Comment on attachment 8390210 [details] [diff] [review]
update-bookmark-star-states.patch

Forgot to add the other stars again…

Comment 15

[Stephen Horlander [:shorlander]]

Attachment: update-bookmark-star-states.patch

Think I got them all this time. For real.

Comment 16

[Mike de Boer [:mikedeboer]]

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.

Comment 17

[Stephen Horlander [:shorlander]]

Attachment: update-bookmark-star-states.patch

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

Comment 18

[Mike de Boer [:mikedeboer]]

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.

Comment 19

[Mike de Boer [:mikedeboer]]

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

Comment 20

[Tim Nguyen :ntim]

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

Comment 21

[Tim Nguyen :ntim]

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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/02be46ac2b55

Comment 23

[:Gijs (he/him)]

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

Comment 24

[Elbart]

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

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4887c80c2f38

Comment 26

[Bogdan Maris, Desktop QA]

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.