Spec here: https://mozilla.invisionapp.com/share/FKD366GDM#/248932284_Customize_-_Toolbar_On

Hey guys, Can I take this one ? I guess the one currently in use is [0] For the fix, is it a good idea to just put the svg code from the attached icon in here [1] [0] https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/icons/bookmark-hollow.svg [1] https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/icons/bookmark-hollow.svg#4-6

Please have a look at this. Hope this helps !

(In reply to [:Towkir] Ahmed from comment #2) > Hey guys, Can I take this one ? > I guess the one currently in use is [0] > > For the fix, is it a good idea to just put the svg code from the attached > icon in here [1] > > [0] > https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/icons/ > bookmark-hollow.svg > [1] > https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/icons/ > bookmark-hollow.svg#4-6 bookmark-hollow.svg is used in other places where we don't want to change the icon. Instead I think we should update and use toolbar.svg here and rename it to bookmarks-toolbar.svg: http://searchfox.org/mozilla-central/rev/56ad02e34d0d36ca4d5ccaa885d26aff270b8ff7/browser/themes/shared/menupanel.inc.css#100

Updated as you suggested !

Comment on attachment 8911527 [details] [diff] [review] bookmark-toolbar-dropzone-icon.patch >--- a/browser/themes/shared/toolbarbutton-icons.inc.css >+++ b/browser/themes/shared/toolbarbutton-icons.inc.css >@@ -186,7 +186,7 @@ toolbar[brighttext] { > > #bookmarks-toolbar-button, > #bookmarks-toolbar-placeholder { >- list-style-image: url("chrome://browser/skin/bookmark-hollow.svg"); >+ list-style-image: url("chrome://browser/skin/toolbar.svg"); Should be bookmarks-toolbar.svg You'll also need to update this rule: http://searchfox.org/mozilla-central/rev/56ad02e34d0d36ca4d5ccaa885d26aff270b8ff7/browser/themes/shared/menupanel.inc.css#100

Comment on attachment 8911536 [details] [diff] [review] bookmarks-toolbar-dropzone-icon.patch Looks good! Thanks!

Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e507ee142c64 Bookmarks toolbar dropzone icon updated; r=dao

The title of this bug is "Bookmarks toolbar dropzone needs new icon and styling" and there is a new icon but the styling is still wrong, it doesn't match the spec on macOS (no space between icon and text, wrong height / padding of the dropzone).

(In reply to Sören Hentzschel from comment #11) > Created attachment 8912339 [details] > screenshot > > The title of this bug is "Bookmarks toolbar dropzone needs new icon and > styling" and there is a new icon but the styling is still wrong, it doesn't > match the spec on macOS (no space between icon and text, wrong height / > padding of the dropzone). The height is intentional for now (bug 1395596). Please file a new bug for the text being too close to the icon.

Comment on attachment 8911536 [details] [diff] [review] bookmarks-toolbar-dropzone-icon.patch Approval Request Comment [Feature/Bug causing the regression]: photon-visual polish [User impact if declined]: no big impact [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: not yet [Needs manual test from QE? If yes, steps to reproduce]: enable the bookmarks toolbar and enter customize mode [List of other uplifts needed for the feature/fix]: / [Is the change risky?]: no [Why is the change risky/not risky?]: straightforward icon swap [String changes made/needed]: /

Comment on attachment 8911536 [details] [diff] [review] bookmarks-toolbar-dropzone-icon.patch Polishing photon, taking it. Should be in 57b4, gtb tomorrow Thursday

Does this icon look black in Latest nightly dark theme too ?

Oops Sorry, didn't mean to touch the flag, got changed due to a mid air collision.

(In reply to [:Towkir] Ahmed from comment #16) > Does this icon look black in Latest nightly dark theme too ? Looks like you accidentally removed fill="context-fill" fill-opacity="context-fill-opacity" from the SVG. Could you please file a new bug?

Hey Dao, As far as I remember I did not touch the svg code :) Looks like we got more of these fill, and fill opacity issues on new icons. Does this relate to the same svg issue ? https://bugzilla.mozilla.org/show_bug.cgi?id=1363028#c44 If so, how about we collect as much icons as possible (which are missing these properties) and fix them in one bug, or specific section's icons per bug.

(In reply to [:Towkir] Ahmed from comment #19) > Hey Dao, > As far as I remember I did not touch the svg code :) Open your patch and look for fill="context-fill" fill-opacity="context-fill-opacity"

My bad. I thought you mentioned the icon attached to this bug. (attachment 8902403 [details]) Not so good at svg, so could not really compare the difference earlier. Sorry for messing that up. Filed bug 1403875 Cheers

I verified the icon of bookmarks toolbar dropzone using Nightly 58.0a1 on Windows 10 x64, Windows 7 x64, Ubuntu 16.04 and Mac OS X 10.12 with Build ID 20170927100120. Based on the fact that a new bug has been opened for icon color issue 1403875, can I mark this bug as a verified fixed? Who will log the bug for new icon styling?

(In reply to Valentina Claudia Ona from comment #22) > Who will log the bug for new icon styling? Whoever feels like it's an important enough issue ;)

Filed bug 1403905

I have also verified this bug on 57.0b7 (20171009192146) on the following OSes: Win 10 x64, Mac OS X 10.9 and Ubuntu 16.04 x64 LTS.