They should have the same color as other toolbar icons, see http://design.firefox.com/people/shorlander/photon/Mockups/windows-10.html. They problem seems to be that the icons don't support context-fill-opacity.

Is this one similar to this problem ? https://bugzilla.mozilla.org/show_bug.cgi?id=1394933#c18

Yeah, similar, but not the same. It's all about context-fill and context-fill-opacity :) If you want to solve those bugs, I'm happy to help.

Adding a screenshot to clarify the bug. Top is the current toolbar, bottom is the mockup. fwiw, I think this is more a P2, the icons look ugly on Win10 and are eye-catching due to the bold border. Surely we can survive it, but releasing a shiny new polished theme with unpolished icons doesn't sound great. On the other side the bookmarks views are all hidden by default.

fwiw, also the bookmarks toolbar, bookmarks menu and Other bookmarks icons are over contrasted compared to the mockups (check the bookmarks sidebar)

Preparing a patch immediately

Hey, Just checked the codes and looks like the mockup icon[0] and the one existing in the codebase[1] have different color fill. Am I right ? or does it matter ? [0] view-source:http://design.firefox.com/people/shorlander/photon/Mockups/images-general/icon-sidebar-folder-16.svg [1] https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/places/folder.svg

I don't know the Photon workflow very well (I worked mostly on the backend and Address bar parts) so I'm leaving this to Johann. Usually the best persons to answer these questions are UX team members.

Hi everyone. BTW according to this guide (Folder-16) that icon should be different http://design.firefox.com/icons/viewer/ Also it is more "Photon style" than the one currently in Nightly. So which one is actually the correct one...

(In reply to [:Towkir] Ahmed from comment #6) > Hey, Just checked the codes and looks like the mockup icon[0] and the one > existing in the codebase[1] have different color fill. Am I right ? or does > it matter ? > > [0] > view-source:http://design.firefox.com/people/shorlander/photon/Mockups/ > images-general/icon-sidebar-folder-16.svg > [1] > https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/places/ > folder.svg That's true, but it uses filter: invert(100%) for dark mode and I think we'd like to avoid that. We might not match the icon colors from the mockup exactly, but we should be able to get a little closer through applying context-fill-opacity. It would be interesting to know how that would look on the folder.svg icon for light and dark toolbar backgrounds. Can you try it? :)

(In reply to Serge from comment #8) > Hi everyone. > BTW according to this guide (Folder-16) that icon should be different > http://design.firefox.com/icons/viewer/ > Also it is more "Photon style" than the one currently in Nightly. > > So which one is actually the correct one... The one that's in your bookmarks toolbar right now. See bug 1363028 comment 31.

(In reply to Johann Hofmann [:johannh] from comment #9) > It would be interesting to know how that would look on > the folder.svg icon for light and dark toolbar backgrounds. Can you try it? :) In fact I did already, but didn't really see any difference with my bare eyes. Can you check if I did it correctly ? Not attaching as a patch because it's not complete yet. some icons might not need this (in this bug) or some may. here are my changes : https://pastebin.com/sJ1fCK2j You can check it yourself by applying it locally. Let me know what happens.

(In reply to [:Towkir] Ahmed from comment #11) > (In reply to Johann Hofmann [:johannh] from comment #9) > > It would be interesting to know how that would look on > > the folder.svg icon for light and dark toolbar backgrounds. Can you try it? :) > In fact I did already, but didn't really see any difference with my bare > eyes. > Can you check if I did it correctly ? > Not attaching as a patch because it's not complete yet. some icons might not > need this (in this bug) or some may. > here are my changes : https://pastebin.com/sJ1fCK2j > You can check it yourself by applying it locally. > Let me know what happens. I think that's what we want. The color of the icons is toned down a little by the extra opacity, so that they now have the same grey as the other toolbar buttons, if I'm not mistaken. I just took a cursory look (the patch didn't apply cleanly), feel free to submit a patch.

This one applies cleanly. Please have a look.

it looks slightly better, but it's still far from the official mockup. More specifically, the border of the folders is still too bold, epecially at the bottom.

from top to bottom: current Nightly, the patch, the mockup

(In reply to Marco Bonardo [::mak] from comment #14) > it looks slightly better, but it's still far from the official mockup. More > specifically, the border of the folders is still too bold, epecially at the > bottom. From an IRC conversation with shorlander it turns out that this is an issue with the SVG file, that we will fix in bug 1385519. I don't think the issue is large enough to block this bug from moving forward.

I agree, the current patch is already an improvement. Though, bug 1385519 doesn't contain any comment about the need to further update the folder icons too, so it would be nice if you could report the result of your conversation there, or it could get lost into nothing. I'm still not sure why these bugs are considered P4, while the bookmarking UI is not visible by default, it is quite used yet and it looks so ugly I'm not sure how we could accept ship 57 with the de-facto situation.

Comment on attachment 8914424 [details] [diff] [review] bookmark-toolbar-icons-fill.patch Review of attachment 8914424 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thank you! ::: browser/themes/shared/places/folder-live.svg @@ +5,1 @@ > <path fill="context-fill" d="M3.5,10A2.5,2.5,0,1,0,6,12.5,2.5,2.5,0,0,0,3.5,10ZM2,1A1,1,0,0,0,2,3,10.883,10.883,0,0,1,13,14a1,1,0,0,0,2,0A12.862,12.862,0,0,0,2,1ZM2,5A1,1,0,0,0,2,7a6.926,6.926,0,0,1,7,7,1,1,0,0,0,2,0A8.9,8.9,0,0,0,2,5Z"/> Nit: you can remove fill=context-fill here

(In reply to Marco Bonardo [::mak] from comment #17) > I agree, the current patch is already an improvement. Though, bug 1385519 > doesn't contain any comment about the need to further update the folder > icons too, so it would be nice if you could report the result of your > conversation there, or it could get lost into nothing. Added a comment, thanks!

Updated !

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bafdf2e14d4e Fixed fill and fill-opacity of Bookmark toolbar icons. r=johannh

Comment on attachment 8915434 [details] [diff] [review] bookmark-toolbar-icons-fill.patch Approval Request Comment [Feature/Bug causing the regression]: Bug 1363028 [User impact if declined]: Bookmark toolbar icons have too bright colors, which looks weird. [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]: No [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: No [Why is the change risky/not risky?]: It's a very simple CSS and SVG change to make these icons do what all other toolbarbuttons do already [String changes made/needed]: None

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0 Build ID: 20171006100327 This issue has been verified as fixed on latest Firefox Nightly Build ID: 20171006100327 on Windows 8.1 x64, Mac OS 10.12 and Ubuntu 14.04. The border of the folders is no longer bolded and the icons are slightly darker.

Comment on attachment 8915434 [details] [diff] [review] bookmark-toolbar-icons-fill.patch Photon polish, Beta57+

I verified this issue using Firefox Beta 57.0b9 with Build ID 20171016185129 on Windows 10 x64, Windows 7 x64, Mac OS X 10.12, Ubuntu 16.04. I will mark this as verified fixed.