1394933 - Bookmarks toolbar dropzone needs new icon

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Aaron Benson]

Attachment: bookmark-toolbar-16.svg

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

Comment 2

[[:Towkir] Ahmed]

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

Comment 3

[[:Towkir] Ahmed]

Attachment: bookmark-toolbar-dropzone-icon.patch

Please have a look at this.
Hope this helps !

Comment 4

[Dão Gottwald [:dao]]

(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

Comment 5

[[:Towkir] Ahmed]

Attachment: bookmark-toolbar-dropzone-icon.patch

Updated as you suggested !

Comment 6

[Dão Gottwald [:dao]]

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 7

[[:Towkir] Ahmed]

Attachment: bookmarks-toolbar-dropzone-icon.patch

Comment 8

[Dão Gottwald [:dao]]

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

Looks good! Thanks!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e507ee142c64
Bookmarks toolbar dropzone icon updated; r=dao

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e507ee142c64

Comment 11

[Sören Hentzschel]

Attachment: 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).

Comment 12

[Dão Gottwald [:dao]]

(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 13

[Dão Gottwald [:dao]]

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 14

[Sylvestre Ledru [:Sylvestre]]

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

Polishing photon, taking it.
Should be in 57b4, gtb tomorrow Thursday

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/af868f36e4b6

Comment 16

[[:Towkir] Ahmed]

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

Comment 17

[[:Towkir] Ahmed]

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

Comment 18

[Dão Gottwald [:dao]]

(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?

Comment 19

[[:Towkir] Ahmed]

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.

Comment 20

[Dão Gottwald [:dao]]

(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"

Comment 21

[[:Towkir] Ahmed]

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

Comment 22

[Valentina Claudia Ona]

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?

Comment 23

[Dão Gottwald [:dao]]

(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 ;)

Comment 24

[Valentina Claudia Ona]

Filed bug 1403905

Comment 25

[Ciprian Georgiu, Desktop QA]

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.