Bookmarks toolbar dropzone needs new icon

VERIFIED FIXED in Firefox 57

Status

()

P1
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: abenson, Assigned: Towkir)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified, firefox58 verified)

Details

(Whiteboard: [reserve-photon-visual])

Attachments

(3 attachments, 2 obsolete attachments)

Whiteboard: [photon-visual] → [photon-visual] [triage]
Blocks: 1325171
Component: Toolbars and Customization → Theme
Whiteboard: [photon-visual] [triage] → [reserve-photon-visual][p4]
Priority: -- → P4
Whiteboard: [reserve-photon-visual][p4] → [reserve-photon-visual]

Updated

a year ago
Duplicate of this bug: 1397892
Flags: qe-verify?
(Assignee)

Comment 2

a year ago
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
Flags: needinfo?(abenson)
(Assignee)

Comment 3

a year ago
Created attachment 8911378 [details] [diff] [review]
bookmark-toolbar-dropzone-icon.patch

Please have a look at this.
Hope this helps !
Assignee: nobody → 3ugzilla
Status: NEW → ASSIGNED
Attachment #8911378 - Flags: review?(dao+bmo)
Priority: P4 → P1
(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
Flags: needinfo?(abenson)
(Assignee)

Comment 5

a year ago
Created attachment 8911527 [details] [diff] [review]
bookmark-toolbar-dropzone-icon.patch

Updated as you suggested !
Attachment #8911378 - Attachment is obsolete: true
Attachment #8911378 - Flags: review?(dao+bmo)
Attachment #8911527 - Flags: review?(dao+bmo)
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
(Assignee)

Comment 7

a year ago
Created attachment 8911536 [details] [diff] [review]
bookmarks-toolbar-dropzone-icon.patch
Attachment #8911527 - Attachment is obsolete: true
Attachment #8911527 - Flags: review?(dao+bmo)
Attachment #8911536 - Flags: review?(dao+bmo)
Comment on attachment 8911536 [details] [diff] [review]
bookmarks-toolbar-dropzone-icon.patch

Looks good! Thanks!
Attachment #8911536 - Flags: review?(dao+bmo) → review+

Comment 9

a year ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e507ee142c64
Bookmarks toolbar dropzone icon updated; r=dao
https://hg.mozilla.org/mozilla-central/rev/e507ee142c64
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Flags: qe-verify? → qe-verify+
QA Contact: ovidiu.boca
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).
(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.
Summary: Bookmarks toolbar dropzone needs new icon and styling → Bookmarks toolbar dropzone needs new 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]: /
Attachment #8911536 - Flags: approval-mozilla-beta?

Updated

a year ago
See Also: → bug 1393507
Comment on attachment 8911536 [details] [diff] [review]
bookmarks-toolbar-dropzone-icon.patch

Polishing photon, taking it.
Should be in 57b4, gtb tomorrow Thursday
Attachment #8911536 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 16

a year ago
Does this icon look black in Latest nightly dark theme too ?
status-firefox57: fixed → ---
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 17

a year ago
Oops
Sorry, didn't mean to touch the flag, got changed due to a mid air collision.
status-firefox57: --- → fixed
(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?
Flags: needinfo?(dao+bmo) → needinfo?(3ugzilla)
(Assignee)

Comment 19

a year ago
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.
Flags: needinfo?(3ugzilla)
(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"
Flags: needinfo?(3ugzilla)
(Assignee)

Updated

a year ago
See Also: → bug 1403875
(Assignee)

Comment 21

a year ago
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
Flags: needinfo?(3ugzilla)
Depends on: 1403875
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?
Flags: needinfo?(dao+bmo)
(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 ;)
Status: RESOLVED → VERIFIED
status-firefox58: fixed → verified
Flags: needinfo?(dao+bmo)
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.
status-firefox57: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.