Closed Bug 947309 Opened 7 years ago Closed 7 years ago

DevTools themes - use new icons for toolbar

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 29

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

(Whiteboard: [good first verify])

Attachments

(2 files, 5 obsolete files)

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.
Attached patch toolbar-icons-shared.patch (obsolete) — Splinter Review
This patch removes individual icons from windows/ linux/ and osx/ folders and instead references shared/devtools/images/*
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Blocks: 837188
Attached patch toolbar-icons-shared.patch (obsolete) — Splinter Review
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.
Attachment #8343849 - Attachment is obsolete: true
Attachment #8343971 - Flags: review?(paul)
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.
Attachment #8343971 - Flags: review?(paul)
(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.
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?
Flags: needinfo?(dhenein)
Attached patch theme-shared-toolbar-icons.patch (obsolete) — Splinter Review
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
Attachment #8343971 - Attachment is obsolete: true
Attached patch theme-shared-toolbar-icons.patch (obsolete) — Splinter Review
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.
Attachment #8355235 - Attachment is obsolete: true
Attachment #8355305 - Flags: review?(paul)
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.
Flags: needinfo?(dhenein)
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)
Attachment #8355305 - Flags: review?(paul) → review+
(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".
(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.
Attached patch theme-shared-toolbar-icons.patch (obsolete) — Splinter Review
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.
Attachment #8355305 - Attachment is obsolete: true
Attachment #8355598 - Flags: review+
Same as previous, but re-adds close.png for support in responsive mode and developer toolbar
Attachment #8355598 - Attachment is obsolete: true
Attachment #8355602 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e84f5f38017e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
Blocks: 957160
Depends on: 957291
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.
Whiteboard: [good first verify]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.