Bookmarks toolbar dropzone needs new icon

VERIFIED FIXED in Firefox 57

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years 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]
Component: Toolbars and Customization → Theme
Whiteboard: [photon-visual] [triage] → [reserve-photon-visual][p4]
Priority: -- → P4
Whiteboard: [reserve-photon-visual][p4] → [reserve-photon-visual]
Duplicate of this bug: 1397892
Flags: qe-verify?
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)
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)
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
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+
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
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Flags: qe-verify? → qe-verify+
QA Contact: ovidiu.boca
Posted image 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?
See Also: → 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+
Does this icon look black in Latest nightly dark theme too ?
Flags: needinfo?(dao+bmo)
Oops
Sorry, didn't mean to touch the flag, got changed due to a mid air collision.
(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)
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)
See Also: → 1403875
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
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.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.