For Photon we want new set of bookmark toolbar buttons, see https://people-mozilla.org/~shorlander/projects/photon/Mockups/windows-10.html

Not sure if this is an easy one, if it us about updating the icons, I guess I can work on this if anyone is eager to mentor. Can you check the like you provided ? Seems like it does not work anymore. Thanks

(In reply to [:Towkir] Ahmed from comment #2) > Not sure if this is an easy one, if it us about updating the icons, I guess > I can work on this if anyone is eager to mentor. > Can you check the like you provided ? Seems like it does not work anymore. My bad, too many typo in one comment. Not sure if this is an easy one, if it is about updating the icons, I guess I can work on this if anyone is eager to mentor. Can you check the link you provided ? Seems like it does not work anymore.

(In reply to [:Towkir] Ahmed from comment #3) > (In reply to [:Towkir] Ahmed from comment #2) > > Not sure if this is an easy one, if it us about updating the icons, I guess > > I can work on this if anyone is eager to mentor. > > Can you check the like you provided ? Seems like it does not work anymore. > My bad, too many typo in one comment. > > Not sure if this is an easy one, if it is about updating the icons, I guess > I can work on this if anyone is eager to mentor. > Can you check the link you provided ? Seems like it does not work anymore. I don't think this would be too complicated, you'll just need to replace some icons and consolidate some that are currently cross-platform. You might want to use the browser toolbox to find the icons to replace. The mockup can be found here now: http://design.firefox.com/people/shorlander/photon/Mockups/windows-10.html Most of the new icons should already be in the tree as part of the bookmarks sidebar. If there's any you can't find you should probably needinfo :shorlander. Dão might be a better mentor for this going forward, he knows this stuff a bit better than me and I'll be on PTO soon. Thanks!

(In reply to Johann Hofmann [:johannh] from comment #4) > I don't think this would be too complicated, you'll just need to replace > some icons and consolidate some that are currently cross-platform. I meant that are currently *not* cross-platform.

Thanks for the instructions, assigning for now, I will submit a patch soon and will flag NI if I need something.

Any news on this ? It's not the most critical bug for 57, but it would really improve consistency.

The crushed height of the Bookmarks Toolbar is nearly unusable.

(In reply to Leman Bennett [Omega] from comment #8) > The crushed height of the Bookmarks Toolbar is nearly unusable. It was unfortunately caused by bug #1389966.

Hi Dave, thanks for taking care of this. I was a bit busy these days. Clearing the NI flag. :) Cheers !

Comment on attachment 8910518 [details] Bug 1363028: Update bookmark toolbar icons to match Photon styles. https://reviewboard.mozilla.org/r/181942/#review187292 I think we need to change the Linux icons as well. The mockup is using the new icons http://design.firefox.com/people/shorlander/photon/Mockups/linux.html and we got rid of the gtk icons in bug 1377011, too. When we do that, we should share the new icon rules in https://searchfox.org/mozilla-central/source/browser/themes/shared/toolbarbutton-icons.inc.css

Towkir, since you were interested in continuing this bug, would you like to finish it up now? Mossop covered all icons that need to get updated, I think. So we'll just need the changes I asked for in comment 12. Thank you!

Please take a look and let me know if anything should be changed.

It's really unfortune that it didn't land on 57 Nightly. Will be possible to uplift this patch to the early 57 beta cycle then? Icons are one of the most visible stuff in the browser and this will really make an "all new" Firefox 57 look much better and consistent.

Comment on attachment 8910718 [details] [diff] [review] bookmark-icons.patch Review of attachment 8910718 [details] [diff] [review]: ----------------------------------------------------------------- The code itself looks good, but you can remove some files now, see this try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=138da76ce9f5916306bdd8abfc82ed255a8c2ea8 (the browser_all_files_referenced.js test checks for files that are unreferenced in the tree). Please remove the files mentioned in the failures (some should be different across platforms) and trigger a new try run. It's quickest to use this command: ./mach try -b do -p linux64,macosx64,win32,win64 -u mochitest-bc,mochitest-e10s-bc -t none --and browser/base/content/test/static/ that should finish quite timely, so you could also let this run before doing another push to review :) Please let me know if you need help with any of this! ::: browser/themes/osx/browser.css @@ -178,4 @@ > -moz-box-align: center; > } > > -/* ----- BOOKMARK BUTTONS ----- */ You can remove this rule as well: https://searchfox.org/mozilla-central/rev/d08b24e613cac8c9c5a4131452459241010701e0/browser/themes/osx/browser.css#831

Note that the asset in bug 1402117 shows a different icon style. It can be updated in a follow-up (if so the folder icon should also be updated for the sidebar/library).

(In reply to Guillaume C. [:ge3k0s] from comment #19) > Note that the asset in bug 1402117 shows a different icon style. It can be > updated in a follow-up (if so the folder icon should also be updated for the > sidebar/library). Ah, thank you for noticing! I agree we should do it in a follow-up bug and we'd have to let Bryan and Stephen sort out which is the correct folder icon first.

Here is the updated patch !

Thanks! I'll do another try push and take a look at it later!

Comment on attachment 8911186 [details] [diff] [review] bookmark-icons.patch Review of attachment 8911186 [details] [diff] [review]: ----------------------------------------------------------------- Oh, right, please remove the files from their jar.mn files as well (you should get a build error otherwise), e.g. here: https://searchfox.org/mozilla-central/rev/15ce5cb2db0c85abbabe39a962b0e697c9ef098f/browser/themes/windows/jar.mn#50

(In reply to Johann Hofmann [:johannh] from comment #20) > (In reply to Guillaume C. [:ge3k0s] from comment #19) > > Note that the asset in bug 1402117 shows a different icon style. It can be > > updated in a follow-up (if so the folder icon should also be updated for the > > sidebar/library). > > Ah, thank you for noticing! I agree we should do it in a follow-up bug and > we'd have to let Bryan and Stephen sort out which is the correct folder icon > first. Matching the places icons (Sidebar, Library, etc) is correct. They are deliberately different.

Sorry about the jar files, Hope this works now. !

Towkir, since we have a decision from Stephen, would you like to include the icon from bug 1402117 in this patch (for the bookmarks toolbar only)? The rest of the patch is looking good. Thanks!

Sure. Currently the icon being used for folder seems to be [0] Does the new one reside in codebase already ? If not, should I replace the old one ? or just add the new icon ? And if so, where? Thanks [0] https://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#192

(In reply to [:Towkir] Ahmed from comment #29) > Sure. > Currently the icon being used for folder seems to be [0] > > Does the new one reside in codebase already ? > If not, should I replace the old one ? or just add the new icon ? And if so, > where? > Thanks > > [0] > https://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser. > css#192 So I initially thought it didn't exist, but it turns out this was added with the downloads library view: chrome://browser/skin/folder.svg You should be able to set that as the list-image.

(In reply to Johann Hofmann [:johannh] from comment #28) > Towkir, since we have a decision from Stephen, would you like to include the > icon from bug 1402117 in this patch (for the bookmarks toolbar only)? > > The rest of the patch is looking good. > > Thanks! Sorry, to be clear, it should look like this

I am a bit confused, still Stephen should decide. http://design.firefox.com/icons/viewer/#folder

Comment on attachment 8911300 [details] [diff] [review] bookmark-icons.patch Review of attachment 8911300 [details] [diff] [review]: ----------------------------------------------------------------- It appears that I just misunderstood Stephen. The patch seems good now.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/cd3e25fd615c Update bookmark toolbar icons to photon styles. r=johannh

As previously asked in #c15 - might this be something to still be considered for uplift to 57.0b3/4?

I have reproduced this bug with Nightly 55.0a1 (2017-05-08) on Ubuntu 16.04, 64 bit! The fix is now verified on Latest Nightly 58.0a1. Build ID 20170926100259 User Agent Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0

Comment on attachment 8911300 [details] [diff] [review] bookmark-icons.patch Approval Request Comment [Feature/Bug causing the regression]: No bug [User impact if declined]: Outdated bookmark icons [Is this code covered by automated tests?]: No, we just updated the icons [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: Not really, the change is easy to see on the bookmarks toolbar [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: No [Why is the change risky/not risky?]: We replaced some icons in CSS. [String changes made/needed]: None

(In reply to Albert Scheiner [:alberts] from comment #36) > As previously asked in #c15 - might this be something to still be considered > for uplift to 57.0b3/4? Yup, thanks for the reminder.

aren't the new folder icons too bright with DARK THEME? i am always in dark theme and with this patch the bookmarks toolbar is unreadeable so i compared with the original design http://design.firefox.com/people/shorlander/photon/Mockups/linux.html and there is a big difference in color (quite white vs middle gray)

(In reply to Stefano Cecere from comment #40) > so i compared with the original design > http://design.firefox.com/people/shorlander/photon/Mockups/linux.html > and there is a big difference in color (quite white vs middle gray) You are right, also the Windows folders are completely different from http://design.firefox.com/people/shorlander/photon/Mockups/windows-10.html and they look quite bad (normal theme, didn't try the dark one).

(In reply to Stefano Cecere from comment #40) > and there is a big difference in color (quite white vs middle gray) It seems like it is the border around the icons that is different. I checked macOS/Win10. - dark theme: white border instead of light gray - light/default theme: border is a bit darker (though not as notable as the dark theme's difference) Regarding the Win10 mockup, the folder is turned to the side. In Nightly Win10 contrary to the mockup it is styled like for Linux and macOS - which I guess is intentional!?

(In reply to Albert Scheiner [:alberts] from comment #42) > > and there is a big difference in color (quite white vs middle gray) yes i was referring to the border.. i tried to correct my comment but it seems impassible to edit a submitted comment right? .. as i couldn't upload a screenshot/attachment to show the problem here, i tweeted it to @FirefoxNightly https://twitter.com/StefanoCecere/status/913173548784594945

The problem seems to be that bookmark toolbar icons (including the new SVGs) don't support context-fill-opacity. I filed bug 1403851 to track adding that. Thanks for reporting the issue!

To be clear, I'd still like to uplift this to beta, since the issue is only minor and the new icons would be nice to have.

(In reply to Albert Scheiner [:alberts] from comment #42) > (In reply to Stefano Cecere from comment #40) > > and there is a big difference in color (quite white vs middle gray) > > It seems like it is the border around the icons that is different. I checked > macOS/Win10. > - dark theme: white border instead of light gray > - light/default theme: border is a bit darker (though not as notable as the > dark theme's difference) > > Regarding the Win10 mockup, the folder is turned to the side. In Nightly > Win10 contrary to the mockup it is styled like for Linux and macOS - which I > guess is intentional!? I think the icons on Windows are intentionally different from Linux/MacOS. They still need to be updated on Windows.

(In reply to Guillaume C. [:ge3k0s] from comment #46) > (In reply to Albert Scheiner [:alberts] from comment #42) > > (In reply to Stefano Cecere from comment #40) > > > and there is a big difference in color (quite white vs middle gray) > > > > It seems like it is the border around the icons that is different. I checked > > macOS/Win10. > > - dark theme: white border instead of light gray > > - light/default theme: border is a bit darker (though not as notable as the > > dark theme's difference) > > > > Regarding the Win10 mockup, the folder is turned to the side. In Nightly > > Win10 contrary to the mockup it is styled like for Linux and macOS - which I > > guess is intentional!? > > I think the icons on Windows are intentionally different from Linux/MacOS. > They still need to be updated on Windows. Ah! Are they different in the sidebar? Can you please file a follow-up bug? Thanks!

(In reply to Johann Hofmann [:johannh] from comment #47) > (In reply to Guillaume C. [:ge3k0s] from comment #46) > > (In reply to Albert Scheiner [:alberts] from comment #42) > > > (In reply to Stefano Cecere from comment #40) > > > > and there is a big difference in color (quite white vs middle gray) > > > > > > It seems like it is the border around the icons that is different. I checked > > > macOS/Win10. > > > - dark theme: white border instead of light gray > > > - light/default theme: border is a bit darker (though not as notable as the > > > dark theme's difference) > > > > > > Regarding the Win10 mockup, the folder is turned to the side. In Nightly > > > Win10 contrary to the mockup it is styled like for Linux and macOS - which I > > > guess is intentional!? > > > > I think the icons on Windows are intentionally different from Linux/MacOS. > > They still need to be updated on Windows. > > Ah! Are they different in the sidebar? Can you please file a follow-up bug? > Thanks! Ah my bad. The icons used in the sidebar on Windows are currently the same than Linux/MacOs, but they diverge from the Windows mockup. Surely Shorlander knows better than me.

I bet the folder icon is turned that way on the Windows mock up because folder icons on Windows Explorer are usually turned that way.

Comment on attachment 8911300 [details] [diff] [review] bookmark-icons.patch Update of the icons, taking it. Should be in 57b4, gtb later today

uhm.. the 57v4 just arrived, but has the folder icons with the white border! but i guess this problem is getting managed at https://bugzilla.mozilla.org/show_bug.cgi?id=1403851, right?

Reproduced this issue using an affected old Nightly build. This is verified fixed on 56.0b7 (20171005195903) under Windows 10 x64, Mac OS X 10.11 and Ubuntu 16.04 x64. Marking verified fixed on 58.01 as well per comment 37.

(In reply to Guillaume C. [:ge3k0s] from comment #48) > (In reply to Johann Hofmann [:johannh] from comment #47) > > (In reply to Guillaume C. [:ge3k0s] from comment #46) > > > (In reply to Albert Scheiner [:alberts] from comment #42) > > > > (In reply to Stefano Cecere from comment #40) > > > > > and there is a big difference in color (quite white vs middle gray) > > > > > > > > It seems like it is the border around the icons that is different. I checked > > > > macOS/Win10. > > > > - dark theme: white border instead of light gray > > > > - light/default theme: border is a bit darker (though not as notable as the > > > > dark theme's difference) > > > > > > > > Regarding the Win10 mockup, the folder is turned to the side. In Nightly > > > > Win10 contrary to the mockup it is styled like for Linux and macOS - which I > > > > guess is intentional!? > > > > > > I think the icons on Windows are intentionally different from Linux/MacOS. > > > They still need to be updated on Windows. > > > > Ah! Are they different in the sidebar? Can you please file a follow-up bug? > > Thanks! > > Ah my bad. The icons used in the sidebar on Windows are currently the same > than Linux/MacOs, but they diverge from the Windows mockup. Surely > Shorlander knows better than me. Planning on fixing most of the issues in bug 1385519.