Closed Bug 969904 Opened 6 years ago Closed 6 years ago

Update Bookmarks Star Icons for Consistency

Categories

(Firefox :: Bookmarks & History, defect)

x86
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified

People

(Reporter: fx4waldi, Assigned: shorlander)

References

Details

(Whiteboard: [Australis:P4+])

Attachments

(1 file, 3 obsolete files)

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
Depends on: 931343
Stephen, Do you think this is needed ?
Flags: needinfo?(shorlander)
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.
Assignee: nobody → shorlander
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(shorlander)
Summary: bookmarks-notification-finish should not be blue for some systems → Update Bookmarks Star Icons for Consistency
P5, but should be a trivial image fix.
Whiteboard: [Australis:P5]
Duplicate of this bug: 970857
Patch updates Toolbar.png and menuPanel.png with new bookmark icons. Also updates bookmarks-notification-finish.png to more accurately match new star icons.
Attachment #8389434 - Flags: review?(mdeboer)
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)
Status: NEW → ASSIGNED
Flags: needinfo?(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.
Flags: needinfo?(shorlander)
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.
Attachment #8389434 - Attachment is obsolete: true
Attachment #8390210 - Flags: review?(mdeboer)
Comment on attachment 8390210 [details] [diff] [review]
update-bookmark-star-states.patch

Forgot to add the other stars again…
Attachment #8390210 - Attachment is obsolete: true
Attachment #8390210 - Flags: review?(mdeboer)
Think I got them all this time. For real.
Attachment #8390461 - Flags: review?(mdeboer)
Whiteboard: [Australis:P5] → [Australis:P4+]
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.
Attachment #8390461 - Flags: review?(mdeboer) → feedback+
Updated patch to fix flipped menuPanel find icons on Windows. Also fixed the selected list item rule.
Attachment #8390461 - Attachment is obsolete: true
Attachment #8390491 - Flags: review?(mdeboer)
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.
Attachment #8390491 - Flags: review?(mdeboer) → review+
Landed as: https://hg.mozilla.org/integration/fx-team/rev/02be46ac2b55
Whiteboard: [Australis:P4+] → [Australis:P4+][fixed-in-fx-team]
(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.
Flags: needinfo?(mdeboer)
(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.
Flags: needinfo?(mdeboer)
https://hg.mozilla.org/mozilla-central/rev/02be46ac2b55
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P4+][fixed-in-fx-team] → [Australis:P4+]
Target Milestone: --- → Firefox 30
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
Attachment #8390491 - Flags: approval-mozilla-aurora?
Attachment #8390491 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
The "Unsorted Bookmarks"-icons in win and linux also have a yellow star.
Depends on: 983721
Depends on: 978491
Keywords: verifyme
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.