Use the new thin icon style from bug 1225184 on toolbar icons

VERIFIED FIXED in Firefox 50

Status

DevTools
General
P2
normal
VERIFIED FIXED
2 years ago
a month ago

People

(Reporter: ntim, Assigned: helenvholmes)

Tracking

(Depends on: 1 bug)

unspecified
Firefox 50
Dependency tree / graph

Firefox Tracking Flags

(firefox50 verified)

Details

(Whiteboard: [btpp-fix-later])

Attachments

(7 attachments, 8 obsolete attachments)

546 bytes, image/svg+xml
Details
635.16 KB, image/png
Details
65.61 KB, image/png
Details
74.27 KB, image/png
Details
89.91 KB, image/png
Details
75.58 KB, image/png
Details
109.40 KB, patch
helenvholmes
: review+
helenvholmes
: ui-review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Reporter)

Comment 1

2 years ago
Created attachment 8735985 [details]
clear.svg
Priority: -- → P2
Whiteboard: [btpp-fix-later]
(Reporter)

Comment 2

2 years ago
Created attachment 8737547 [details] [diff] [review]
WIP
(Reporter)

Comment 3

2 years ago
Created attachment 8737594 [details] [diff] [review]
WIP 2
Attachment #8737547 - Attachment is obsolete: true
(Reporter)

Comment 4

2 years ago
Created attachment 8744440 [details] [diff] [review]
WIP 3
Attachment #8737594 - Attachment is obsolete: true
(Reporter)

Comment 5

2 years ago
Created attachment 8744441 [details] [diff] [review]
WIP 3.1
Attachment #8744440 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Assignee: nobody → hholmes
(Reporter)

Updated

2 years ago
Blocks: 1268591
Created attachment 8769738 [details] [diff] [review]
thin-icons.patch

Hey Tim,

Tried to pick up where you left off. I pixelsnapped some additional toolbars in addition to cleaning up the debugger icons (step-over, step-in, etc.), the command-no-autohide button (bug 1260560), and a few other odds and ends.
Attachment #8769738 - Flags: review?(ntim.bugs)
Created attachment 8769748 [details] [diff] [review]
thin-icons.patch
Attachment #8769738 - Attachment is obsolete: true
Attachment #8769738 - Flags: review?(ntim.bugs)
Attachment #8769748 - Flags: review?(ntim.bugs)
(Reporter)

Comment 8

2 years ago
Created attachment 8769766 [details]
critique helen's icons-1.png

The new icons are a definite improvement. I really the perf icon :)

I'm seeing some sizing inconsistencies as highlighted in the screenshot, and some shape inconsistencies as well (the shape of both breakpoint icons is different, as well as the { } icon).
(Reporter)

Comment 9

2 years ago
(In reply to Tim Nguyen :ntim (use needinfo?) from comment #8)
> Created attachment 8769766 [details]
> critique helen's icons-1.png
> 
> The new icons are a definite improvement. I really the perf icon :)
I really like*
(Reporter)

Comment 10

2 years ago
Created attachment 8769770 [details]
critique-2.png
(Reporter)

Comment 11

2 years ago
Created attachment 8769772 [details]
feedback-3.png
(Reporter)

Comment 12

2 years ago
Comment on attachment 8769748 [details] [diff] [review]
thin-icons.patch

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

The code changes look good so far.

::: devtools/client/themes/images/itemToggle.svg
@@ +1,4 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg width="16" height="16" viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg" fill="whitesmoke">

You might want to put this icon in sync with debugger-blackbox.svg, since it's supposed to be the same eye icon.

