Brian Grinstead [:bgrins] Assignee
	Description • 11 years ago

We should use the new icons for the toolbar inside of this zip file: https://bugzilla.mozilla.org/attachment.cgi?id=8339341.

I would like to move the new icons into the shared/devtools theme folder, inside of a subfolder called images/.  Then we can update the references in jar.mn for each theme to point to these shared folders to help get rid of duplication in the themes.

	Brian Grinstead [:bgrins] Assignee
	Comment 1 • 11 years ago

This patch removes individual icons from windows/ linux/ and osx/ folders and instead references shared/devtools/images/*

	Brian Grinstead [:bgrins] Assignee
	Comment 2 • 11 years ago

This patch removes all the old icons from the toolbar and replaces them with the new versions from Bug 837188.  This creates the folder themes/shared/devtools/images, which will be a place where we can move the rest of the images (including the 2x versions) in the future.

There is also a fix for a minor issue that was introduced in Bug 941673 from the linux toolbox.css file, where the selected tab's image still has some opacity (unless if it is :active).  With this patch applied, the selected image will always be fully opaque, hover or not.

	Paul Rouget [:paul]
	Comment 3 • 11 years ago

Comment on attachment 8343971 [details] [diff] [review]
toolbar-icons-shared.patch

2x icons for 1x displays. Is that what we want to do? If so, we need green light from UX.

	Tim Nguyen :ntim
	Comment 4 • 11 years ago

(In reply to Paul Rouget [:paul] from comment #3)
> Comment on attachment 8343971 [details] [diff] [review]
> toolbar-icons-shared.patch
> 
> 2x icons for 1x displays. Is that what we want to do? If so, we need green
> light from UX.

The zip file also includes 1x icons. I think he used those icons.

	Brian Grinstead [:bgrins] Assignee
	Comment 5 • 11 years ago

Darrin, can you send me a 2x version of the icon used for the split console command here: https://bugzilla.mozilla.org/attachment.cgi?id=8338714?

	Brian Grinstead [:bgrins] Assignee
	Comment 6 • 11 years ago

Updated patch to use 2x icons for command icons, toolbox toolbar icons, and docking / close icons.  Here is how each one is being handled:

* Toolbox toolbar icons: use 2x all the time.  These are specified as the src of an <image> tag, it is easiest to just load the full size icons all the time and have them automatically be scaled, since we don't have control of them in CSS.
* Docking / close icons: Use css background-image on the unused image tag for the toolbarbutton instead of list-style-image.  This allows us to target it similar to this:

    #toolbox-dock-buttons > toolbarbutton > image {
      -moz-appearance: none;
      width: 16px;
      height: 16px;
      background-position: 0 center;
      background-repeat: no-repeat;
    }

    #toolbox-dock-bottom > image {
      background-image: url("chrome://browser/skin/devtools/dock-bottom.png");
    }

    @media (min-resolution: 2dppx) {
      #toolbox-dock-buttons > toolbarbutton > image {
        background-size: 16px 16px;
      }

      #toolbox-dock-bottom > image {
        background-image: url("chrome://browser/skin/devtools/dock-bottom@2x.png");
      }
    }

* Command icons: same as docking and close icon, except also had to account for states in the image sprite with -moz-image-region by using background-position.

I've pushed to try to make sure that the jar.mn changes are working across each OS: https://tbpl.mozilla.org/?tree=Try&rev=db906d9ded83

	Brian Grinstead [:bgrins] Assignee
	Comment 7 • 11 years ago

Patch ready for review.  See Comment 6 for more information, but it basically moves all the icons for the top toolbar into a shared folder, while using the 2x versions of these icons when needed.

For testing retina displays, I've been switching the flag layout.css.devPixelsPerPx to 2.  Here is a try push for testing pre built binaries: https://tbpl.mozilla.org/?tree=Try&rev=94858d2038da.

	Brian Grinstead [:bgrins] Assignee
	Comment 8 • 11 years ago

Darrin,
As mentioned earlier, I need a 2x version of the command-console icon.  In addition to this, I wanted to share a screenshot showing a minor brightness difference with that icon (on the far left) versus the others, which were grabbed from Bug 837188.  The others seem a little bit brighter to me - so we may want to adjust the color in the console icon to match as well.

	Paul Rouget [:paul]
	Comment 9 • 11 years ago

Comment on attachment 8355305 [details] [diff] [review]
theme-shared-toolbar-icons.patch

Code looks good to me. I'm ok if we land this, but don't you think we could simplify this code?

* could we use the same code for the dock/close icons and the command icons?
* couldn't we use 2x icons all the time? (as in: not include any 1x icon). That would simplify the code and the overhead won't be much (memory footprint, CPU for downscaling and image quality). We already do it for the main icons, why not do it across all the icons?

Also, can you file bugs for the remaining icons (responsive mode, webconsole output, webconsole input prompt, inspector twisties and checkboxes, debugger breakpoint, debugger widget arrow, debugger icons (controls and blackboxing & breakpoints buttons), style editor sidebar arrow & eye, profiler icon)

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 10 • 11 years ago

(In reply to Brian Grinstead [:bgrins] from comment #8)
> As mentioned earlier, I need a 2x version of the command-console icon.  In
> addition to this, I wanted to share a screenshot showing a minor brightness
> difference with that icon (on the far left) versus the others, which were
> grabbed from Bug 837188.  The others seem a little bit brighter to me - so
> we may want to adjust the color in the console icon to match as well.

Maybe I'm just that old, but the split console icon looks a lot like the original Macintosh (or even my old iMac) to me, and making it brighter could presumably make it more noticeable. But perhaps that's a good thing. I'm certainly enjoying referring to it as the "Macintosh icon".

	Brian Grinstead [:bgrins] Assignee
	Comment 11 • 11 years ago

(In reply to Paul Rouget [:paul] from comment #9)
> Comment on attachment 8355305 [details] [diff] [review]
> theme-shared-toolbar-icons.patch
> 
> Code looks good to me. I'm ok if we land this, but don't you think we could
> simplify this code?
> 
> * could we use the same code for the dock/close icons and the command icons?

The command icons are a little different because they have different states so the background-size and background-position will be different.  We could share some of the CSS though - I'll try it and see if it makes things simpler or not.

> * couldn't we use 2x icons all the time? (as in: not include any 1x icon).
> That would simplify the code and the overhead won't be much (memory
> footprint, CPU for downscaling and image quality). We already do it for the
> main icons, why not do it across all the icons?

Using the 2x icon all the time is fine with me.  It would be especially nice when we pull in the light theme icons, this will bring the number of images for each icon down to 2 instead of 4.

	Brian Grinstead [:bgrins] Assignee
	Comment 12 • 11 years ago

Tested across OS with this try push: https://tbpl.mozilla.org/?tree=Try&rev=b08dacdec0f4.  This is good to go, except for the 2x "Macintosh" / split console icon.  This icon actually actually OK with just the 1x (not bad enough to hold up landing this).  I will update this icon along with all the other remaining ones in a separate bug.

	Brian Grinstead [:bgrins] Assignee
	Comment 13 • 11 years ago

Same as previous, but re-adds close.png for support in responsive mode and developer toolbar

	Brian Grinstead [:bgrins] Assignee
	Comment 14 • 11 years ago

https://hg.mozilla.org/integration/fx-team/rev/e84f5f38017e
https://tbpl.mozilla.org/?tree=Fx-Team&rev=e84f5f38017e

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 11 years ago

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

	Alfred Kayser
	Comment 16 • 11 years ago

Instead of pushing 2x icons, and then force scaling them back to 16x16, and hardcoding image paths in .js, one could also just set class names and let the theme assign the correct icons (including 1x/2x selection.