Closed Bug 1394933 Opened 7 years ago Closed 7 years ago

Bookmarks toolbar dropzone needs new icon

Categories

(Firefox :: Theme, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 58
Tracking Status
firefox57 --- verified
firefox58 --- verified

People

(Reporter: abenson, Assigned: Towkir)

References

(Blocks 1 open bug)

Details

(Whiteboard: [reserve-photon-visual])

Attachments

(3 files, 2 obsolete files)

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]
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Flags: qe-verify? → qe-verify+
QA Contact: ovidiu.boca
Attached 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.

Attachment

General

Creator:
Created:
Updated:
Size: