Bookmark toolbar icons are too bright in dark theme

VERIFIED FIXED in Firefox 57

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: johannh, Assigned: Towkir)

Tracking

(Blocks 1 bug)

57 Branch
Firefox 58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified, firefox58 verified)

Details

(Whiteboard: [reserve-photon-visual])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
They should have the same color as other toolbar icons, see http://design.firefox.com/people/shorlander/photon/Mockups/windows-10.html.

They problem seems to be that the icons don't support context-fill-opacity.
Flags: qe-verify+
(Assignee)

Comment 1

2 years ago
Is this one similar to this problem ?
https://bugzilla.mozilla.org/show_bug.cgi?id=1394933#c18
Flags: needinfo?(jhofmann)
(Reporter)

Comment 2

2 years ago
Yeah, similar, but not the same. It's all about context-fill and context-fill-opacity :)

If you want to solve those bugs, I'm happy to help.
Flags: needinfo?(jhofmann)
Adding a screenshot to clarify the bug. Top is the current toolbar, bottom is the mockup.

fwiw, I think this is more a P2, the icons look ugly on Win10 and are eye-catching due to the bold border. Surely we can survive it, but releasing a shiny new polished theme with unpolished icons doesn't sound great. On the other side the bookmarks views are all hidden by default.
fwiw, also the bookmarks toolbar, bookmarks menu and Other bookmarks icons are over contrasted compared to the mockups (check the bookmarks sidebar)
(Assignee)

Comment 5

2 years ago
Preparing a patch immediately
Assignee: nobody → 3ugzilla
Status: NEW → ASSIGNED
(Assignee)

Comment 6

2 years ago
Hey, Just checked the codes and looks like the mockup icon[0] and the one existing in the codebase[1] have different color fill. Am I right ? or does it matter ?

[0] view-source:http://design.firefox.com/people/shorlander/photon/Mockups/images-general/icon-sidebar-folder-16.svg
[1] https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/places/folder.svg
Flags: needinfo?(mak77)
Flags: needinfo?(jhofmann)
I don't know the Photon workflow very well (I worked mostly on the backend and Address bar parts) so I'm leaving this to Johann. Usually the best persons to answer these questions are UX team members.
Flags: needinfo?(mak77)

Comment 8

2 years ago
Hi everyone.
BTW according to this guide (Folder-16) that icon should be different
http://design.firefox.com/icons/viewer/
Also it is more "Photon style" than the one currently in Nightly.

So which one is actually the correct one...
(Reporter)

Comment 9

2 years ago
(In reply to [:Towkir] Ahmed from comment #6)
> Hey, Just checked the codes and looks like the mockup icon[0] and the one
> existing in the codebase[1] have different color fill. Am I right ? or does
> it matter ?
> 
> [0]
> view-source:http://design.firefox.com/people/shorlander/photon/Mockups/
> images-general/icon-sidebar-folder-16.svg
> [1]
> https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/places/
> folder.svg

That's true, but it uses filter: invert(100%) for dark mode and I think we'd like to avoid that. We might not match the icon colors from the mockup exactly, but we should be able to get a little closer through applying context-fill-opacity. It would be interesting to know how that would look on the folder.svg icon for light and dark toolbar backgrounds. Can you try it? :)
Flags: needinfo?(jhofmann)
(Reporter)

Comment 10