::: devtools/client/themes/images/power.svg
@@ +1,5 @@
> +<?xml version="1.0" encoding="UTF-8" standalone="no"?>
> +<svg width="16px" height="16px" viewBox="0 0 16 16" fill="whitesmoke" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:sketch="http://www.bohemiancoding.com/sketch/ns">
> +            <path d="M8.5,14 C5.46243388,14 3,11.5375661 3,8.5 C3,5.46243388 5.46243388,3 8.5,3 C11.5375661,3 14,5.46243388 14,8.5 C14,11.5375661 11.5375661,14 8.5,14 L8.5,14 Z M8.5,13 C10.9852814,13 13,10.9852814 13,8.5 C13,6.01471863 10.9852814,4 8.5,4 C6.01471863,4 4,6.01471863 4,8.5 C4,10.9852814 6.01471863,13 8.5,13 L8.5,13 Z M10.6892468,4.56750356 L11.3722285,3.80863499 L11.3722285,3.80863499 C10.5362548,3.29572747 9.55266283,3 8.5,3 C7.44733717,3 6.46374519,3.29572747 5.62777149,3.80863499 L6.3107532,4.56750356 C6.95873288,4.20599601 7.70530997,4 8.5,4 C9.29469003,4 10.0412671,4.20599601 10.6892468,4.56750356 L10.6892468,4.56750356 L10.6892468,4.56750356 Z" id="Triangle" sketch:type="MSShapeGroup"></path>
> +            <path d="M8,8 L9,8 L9,2 L8,2 L8,8 Z" id="Shape" sketch:type="MSShapeGroup"></path>
> + </svg>
\ No newline at end of file

This SVG needs cleanup

::: devtools/client/themes/images/tool-profiler.svg
@@ +1,5 @@
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg width="16" height="16" viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg" fill="whitesmoke">
> +  <g fill-rule="evenodd">

the <g> tag can be removed, and fill-rule="evenodd" can be set on <svg>

::: devtools/client/themes/images/tool-scratchpad.svg
@@ +1,4 @@
> +<svg width="16" height="16" viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg">
> +  <title>
> +    tool-scratchpad-expanded
> +  </title>

The title can be removed

@@ +1,5 @@
> +<svg width="16" height="16" viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg">
> +  <title>
> +    tool-scratchpad-expanded
> +  </title>
> +  <g fill="whitesmoke" fill-rule="evenodd">

Same comment about the g tag.
Attachment #8769748 - Flags: review?(ntim.bugs) → review+
Created attachment 8769853 [details] [diff] [review]
thin-icons.patch

I know you marked the last patch as r+, but would you mind taking a second look just to be safe? I ended up updating a few more icons, couldn't help myself.

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #8)
> The new icons are a definite improvement. I really the perf icon :)
That icon is all Bryan Bell, can't take credit for it :D

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #8)
> Created attachment 8769766 [details]
> critique helen's icons-1.png
> I'm seeing some sizing inconsistencies as highlighted in the screenshot, and
> some shape inconsistencies as well (the shape of both breakpoint icons is
> different, as well as the { } icon).

I updated the {} icon to be the same. The debugger icon and the toggle breakpoints icon is purposefully different, however.

Updated the shader editor icon, it was a little blurry.

Made the commandnoautohide icon a little larger, but wasn't sure what to do about webaudio and scratchpad—on scratchpad the lines aren't centered if I give it one pixel of height, and when I give it two pixels of height it looks kinda large. Since both it and webaudio aren't /super/ used, I think it's okay to leave them.

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #10)
> Created attachment 8769770 [details]
> critique-2.png
I think the stroke seeming large on the filter icon is visual trickery—I bumped down the size of the "liquid" inside the filter and I think it helps, let me know what you think.

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #11)
> Created attachment 8769772 [details]
> feedback-3.png
Updated!
Attachment #8769748 - Attachment is obsolete: true
Attachment #8769853 - Flags: review?(ntim.bugs)
(Reporter)

Updated

2 years ago
Attachment #8744441 - Attachment is obsolete: true
(Reporter)

Comment 14

