Update Command Button Icons (SVG)

VERIFIED FIXED in Firefox 48

Status

defect
VERIFIED FIXED
4 years ago
11 months ago

People

(Reporter: hholmes, Assigned: hholmes)

Tracking

unspecified
Firefox 48
Dependency tree / graph

Firefox Tracking Flags

(firefox48 verified)

Details

(Whiteboard: [devtools-ux][devtools-toolbar])

Attachments

(3 attachments, 5 obsolete attachments)

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.
Assignee: nobody → hholmes
Whiteboard: [devtools-ux]
Depends on: 1223701
Summary: Update Common Button Icons → Update Command Button Icons
Duplicate of this bug: 1174395
Whiteboard: [devtools-ux] → [devtools-ux][devtools-toolbar]
Posted patch command-svgs.patch (obsolete) — Splinter Review
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).
Attachment #8708520 - Flags: ui-review?(ntim.bugs)
Attachment #8708520 - Flags: review?(bgrinstead)
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!)
Attachment #8708520 - Flags: review?(bgrinstead)

Comment 4

3 years ago
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.
Attachment #8708520 - Flags: ui-review?(ntim.bugs)
Summary: Update Command Button Icons → Update Command Button Icons (SVG)

Updated

3 years ago
Blocks: 1257348
Posted patch command-svgs.patch (obsolete) — Splinter Review
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...
Attachment #8708520 - Attachment is obsolete: true
Attachment #8735529 - Flags: ui-review?(ntim.bugs)
Attachment #8735529 - Flags: review?(bgrinstead)
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/cubic-bezier-swatch@2x.png (themes/images/cubic-bezier-swatch@2x.png)
> +    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 undock@2x.png 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!
Attachment #8735529 - Flags: review?(bgrinstead)

Comment 7

3 years ago
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

3 years ago
Posted image 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.

Updated

3 years ago
Attachment #8735529 - Flags: ui-review?(ntim.bugs) → feedback+

Comment 9

3 years ago
Here's how the icons look on Windows.
Posted patch command-svgs.patch (obsolete) — Splinter Review
(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/cubic-bezier-swatch@2x.png (themes/images/cubic-bezier-swatch@2x.png)
> > +    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!
Attachment #8735529 - Attachment is obsolete: true
Attachment #8735883 - Flags: review?(bgrinstead)
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
Posted patch command-svgs.patch (obsolete) — Splinter Review
Attachment #8735883 - Attachment is obsolete: true
Attachment #8735883 - Flags: review?(bgrinstead)

Comment 13

3 years ago
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.
Posted patch command-svgs.patch (obsolete) — Splinter Review
Attachment #8735969 - Attachment is obsolete: true

Comment 15

3 years ago
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.
Attachment #8735986 - Attachment is obsolete: true

Updated

3 years ago
Attachment #8735995 - Flags: review+

Updated

3 years ago
Depends on: 1260562

Updated

3 years ago
Depends on: 1260565
has problems to apply:

unable to find 'devtools/client/themes/images/undock@2x.png' for patching
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/themes/images/undock@2x.png.rej
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
Flags: needinfo?(hholmes)
Keywords: checkin-needed

Comment 19

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f3cf9faeb728
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48

Comment 20

3 years ago
(In reply to Carsten Book [:Tomcat] from comment #18)
> has problems to apply:
> 
> unable to find 'devtools/client/themes/images/undock@2x.png' for patching
> 1 out of 1 hunks FAILED -- saving rejects to file
> devtools/client/themes/images/undock@2x.png.rej
> 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!
Flags: needinfo?(hholmes)

Updated

3 years ago
Depends on: 1261430
Depends on: 1283072
VERIFIED FIXED in Firefox 48.0b4 (20160630123429)on Windows 10 x64 and OSx 10.9.5.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1302691

Updated

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