Closed Bug 1363028 Opened 8 years ago Closed 7 years ago

Update bookmark toolbar icons

Categories

(Firefox :: Theme, enhancement, P1)

Product:
Firefox
Component:
Theme
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox57 --- verified
firefox58 --- verified

People

(Reporter: johannh, Assigned: Towkir)

References

(Blocks 1 open bug)

Details

(Whiteboard: [reserve-photon-visual][p3])

Attachments

(2 files, 3 obsolete files)

bookmark-icons.patch
(deleted), patch
johannh
: review+
Sylvestre
: approval-mozilla-beta+
Details | Diff | Splinter Review
Places Icons
(deleted), image/png
Details 
For Photon we want new set of bookmark toolbar buttons, see https://people-mozilla.org/~shorlander/projects/photon/Mockups/windows-10.html
Blocks: photon-visual
No longer blocks: 1355455
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
QA Contact: brindusa.tot
Whiteboard: [reserve-photon-visual][p4] → [reserve-photon-visual][p3]
Blocks: 1368580
Blocks: 1387705
Blocks: 1388676
Priority: P3 → P4
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
Flags: needinfo?(jhofmann)
(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!
Flags: needinfo?(jhofmann)
(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.
Assignee: nobody → 3ugzilla
Status: NEW → ASSIGNED
Priority: P4 → P1
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.
Blocks: 1398332
Assignee: 3ugzilla → dtownsend
Hi Dave, thanks for taking care of this. I was a bit busy these days.
Clearing the NI flag. :)
Cheers !
Flags: needinfo?(3ugzilla)
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
Attachment #8910518 - Flags: review?(jhofmann) → review-
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!
Flags: needinfo?(3ugzilla)
Attached patch bookmark-icons.patch (obsolete) (deleted) — — Splinter Review 
Please take a look and let me know if anything should be changed.
Assignee: dtownsend → 3ugzilla
Attachment #8910518 - Attachment is obsolete: true
Flags: needinfo?(3ugzilla)
Attachment #8910718 - Flags: review?(jhofmann)
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
Attachment #8910718 - Flags: review?(jhofmann) → feedback+
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.
Attached patch bookmark-icons.patch (obsolete) (deleted) — — Splinter Review 
Here is the updated patch !
Attachment #8910718 - Attachment is obsolete: true
Attachment #8911186 - Flags: review?(jhofmann)
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
Attachment #8911186 - Flags: review?(jhofmann)
(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.
Attached patch bookmark-icons.patch (deleted) — — Splinter Review 
Sorry about the jar files,
Hope this works now. !
Attachment #8911186 - Attachment is obsolete: true
Attachment #8911300 - Flags: review?(jhofmann)
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!
Flags: needinfo?(3ugzilla)
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
Flags: needinfo?(3ugzilla)
(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.
Attached image Places Icons (deleted) — 
(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
Flags: needinfo?(jhofmann)
I am a bit confused, still Stephen should decide.
http://design.firefox.com/icons/viewer/#folder
Flags: needinfo?(shorlander)
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.
Attachment #8911300 - Flags: review?(jhofmann) → review+
Flags: needinfo?(shorlander)
Flags: needinfo?(jhofmann)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd3e25fd615c
Update bookmark toolbar icons to photon styles. r=johannh
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cd3e25fd615c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
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
QA Whiteboard: [bugday-20170927]
Status: RESOLVED → VERIFIED
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
Attachment #8911300 - Flags: approval-mozilla-beta?
(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
Depends on: 1403851
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.
Flags: needinfo?(shorlander)
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
Attachment #8911300 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/995866fb3357
status-firefox57: --- → fixed
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.
status-firefox57: fixed → verified
status-firefox58: fixed → verified
Flags: qe-verify+
(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.
Flags: needinfo?(shorlander)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: