Closed
Bug 1389462
Opened 7 years ago
Closed 7 years ago
Outdated compass icon for the middle click autoscroll
Categories
(Firefox :: Theme, defect, P1)
Tracking
()
VERIFIED
FIXED
Firefox 58
People
(Reporter: utybodev, Assigned: dao)
References
(Blocks 1 open bug)
Details
(Keywords: good-first-bug, Whiteboard: [reserve-photon-visual])
Attachments
(2 files, 2 obsolete files)
The middle click autoscroll compass icon does not respect the new Photon UI guidelines, and looks more of a native icon from Windows 7. As such, it does not respect Windows 8 or 10 icons style, nor does it respect the general Photon UI, and looks very out of place.
I have made a new icon, following the Photon guidelines(1) as well as the MDN SVG Cleanup guide(2). This compass can be used for all of the different compass types by simply disabling or enabling the "northsouth" and "westeast" components. Note that the icon has a light background to allow it to be seen even on dark backgrounds.
(This icon was made by me after reddit user u/Jop902 pointed out the outdated icon, and u/zbraniecki suggested I make a new one)
(1) http://design.firefox.com/photon/visual/icons.html
(2) https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/SVG_Guidelines
Updated•7 years ago
|
Whiteboard: [photon-visual][triage]
Comment 1•7 years ago
|
||
Thank you for your contribution Matthieu!
:Gijs set the [triage] whiteboard status which will result in someone from the UX team evaluating if the icon needs any finetuning.
Once this is settled, I'll help you write a patch to get it into Firefox :)
Assignee | ||
Comment 2•7 years ago
|
||
Shorlander, does the middle-click / autoscroll image need an update? Does the attached one look good to you?
Flags: needinfo?(shorlander)
Comment 3•7 years ago
|
||
This looks great, thank you!
The only tweak I would suggest is matching the 45º angle of our other arrow heads: http://design.firefox.com/icons/viewer/#arrow
Flags: needinfo?(shorlander)
Assignee | ||
Comment 4•7 years ago
|
||
Matthieu, do you want to update the icon as Stephen suggests?
Blocks: photon-visual
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(utybodev)
Priority: -- → P4
Whiteboard: [photon-visual][triage] → [reserve-photon-visual]
Reporter | ||
Comment 5•7 years ago
|
||
I have fixed the icon following Stephen's suggestion. Does this look better?
Attachment #8896240 -
Attachment is obsolete: true
Flags: needinfo?(utybodev)
Updated•7 years ago
|
Flags: needinfo?(shorlander)
Comment 6•7 years ago
|
||
(In reply to Matthieu "utybo" S. from comment #5)
> Created attachment 8900870 [details]
> Compass icon with fixed arrows angles
>
> I have fixed the icon following Stephen's suggestion. Does this look better?
Yes, thank you.
Flags: needinfo?(shorlander)
Assignee | ||
Comment 7•7 years ago
|
||
Matthieu, would you also like to write the patch for integrating this icon?
Flags: needinfo?(utybodev)
Comment 8•7 years ago
|
||
Optimized the SVG further and used context-fill.
Comment 9•7 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #7)
> Matthieu, would you also like to write the patch for integrating this icon?
This search should help: https://dxr.mozilla.org/mozilla-central/search?q=autoscroll.png&redirect=false
Assignee | ||
Comment 10•7 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #8)
> Created attachment 8901260 [details]
> compass-icon.svg
>
> Optimized the SVG further and used context-fill.
Why would we use context-fill here?
Comment 11•7 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #10)
> (In reply to Tim Nguyen :ntim from comment #8)
> > Created attachment 8901260 [details]
> > compass-icon.svg
> >
> > Optimized the SVG further and used context-fill.
>
> Why would we use context-fill here?
The original PNG has different states, so I thought it may be useful here.
Comment 12•7 years ago
|
||
If it isn't, it could easily be removed anyway.
Assignee | ||
Updated•7 years ago
|
Attachment #8901260 -
Attachment is obsolete: true
Reporter | ||
Comment 13•7 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #7)
> Matthieu, would you also like to write the patch for integrating this icon?
I think I would prefer having someone else qualified write the patch, as I have no idea of how to do it and I do not feel comfortable with playing around with Firefox sources. Sorry :/
Flags: needinfo?(utybodev) → needinfo?(dao+bmo)
Assignee | ||
Comment 14•7 years ago
|
||
No problem, we can take care of this. Thanks for the image!
Flags: needinfo?(dao+bmo)
Comment 15•7 years ago
|
||
:dao - what's needed in this bug? Is it an easy-first-bug?
Flags: needinfo?(dao+bmo)
Assignee | ||
Comment 16•7 years ago
|
||
We basically just need to update the references to autoscroll.png. It's not very heard but also not super trivial since autscroll.png has a weird format containing multiple icons.
Flags: needinfo?(dao+bmo)
Comment 17•7 years ago
|
||
Ok. So the next step is to generate the updated autoscroll.png with the new icon.
Here's the current one:
* Windows: http://searchfox.org/mozilla-central/source/toolkit/themes/windows/global/icons/autoscroll.png
* Mac: http://searchfox.org/mozilla-central/source/toolkit/themes/osx/global/icons/autoscroll.png
* Linux: http://searchfox.org/mozilla-central/source/toolkit/themes/linux/global/icons/autoscroll.png
It seems to me that Windows uses the right column for XP_WIN (Windows XP) and left column for others.
The Mac and Linux only use the right column.
:shorlander - do you want per-platform icons for the new one? If so, can you instruct how to alter them?
Flags: needinfo?(shorlander)
Assignee | ||
Comment 18•7 years ago
|
||
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #17)
> Ok. So the next step is to generate the updated autoscroll.png with the new
> icon.
No, we should update the CSS to reference the new SVG icon.
Comment 19•7 years ago
|
||
> No, we should update the CSS to reference the new SVG icon.
Ok.
But we need three versions of the icon - for horizontal, vertical and both directions scrolling, right?
Comment 20•7 years ago
|
||
Tim, I've seen your amazing work with icons all around! Congrats!
Is there any chance you'd be interested in helping with this bug? :)
Flags: needinfo?(ntim.bugs)
Assignee | ||
Comment 21•7 years ago
|
||
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #19)
> > No, we should update the CSS to reference the new SVG icon.
>
> Ok.
>
> But we need three versions of the icon - for horizontal, vertical and both
> directions scrolling, right?
Yep, they're easy to produce from the attached SVG since Matthieu already grouped the "northsouth" and "westeast" paths.
Updated•7 years ago
|
Flags: qe-verify?
Assignee | ||
Updated•7 years ago
|
Flags: qe-verify? → qe-verify+
Updated•7 years ago
|
Keywords: good-first-bug
Updated•7 years ago
|
QA Contact: ovidiu.boca
Comment 23•7 years ago
|
||
If this is a good first bug, I would like to try helping with it as long as someone can point me in the right direction. The concept seems simple but I haven't worked with SVG files before.
Comment 24•7 years ago
|
||
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #17)
> :shorlander - do you want per-platform icons for the new one? If so, can you
> instruct how to alter them?
No, I don't think there is anything platform specific we need to do here.
Flags: needinfo?(shorlander)
Assignee | ||
Comment 25•7 years ago
|
||
gandalf, still want to mentor this? There's interest in comment 23.
Flags: needinfo?(gandalf)
Comment 26•7 years ago
|
||
I did my best to collect all the relevant information from the stakeholders.
I think at this point all that has to be done is:
* generate the correct SVG file
* generate the correct CSS to switch between states
* place the SVG in the tree and update CSS code
I've never worked on SVG+CSS so I'm not sure how those two steps should be done, but I can help with the third.
Flags: needinfo?(gandalf)
Assignee | ||
Updated•7 years ago
|
Mentor: gandalf
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Priority: P4 → P1
Comment hidden (mozreview-request) |
Comment 28•7 years ago
|
||
mozreview-review |
Comment on attachment 8915941 [details]
Bug 1389462 - Update autoscroll icon.
https://reviewboard.mozilla.org/r/187208/#review192220
Wasnt able to test due to lack of mouse, but codewise the patch looks simple and correct, cheers
Attachment #8915941 -
Flags: review?(dharvey) → review+
Comment 29•7 years ago
|
||
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5b41d7a0fe73
Update autoscroll icon. r=daleharvey
Comment 30•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Assignee | ||
Comment 31•7 years ago
|
||
Not going to try to uplift this to 57 -- let's have this ride the 58 release train.
status-firefox57:
--- → wontfix
Comment 32•7 years ago
|
||
Thank you for your contribution Matthieu! :)
Comment 33•7 years ago
|
||
I don't think this icon matches the default theme and cursors on Windows 7. Is it appropriate to add it to all platforms even if it doesn't match?
Right now it looks seriously out of place on my browser.
Reporter | ||
Comment 34•7 years ago
|
||
Looks as if this is getting loads of criticism from r/firefox, you guys may want to have a look at it
https://www.reddit.com/r/firefox/comments/752s7x/thanks_to_mozilla_for_changing_the_middle_click/
Comment 35•7 years ago
|
||
IMHO this isn’t a UI element that needs to be “Photonized” to that extreme: even under Windows 10, the overly thick lines look quite ludicrous. It might look alright for high-contrast themes, though.
Comment 36•7 years ago
|
||
(In reply to Matthieu "utybo" S. from comment #34)
> Looks as if this is getting loads of criticism from r/firefox, you guys may
> want to have a look at it
>
>
> https://www.reddit.com/r/firefox/comments/752s7x/
> thanks_to_mozilla_for_changing_the_middle_click/
In case it helps, my intuitive expectation is that, unless everything including the default cursor has been re-themed (eg. in a game expected to run full-screen), all cursors are "OS elements" and, thus, should match the OS's design language rather than the application's.
(Or, to put it another way, consistency between cursors is paramount,)
Comment 37•7 years ago
|
||
Verified fixed using the latest Nightly (2017-10-13) on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12.
You need to log in
before you can comment on or make changes to this bug.
Description
•