2 years ago
(In reply to Serge from comment #8)
> Hi everyone.
> BTW according to this guide (Folder-16) that icon should be different
> http://design.firefox.com/icons/viewer/
> Also it is more "Photon style" than the one currently in Nightly.
> 
> So which one is actually the correct one...

The one that's in your bookmarks toolbar right now. See bug 1363028 comment 31.
Priority: P3 → P4
(Assignee)

Comment 11

2 years ago
(In reply to Johann Hofmann [:johannh] from comment #9)
> It would be interesting to know how that would look on
> the folder.svg icon for light and dark toolbar backgrounds. Can you try it? :)
In fact I did already, but didn't really see any difference with my bare eyes. 
Can you check if I did it correctly ?
Not attaching as a patch because it's not complete yet. some icons might not need this (in this bug) or some may.
here are my changes : https://pastebin.com/sJ1fCK2j
You can check it yourself by applying it locally.
Let me know what happens.
(Assignee)

Updated

2 years ago
Flags: needinfo?(jhofmann)
Priority: P4 → P1
(Reporter)

Comment 12

2 years ago
(In reply to [:Towkir] Ahmed from comment #11)
> (In reply to Johann Hofmann [:johannh] from comment #9)
> > It would be interesting to know how that would look on
> > the folder.svg icon for light and dark toolbar backgrounds. Can you try it? :)
> In fact I did already, but didn't really see any difference with my bare
> eyes. 
> Can you check if I did it correctly ?
> Not attaching as a patch because it's not complete yet. some icons might not
> need this (in this bug) or some may.
> here are my changes : https://pastebin.com/sJ1fCK2j
> You can check it yourself by applying it locally.
> Let me know what happens.

I think that's what we want. The color of the icons is toned down a little by the extra opacity, so that they now have the same grey as the other toolbar buttons, if I'm not mistaken.

I just took a cursory look (the patch didn't apply cleanly), feel free to submit a patch.
Flags: needinfo?(jhofmann)
(Assignee)

Comment 13

2 years ago
This one applies cleanly. Please have a look.
Attachment #8914424 - Flags: review?(jhofmann)
it looks slightly better, but it's still far from the official mockup. More specifically, the border of the folders is still too bold, epecially at the bottom.
Posted image comparison.png
from top to bottom: current Nightly, the patch, the mockup
(Reporter)

Comment 16

2 years ago
(In reply to Marco Bonardo [::mak] from comment #14)
> it looks slightly better, but it's still far from the official mockup. More
> specifically, the border of the folders is still too bold, epecially at the
> bottom.

From an IRC conversation with shorlander it turns out that this is an issue with the SVG file, that we will fix in bug 1385519. I don't think the issue is large enough to block this bug from moving forward.
I agree, the current patch is already an improvement. Though, bug 1385519 doesn't contain any comment about the need to further update the folder icons too, so it would be nice if you could report the result of your conversation there, or it could get lost into nothing.
I'm still not sure why these bugs are considered P4, while the bookmarking UI is not visible by default, it is quite used yet and it looks so ugly I'm not sure how we could accept ship 57 with the de-facto situation.
(Reporter)

Comment 18

2 years ago
Comment on attachment 8914424 [details] [diff] [review]
bookmark-toolbar-icons-fill.patch

Review of attachment 8914424 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thank you!

::: browser/themes/shared/places/folder-live.svg
@@ +5,1 @@
>    <path fill="context-fill" d="M3.5,10A2.5,2.5,0,1,0,6,12.5,2.5,2.5,0,0,0,3.5,10ZM2,1A1,1,0,0,0,2,3,10.883,10.883,0,0,1,13,14a1,1,0,0,0,2,0A12.862,12.862,0,0,0,2,1ZM2,5A1,1,0,0,0,2,7a6.926,6.926,0,0,1,7,7,1,1,0,0,0,2,0A8.9,8.9,0,0,0,2,5Z"/>

Nit: you can remove fill=context-fill here
Attachment #8914424 - Flags: review?(jhofmann) → review+
(Reporter)

Comment 19

2 years ago
(In reply to Marco Bonardo [::mak] from comment #17)
> I agree, the current patch is already an improvement. Though, bug 1385519
> doesn't contain any comment about the need to further update the folder
> icons too, so it would be nice if you could report the result of your
> conversation there, or it could get lost into nothing.

Added a comment, thanks!
(Assignee)

Comment 20

2 years ago
Updated !
Attachment #8914424 - Attachment is obsolete: true
Attachment #8915434 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 21

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bafdf2e14d4e
Fixed fill and fill-opacity of Bookmark toolbar icons. r=johannh
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bafdf2e14d4e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Blocks: 1393507
(Reporter)

Comment 23

2 years ago
Comment on attachment 8915434 [details] [diff] [review]
bookmark-toolbar-icons-fill.patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1363028
[User impact if declined]: Bookmark toolbar icons have too bright colors, which looks weird.
[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]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's a very simple CSS and SVG change to make these icons do what all other toolbarbuttons do already
[String changes made/needed]: None
Attachment #8915434 - Flags: approval-mozilla-beta?
User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171006100327

This issue has been verified as fixed on latest Firefox Nightly Build ID: 20171006100327 on Windows 8.1 x64, Mac OS 10.12 and Ubuntu 14.04. The border of the folders is no longer bolded and the icons are slightly darker.
Status: RESOLVED → VERIFIED
Comment on attachment 8915434 [details] [diff] [review]
bookmark-toolbar-icons-fill.patch

Photon polish, Beta57+
Attachment #8915434 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I verified this issue using Firefox Beta 57.0b9 with Build ID 20171016185129 on Windows 10 x64, Windows 7 x64, Mac OS X 10.12, Ubuntu 16.04.
I will mark this as verified fixed.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.