Closed
Bug 1394933
Opened 7 years ago
Closed 7 years ago
Bookmarks toolbar dropzone needs new icon
Categories
(Firefox :: Theme, defect, P1)
Firefox
Theme
Tracking
()
VERIFIED
FIXED
Firefox 58
People
(Reporter: abenson, Assigned: Towkir)
References
(Blocks 1 open bug)
Details
(Whiteboard: [reserve-photon-visual])
Attachments
(3 files, 2 obsolete files)
476 bytes,
image/svg+xml
|
Details | |
3.13 KB,
patch
|
dao
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
13.00 KB,
image/png
|
Details |
Updated•7 years ago
|
Whiteboard: [photon-visual] → [photon-visual] [triage]
Updated•7 years ago
|
Blocks: photon-visual
Component: Toolbars and Customization → Theme
Whiteboard: [photon-visual] [triage] → [reserve-photon-visual][p4]
Updated•7 years ago
|
Priority: -- → P4
Whiteboard: [reserve-photon-visual][p4] → [reserve-photon-visual]
Updated•7 years ago
|
Flags: qe-verify?
Assignee | ||
Comment 2•7 years 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•7 years ago
|
||
Please have a look at this.
Hope this helps !
Updated•7 years ago
|
Priority: P4 → P1
Comment 4•7 years ago
|
||
(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•7 years ago
|
||
Updated as you suggested !
Attachment #8911378 -
Attachment is obsolete: true
Attachment #8911378 -
Flags: review?(dao+bmo)
Attachment #8911527 -
Flags: review?(dao+bmo)
Comment 6•7 years ago
|
||
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•7 years ago
|
||
Attachment #8911527 -
Attachment is obsolete: true
Attachment #8911527 -
Flags: review?(dao+bmo)
Attachment #8911536 -
Flags: review?(dao+bmo)
Comment 8•7 years ago
|
||
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
Comment 10•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Updated•7 years ago
|
Flags: qe-verify? → qe-verify+
QA Contact: ovidiu.boca
Comment 11•7 years ago
|
||
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•7 years ago
|
||
(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 13•7 years ago
|
||
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?
Comment 14•7 years ago
|
||
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+
Comment 15•7 years ago
|
||
bugherder uplift |
status-firefox57:
--- → fixed
Assignee | ||
Comment 16•7 years ago
|
||
Does this icon look black in Latest nightly dark theme too ?
status-firefox57:
fixed → ---
Flags: needinfo?(dao+bmo)
Assignee | ||
Comment 17•7 years ago
|
||
Oops
Sorry, didn't mean to touch the flag, got changed due to a mid air collision.
status-firefox57:
--- → fixed
Comment 18•7 years ago
|
||
(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•7 years 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)
Comment 20•7 years ago
|
||
(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 | ||
Comment 21•7 years 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)
Comment 22•7 years ago
|
||
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)
Comment 23•7 years ago
|
||
(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•7 years ago
|
||
Filed bug 1403905
Comment 25•7 years ago
|
||
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.
Description
•