2 years ago
(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #13)
> Created attachment 8769853 [details] [diff] [review]
> thin-icons.patch
> 
> I know you marked the last patch as r+, but would you mind taking a second
> look just to be safe? I ended up updating a few more icons, couldn't help
> myself.
Sure, taking a look.

> (In reply to Tim Nguyen :ntim (use needinfo?) from comment #8)
> > Created attachment 8769766 [details]
> > critique helen's icons-1.png
> > I'm seeing some sizing inconsistencies as highlighted in the screenshot, and
> > some shape inconsistencies as well (the shape of both breakpoint icons is
> > different, as well as the { } icon).
> 
> I updated the {} icon to be the same. The debugger icon and the toggle
> breakpoints icon is purposefully different, however.
Looks better.

> Updated the shader editor icon, it was a little blurry.
Looks better as well.

> Made the commandnoautohide icon a little larger, but wasn't sure what to do
> about webaudio and scratchpad—on scratchpad the lines aren't centered if I
> give it one pixel of height, and when I give it two pixels of height it
> looks kinda large. Since both it and webaudio aren't /super/ used, I think
> it's okay to leave them.
I thought the old scratchpad icon looked better sizing-wise, any reason why we can't keep it/pixel-snap it ?

> (In reply to Tim Nguyen :ntim (use needinfo?) from comment #10)
> > Created attachment 8769770 [details]
> > critique-2.png
> I think the stroke seeming large on the filter icon is visual trickery—I
> bumped down the size of the "liquid" inside the filter and I think it helps,
> let me know what you think.
It looks a bit better, but the stroke still feels a bit thick. I've opened the icon with sketch, and it looks like the bottom part of the icon isn't pixel snapped, which is making the stroke blurry.

> (In reply to Tim Nguyen :ntim (use needinfo?) from comment #11)
> > Created attachment 8769772 [details]
> > feedback-3.png
> Updated!
Thanks, these look good!
(Reporter)

Comment 15

2 years ago
Comment on attachment 8769853 [details] [diff] [review]
thin-icons.patch

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

r+ for the code changes.
f+ for UI.

Also, I've just realised you've missed 2 toolbar icons: rewind.png and fast-forward.png. Can you update them as well ? I guess you should be able to fork the play icon.

::: devtools/client/themes/images/diff.svg
@@ +3,5 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg height="16" width="16" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16" fill="whitesmoke">
> +   <path d="M10.2 4.1c-.6 0-1.2.2-1.8.4-.6-.4-1.4-.6-2.2-.6-2.5 0-4.6 2.1-4.6 4.6s2.1 4.6 4.6 4.6c.9 0 1.6-.2 2.3-.7.5.2 1.1.4 1.7.4 2.4 0 4.3-1.9 4.3-4.3.1-2.4-1.9-4.4-4.3-4.4zm-4 7.9c-1.9 0-3.5-1.6-3.5-3.5S4.2 5 6.2 5c.3 0 .7 0 1 .1-.8.9-1.4 2.1-1.4 3.4 0 1.3.6 2.5 1.5 3.3-.4.1-.8.2-1.1.2zm2.1-.7c-.9-.6-1.4-1.6-1.4-2.8 0-1.1.6-2.1 1.4-2.8.8.6 1.4 1.6 1.4 2.8 0 1.1-.6 2.1-1.4 2.8z"/>
> +   <path d="M7.6 8c-.2 0-.3-.1-.4-.2-.1-.2 0-.4.2-.5l1.1-.6c.2-.1.4 0 .5.2.1.2 0 .4-.2.5l-1.1.5c0 .1-.1.1-.1.1zM7.6 9.1c-.1 0-.3-.1-.4-.2-.1-.2 0-.4.2-.5l1.1-.6c.3-.1.5 0 .6.2.1.2 0 .4-.2.5l-1.1.6h-.2zM7.8 10.3c-.1 0-.3-.1-.4-.2-.1-.2 0-.4.2-.5L8.8 9c.2-.1.4 0 .5.2.1.2 0 .4-.2.5l-1.1.6h-.2z"/>
> + </svg>
\ No newline at end of file

nit: leading whitespace

::: devtools/client/themes/images/filter.svg
@@ +1,4 @@
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16" fill="whitesmoke">

The fill was intentionally set to #aaa for this icon, to prevent it from being invisible in the light theme filter boxes (where we can't apply the invert filter)

If you want both colours to be supported, you can use a location hash to designate the wanted colour. You can see how it's done here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/images/commandline-icon.svg

Let me know if you'd like me to do it.
Attachment #8769853 - Flags: review?(ntim.bugs) → review+
(Reporter)

Comment 16

2 years ago
Created attachment 8769910 [details]
off centre icon.png

The play icon is offcenter. This is not noticeable in the debugger, but it looks a bit odd in the animation inspector.
Created attachment 8770245 [details] [diff] [review]
thin-icons.patch

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #14)
> I thought the old scratchpad icon looked better sizing-wise, any reason why
> we can't keep it/pixel-snap it ?
I made it a little larger—think it's looking pretty similar. We can fudge it another pixel if you still don't like it.

> > (In reply to Tim Nguyen :ntim (use needinfo?) from comment #10)
> It looks a bit better, but the stroke still feels a bit thick. I've opened
> the icon with sketch, and it looks like the bottom part of the icon isn't
> pixel snapped, which is making the stroke blurry.
I think this is fixed now, take a gander.

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #16)
> Created attachment 8769910 [details]
> off centre icon.png
> 
> The play icon is offcenter. This is not noticeable in the debugger, but it
> looks a bit odd in the animation inspector.
Fixed!

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #15)
> Also, I've just realised you've missed 2 toolbar icons: rewind.png and
> fast-forward.png. Can you update them as well ? I guess you should be able
> to fork the play icon.
Added these. Didn't see the fast-forward icon used anywhere in dxr, but this might be worth double-checking.

> ::: devtools/client/themes/images/filter.svg
> @@ +1,4 @@
> >  <!-- This Source Code Form is subject to the terms of the Mozilla Public
> >     - License, v. 2.0. If a copy of the MPL was not distributed with this
> >     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> > +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16" fill="whitesmoke">
> 
> The fill was intentionally set to #aaa for this icon, to prevent it from
> being invisible in the light theme filter boxes (where we can't apply the
> invert filter)
> 
> If you want both colours to be supported, you can use a location hash to
> designate the wanted colour. You can see how it's done here:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/images/
> commandline-icon.svg
> 
> Let me know if you'd like me to do it.
I just set this back to #aaa. I'd like to change some icon colors at some point (I think your point in bug 1268591 is a good one) but I'd rather do it in another patch.
Attachment #8769853 - Attachment is obsolete: true
Attachment #8770245 - Flags: ui-review?(ntim.bugs)
Attachment #8770245 - Flags: review+
(Reporter)

Comment 18

2 years ago
(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #17)
> Created attachment 8770245 [details] [diff] [review]
> thin-icons.patch
> 
> (In reply to Tim Nguyen :ntim (use needinfo?) from comment #14)
> > I thought the old scratchpad icon looked better sizing-wise, any reason why
> > we can't keep it/pixel-snap it ?
> I made it a little larger—think it's looking pretty similar. We can fudge it
> another pixel if you still don't like it.
It looks better, but I still feel it's smaller than the other ones, maybe it's because the DOM icon next to it is too big ? 

> > > (In reply to Tim Nguyen :ntim (use needinfo?) from comment #10)
> > It looks a bit better, but the stroke still feels a bit thick. I've opened
> > the icon with sketch, and it looks like the bottom part of the icon isn't
> > pixel snapped, which is making the stroke blurry.
> I think this is fixed now, take a gander.
Looks much better, thanks !

> (In reply to Tim Nguyen :ntim (use needinfo?) from comment #16)
> > Created attachment 8769910 [details]
> > off centre icon.png
> > 
> > The play icon is offcenter. This is not noticeable in the debugger, but it
> > looks a bit odd in the animation inspector.
> Fixed!
Thanks! Looks like the pause icon is also a bit off centre, but I'm fine with leaving it that way.

> (In reply to Tim Nguyen :ntim (use needinfo?) from comment #15)
> > Also, I've just realised you've missed 2 toolbar icons: rewind.png and
> > fast-forward.png. Can you update them as well ? I guess you should be able
> > to fork the play icon.
> Added these. Didn't see the fast-forward icon used anywhere in dxr, but this
> might be worth double-checking.
Thanks! the fast-forward icon doesn't seem used, but it doesn't hurt updating it :)
Anyway, I've noticed a pixel snapping issue with the rewind icon.
(Reporter)

Updated

2 years ago
Attachment #8770245 - Flags: ui-review?(ntim.bugs) → ui-review+
(Reporter)

Comment 19

2 years ago
Created attachment 8770262 [details]
anim-inspector-new-icons.png
(Reporter)

Comment 20

2 years ago
Also, little detail, it would be nice if the | part of the rewind icon has the same size as the | of the pause icon.
Created attachment 8770649 [details] [diff] [review]
thin-icons.patch

> > (In reply to Tim Nguyen :ntim (use needinfo?) from comment #16)
> Thanks! Looks like the pause icon is also a bit off centre, but I'm fine
> with leaving it that way.
Fixed!

> > (In reply to Tim Nguyen :ntim (use needinfo?) from comment #15)
> Anyway, I've noticed a pixel snapping issue with the rewind icon.
Fixed this too.

> > (In reply to Tim Nguyen :ntim (use needinfo?) from comment #14)
> It looks better, but I still feel it's smaller than the other ones, maybe
> it's because the DOM icon next to it is too big ? 
Would you like to do another review cycle on this patch for this, or should we file a new bug? I'm kinda okay with this sizing difference personally, but it's up to you.

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #20)
> Also, little detail, it would be nice if the | part of the rewind icon has
> the same size as the | of the pause icon.
Added this as well, I agree.
Attachment #8770245 - Attachment is obsolete: true
Flags: needinfo?(ntim.bugs)
Attachment #8770649 - Flags: ui-review+
Attachment #8770649 - Flags: review+
(Reporter)

Comment 22

2 years ago
The scratchpad and webaudio tools are not frequently used, so I'm fine with a new bug.
Flags: needinfo?(ntim.bugs)
(Reporter)

Updated

2 years ago
Keywords: checkin-needed
(Reporter)

Comment 23

2 years ago
Thanks for adressing my issues!
(Assignee)

Updated

2 years ago
See Also: → bug 1286624
(In reply to Tim Nguyen :ntim (use needinfo?) from comment #22)
> The scratchpad and webaudio tools are not frequently used, so I'm fine with
> a new bug.
Okeydoke, filed Bug 1286624.

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #23)
> Thanks for adressing my issues!
Thanks for doing all of these reviews!

Comment 25

2 years ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/105d425837e7
pixelsnap more toolbar icons and use thin style, r=ntim
Keywords: checkin-needed

Comment 26

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/105d425837e7
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50

Updated

2 years ago
Depends on: 1286913
(Reporter)

Updated

2 years ago
No longer depends on: 1286913

Updated

2 years ago
Depends on: 1287310
(Reporter)

Updated

2 years ago
Depends on: 1287396

Updated

2 years ago
Depends on: 1293322

Updated

2 years ago
Depends on: 1296917
I have reproduced this on firefox Nightly according to(2016-03-29)

Fixing bug is verified on Latest Developer Edition-- Build ID: (20160831004001),User Agent: Mozilla/5.0 (Windows NT 10.0; rv:50.0) Gecko/20100101 Firefox/50.0

Tested OS-- Windows10 32bit
QA Whiteboard: [bugday-20160831]
(Reporter)

Updated

2 years ago
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.