1225184 - Update Command Button Icons (SVG)

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[Helen V. Holmes (:helenvholmes) (:✨)]

I've redesigned a bunch of these to match the panel icons—since ntim's done the work to convert these from PNGs, I'd like to get in the new icons to match everything else.

Comment 2

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: command-svgs.patch

I had to mess with the sizing a little bit to make them seem like they were all on the same baseline, so you'll notice they all look a little smaller. Also, now that everything is SVG, there's probably some follow-up CSS cleanup that would be worth doing (I didn't do that in this patch, I think it probably belongs in a separate bug).

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8708520 [details] [diff] [review]
command-svgs.patch

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

The docking buttons look a bit off-center now.. I'm not sure the fix off hand but I think #toolbox-dock-buttons > toolbarbutton and #toolbox-dock-buttons > toolbarbutton > image may need some updates

::: devtools/client/themes/toolbars.css
@@ +84,3 @@
>  
>  .devtools-toolbarbutton:not([label]) > .toolbarbutton-icon,
>  .devtools-button::before {

This shouldn't be removed - otherwise icons will get stretched out in other panels (see debugger for instance).  If we need a diff size for command icons only we should override it.

@@ +758,3 @@
>  }
>  
>  @media (min-resolution: 1.1dppx) {

May as well delete this whole media block now since the background-image rules are duplicated (one of the big benefits of this change!)

Comment 4

[Tim Nguyen :ntim]

Comment on attachment 8708520 [details] [diff] [review]
command-svgs.patch

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

Overall comments about the SVGs:
- They feel quite inconsistent compared to other devtools/firefox icons (in sizing and in style) But to be fair, our current icons are not very consistent either.
- They have odd viewBoxes which cause them to look blurry on 1x screens (the icon's pixel grid dimensions should be equal to the CSS dimensions and the viewBox dimensions, otherwise they are oddly resized on 1x screens)

Other stuff I've noticed :
- A lot of toolbar icons are either stretched or disappeared (what Brian noticed)
- Some close icon references need to be updated (developer toolbar, responsive mode)

::: devtools/client/themes/toolbars.css
@@ +200,4 @@
>  /* Icon-only buttons */
>  .devtools-button:empty::before,
>  .devtools-toolbarbutton:not([label]):not([disabled]) > image {
> +  opacity: 0.6;

Changing the opacity here affects a bunch of other places as well.

Comment 5

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: command-svgs.patch

Some known issues:

- The console icon sits a little low imo. Unfortunately, it sits too high in the next place it pixel snaps. :|
- The settings gear is still a little blurry, but I'd prefer to open a new bug for this and get the rest of this patch landed. Maybe we can do some research and find an already-pixel-snapped settings icon...

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8735529 [details] [diff] [review]
command-svgs.patch

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

CSS changes look great to me.  I'll take one more look at it after updates but don't see any issues with this

::: devtools/client/jar.mn
@@ -308,4 @@
>      skin/images/tool-webaudio.svg (themes/images/tool-webaudio.svg)
>      skin/images/tool-memory.svg (themes/images/tool-memory.svg)
>      skin/images/tool-memory-active.svg (themes/images/tool-memory-active.svg)
> -    skin/images/close.png (themes/images/close.png)

Note that these are also currently referenced by GCLI: https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/commandline.inc.css#105, but that's being removed in Bug 1257348 so it shouldn't need any updates from you.

However, it looks like the images themselves aren't deleted in the patch.

@@ +310,4 @@
>      skin/images/sort-arrows.svg (themes/images/sort-arrows.svg)
>      skin/images/cubic-bezier-swatch.png (themes/images/cubic-bezier-swatch.png)
>      skin/images/[e-mail] (themes/images/[e-mail])
> +    skin/images/dock-undock.svg (themes/images/dock-undock.svg)

Can you move this up to be next to the other dock-*.svg images?

Also, it appears that the [e-mail] file hasn't been deleted

::: devtools/client/themes/toolbars.css
@@ +493,3 @@
>    height: 16px;
>    -moz-appearance: none;
>    background-size: 16px 16px;

I think we could use background-size: cover here without changing the appearance

@@ -771,5 @@
>  #command-button-measure > image {
> -  background-image: url("chrome://devtools/skin/images/command-measure.png");
> -}
> -
> -@media (min-resolution: 1.1dppx) {

Glad to see this go!

Comment 7

[Tim Nguyen :ntim]

Comment on attachment 8735529 [details] [diff] [review]
command-svgs.patch

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

In the SVGs, I see .st0 classes being applied, can you follow the SVG guidelines and set inline attributes instead of using a class for one specific element ?

I've pushed this to MozScreenshots, so we can check whether icons are blurry or not on Windows: https://treeherder.mozilla.org/#/jobs?repo=try&revision=083f1677ef40

::: devtools/client/themes/toolbars.css
@@ +733,4 @@
>  }
>  
>  #command-button-screenshot > image {
> +  background-image: url("chrome://devtools/skin/images/command-screenshot.svg");

There are some references in memory.css you need to update as well (for command-screenshot.svg).

Comment 8

[Tim Nguyen :ntim]

Attachment: Comments

My main concern is the sizing inconsistency between the command button icons and the tool icons. I would use the sizing of the command button icons, since it's closer to what add-ons use (React DevTools for example).

Otherwise, I like the new icons.

Comment 9

[Tim Nguyen :ntim]

Attachment: windows-screenshot.PNG

Here's how the icons look on Windows.

Comment 10

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: command-svgs.patch

(In reply to Brian Grinstead [:bgrins] from comment #6)
> However, it looks like the images themselves aren't deleted in the patch.
Whoops, think I've taken care of all of them now.

> @@ +310,4 @@
> >      skin/images/sort-arrows.svg (themes/images/sort-arrows.svg)
> >      skin/images/cubic-bezier-swatch.png (themes/images/cubic-bezier-swatch.png)
> >      skin/images/[e-mail] (themes/images/[e-mail])
> > +    skin/images/dock-undock.svg (themes/images/dock-undock.svg)
> 
> Can you move this up to be next to the other dock-*.svg images?
:+1:


(In reply to Tim Nguyen [:ntim] from comment #8)
> My main concern is the sizing inconsistency between the command button icons
> and the tool icons. I would use the sizing of the command button icons,
> since it's closer to what add-ons use (React DevTools for example).
So I think it's all right to leave them separate. The reason I say this is two-fold:

1. The icons currently look good relative to the text they appear next to, which I think is more important.
2. When I resize them to the next size where they look good pixel-snapped, they seem... overly large. I'd rather do this which seems like a reasonable solution between the two.

As for the storage icon, that again was a pixel snapping issue. I don't know what to say other than :/ 


(In reply to Tim Nguyen [:ntim] from comment #9)
> Created attachment 8735785 [details]
> windows-screenshot.PNG
> 
> Here's how the icons look on Windows.
I must be getting better at this, those look reasonable to me!

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8735883 [details] [diff] [review]
command-svgs.patch

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

.devtools-responsiveui-close is using the close icons still - can you convert those to use the new svg? https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/responsivedesign.inc.css#167 and https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/responsivedesign.inc.css#176

Comment 12

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: command-svgs.patch

Comment 13

[Tim Nguyen :ntim]

Comment on attachment 8735969 [details] [diff] [review]
command-svgs.patch

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

::: devtools/client/jar.mn
@@ +175,5 @@
> +    skin/images/command-paintflashing.svg (themes/images/command-paintflashing.svg)
> +    skin/images/command-screenshot.svg (themes/images/command-screenshot.svg)
> +    skin/images/command-responsivemode.svg (themes/images/command-responsivemode.svg)
> +    skin/images/command-scratchpad.svg (themes/images/command-scratchpad.svg)
> +    skin/images/command-pick.svg (themes/images/command-pick.svg)

This has references in animationinspector.css that need to be updated.

::: devtools/client/themes/images/close.svg
@@ +3,5 @@
> +<svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
> +	 width="16px" height="16px" viewBox="0 0 16 16" style="enable-background:new 0 0 16 16;" xml:space="preserve">
> +<style type="text/css">
> +	.st0{fill:#EDF0F1;}
> +</style>

This SVG needs cleanup, also the fill should be whitesmoke.

::: devtools/client/themes/images/tool-memory-active.svg
@@ +2,3 @@
>     - 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 height="16" width="16" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16" fill="#71c054">

Can you update tool-memory-active.svg to match the style of tool-memory.svg ?
While you're there, can you match this icon's fill with other active icon fills ?

::: devtools/client/themes/images/tool-profiler-active.svg
@@ +4,5 @@
> +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16">
> +  <path fill="#2E761A" d="M6.9 7.8c-.3.1-.6.4-.6.7-.1.5.2 1 .7 1.1.3 0 .7-.1.9-.3l2.5-2.9-3.5 1.4z"/>
> +  <path fill="#2E761A" opacity="0.5" d="M4.7 10.6c.7 1.1 1.9 1.9 3.3 1.9s2.6-.8 3.3-1.9H4.7z"/>
> +  <path fill="whitesmoke" d="M-12.7-2.5c-3.8 0-6.7 3-6.7 6.7s3 6.7 6.7 6.7c3.8 0 6.7-3 6.7-6.7s-2.9-6.7-6.7-6.7zM-12.8 9c-2.5 0-4.6-2.1-4.8-4.5v-.2h.6c.6 0 1-.4 1-1s-.4-1-1-1h-.4c.4-.9.8-1.4 1.5-1.9l.2.4c.3.5.8.7 1.3.4.5-.2.7-.7.4-1.2l-.2-.4c.4-.1.9-.2 1.4-.2.6 0 1.1.1 1.6.3l-.2.4c-.3.5-.1 1 .4 1.3.5.3 1 .1 1.3-.4l.2-.6c.6.6 1.2 1.5 1.4 1.8h-.4c-.6 0-1 .4-1 1s.4 1 1 1h.6v.2C-8.2 6.9-10.3 9-12.8 9zM-12.8 12.7c-3.4 0-6.2 2.7-6.2 6.2s2.7 6.3 6.3 6.3c3.5 0 6.3-2.7 6.3-6.3-.1-3.4-2.9-6.2-6.4-6.2zm0 11.4c-2.9 0-5.2-2.3-5.2-5.2 0-2.9 2.3-5.2 5.2-5.2s5.2 2.3 5.2 5.2c0 3-2.2 5.2-5.2 5.2z"/>
> +  <path fill="whitesmoke" d="M-14.5 16.3c-.2 0-.4.2-.4.4v4.5c0 .2.3.4.5.4s.5-.2.5-.4v-4.5c0-.2-.3-.4-.5-.4"/>

I *think* you can safely remove the paths with fill="whitesmoke" as they're invisible. You can also set fill="#2E761A" globally on the <svg> tag.

Comment 14

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: command-svgs.patch

Comment 15

[Tim Nguyen :ntim]

Comment on attachment 8735986 [details] [diff] [review]
command-svgs.patch

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

::: devtools/client/themes/images/dock-undock.svg
@@ +2,5 @@
> +   - 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">
> +    <path d="M4.3 2.8v5.9c0 .2.2.4.5.4h5.7c.2 0 .5-.2.5-.4V2.8c0-.2-.2-.4-.5-.4H4.8c-.4 0-.5.3-.5.4zm5.8 2.8v2.6h-5V5.6h5zm0-2.5V5h-5V3.1h5z"/>
> +    <path d="M7.1 9.9v2.2h-5V9.7h1.2V9H2.1V7h1.2v-.7H1.7c-.4 0-.5.3-.5.4v5.9c0 .2.2.4.5.4h5.7c.2 0 .5-.2.5-.4V9.9h-.8z"/>

nit: 2 space indentation.

Comment 16

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: command-svgs.patch

Comment 17

[Tim Nguyen :ntim]

Landed: https://hg.mozilla.org/integration/fx-team/rev/8edeb8f0ca56
Backed out for landing with wrong bug number: https://hg.mozilla.org/integration/fx-team/rev/d8b9796cb557

...and relanded with correct bug number: https://hg.mozilla.org/integration/fx-team/rev/f3cf9faeb728

Comment 18

[Carsten Book [:Tomcat]]

has problems to apply:

unable to find 'devtools/client/themes/images/[e-mail]' for patching
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/themes/images/[e-mail]
patching file devtools/client/themes/memory.css
Hunk #1 FAILED at 103
Hunk #2 FAILED at 114
2 out of 2 hunks FAILED -- saving rejects to file devtools/client/themes/memory.css.rej
patching file devtools/client/themes/responsivedesign.inc.css
Hunk #1 FAILED at 163
Hunk #2 FAILED at 171
2 out of 2 hunks FAILED -- saving rejects to file devtools/client/themes/responsivedesign.inc.css.rej
patching file devtools/client/themes/toolbars.css
Hunk #1 FAILED at 491
Hunk #2 FAILED at 633
Hunk #3 FAILED at 728
Hunk #4 FAILED at 760
4 out of 4 hunks FAILED -- saving rejects to file devtools/client/themes/toolbars.css.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh command-svgs.patch

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f3cf9faeb728

Comment 20

[Tim Nguyen :ntim]

(In reply to Carsten Book [:Tomcat] from comment #18)
> has problems to apply:
> 
> unable to find 'devtools/client/themes/images/[e-mail]' for patching
> 1 out of 1 hunks FAILED -- saving rejects to file
> devtools/client/themes/images/[e-mail]
> patching file devtools/client/themes/memory.css
> Hunk #1 FAILED at 103
> Hunk #2 FAILED at 114
> 2 out of 2 hunks FAILED -- saving rejects to file
> devtools/client/themes/memory.css.rej
> patching file devtools/client/themes/responsivedesign.inc.css
> Hunk #1 FAILED at 163
> Hunk #2 FAILED at 171
> 2 out of 2 hunks FAILED -- saving rejects to file
> devtools/client/themes/responsivedesign.inc.css.rej
> patching file devtools/client/themes/toolbars.css
> Hunk #1 FAILED at 491
> Hunk #2 FAILED at 633
> Hunk #3 FAILED at 728
> Hunk #4 FAILED at 760
> 4 out of 4 hunks FAILED -- saving rejects to file
> devtools/client/themes/toolbars.css.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working directory
> errors during apply, please fix and qrefresh command-svgs.patch

Forgot to remove the checkin-needed flag when I landed this, sorry!

Comment 21

[Alexandru Simonca [:asimonca], Release Desktop QA]

VERIFIED FIXED in Firefox 48.0b4 (20160630123429)on Windows 10 x64 and OSx 10.9.5.