Closed Bug 1166029 Opened 9 years ago Closed 9 years ago

DevTools SVG toolbar icons color doesn't match png toolbar icons

Categories

(DevTools :: Framework, defect)

40 Branch
defect
Not set
normal

Tracking

(firefox40 verified, firefox41 fixed)

VERIFIED FIXED
Firefox 41
Tracking Status
firefox40 --- verified
firefox41 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(2 files, 3 obsolete files)

The add.svg icon seems a little too light in the dark theme and dark in the light theme.
Attached patch add-button-color.patch (obsolete) — Splinter Review
Attached image add-rule-colors.png (obsolete) —
Stephen, what do you think about the change?  Just updated the 'add' icon color to make it match the other toolbar icons.  Or is it intentionally different?
Attachment #8607181 - Flags: ui-review?(shorlander)
Comment on attachment 8607177 [details] [diff] [review]
add-button-color.patch

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

You could use fill-opacity instead of setting a new color.
(In reply to Tim Nguyen [:ntim] (limited availability) from comment #3)
> Comment on attachment 8607177 [details] [diff] [review]
> add-button-color.patch
> 
> Review of attachment 8607177 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You could use fill-opacity instead of setting a new color.

Especially since the current color is the same as the one used for tab bar icons.
Can we make a svg copy of the add new tab button at the top of the browser? The add button actually looks a bit off from the general firefox theme.

@shorlander: any thoughts?
Flags: needinfo?(shorlander)
(In reply to Gabriel Luong [:gl] from comment #5)
> Can we make a svg copy of the add new tab button at the top of the browser?
> The add button actually looks a bit off from the general firefox theme.
The add.svg icon used to be a copy of a new tab button in the firefox theme, until Stephen changed it in bug 1136101 since it was blurry on non-retina screens.
Looks better. I copied the color from one of the existing SVG files. Do they have different styling that would affect the color?
Flags: needinfo?(shorlander)
Attachment #8607181 - Flags: ui-review?(shorlander) → ui-review+
(In reply to Gabriel Luong [:gl] from comment #5)
> Can we make a svg copy of the add new tab button at the top of the browser?
> The add button actually looks a bit off from the general firefox theme.
> 
> @shorlander: any thoughts?

yeah, I think these dimensions should make it the same size as the new tab icon:

  <g fill="#edf0f1">
    <rect x="3" y="7" width="10" height="2" />
    <rect x="7" y="3" width="2" height="10" />
  </g>
(In reply to Stephen Horlander [:shorlander] from comment #7)
> Looks better. I copied the color from one of the existing SVG files. Do they
> have different styling that would affect the color?

I see the color is used in other svg files: https://dxr.mozilla.org/mozilla-central/search?q=edf0f1&case=true&redirect=true.  Looks like they are mostly the performance tool (and one from the web audio editor):

http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/images/performance-icons.svg
http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/images/power.svg
http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/images/profiler-stopwatch.svg

I doubt these are intentionally different from the rest of the toolbar icons, and they should probably be updated to match this color.  I can do that here if you think we should.
Attached patch svg-fill-colors.patch (obsolete) — Splinter Review
This looks much more consistent.  Got ui-review+ for the initial change, this just includes other icons that were using the extra-bright colors (mostly perf tools + web audio editor).  IMO they fit better into the toolbar this way, but quick r? for Jordan to make sure it wasn't intentionally different.
Assignee: nobody → bgrinstead
Attachment #8607181 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8607587 - Flags: review?(jsantell)
Comment on attachment 8607587 [details] [diff] [review]
svg-fill-colors.patch

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

Note that I originally used #edf0f1 in the first SVG icons because it was the color used in the tab bar icons.
What's making the icons look brighter in the toolbar is the fact that no opacity is set.

::: browser/themes/shared/devtools/images/add.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" xmlns:xlink="http://www.w3.org/1999/xlink">
> +  <g fill="#BABEC3">

nit : We're privileging lower case hex in the code
(In reply to Tim Nguyen [:ntim] (limited availability) from comment #11)
> Comment on attachment 8607587 [details] [diff] [review]
> svg-fill-colors.patch
> 
> Review of attachment 8607587 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Note that I originally used #edf0f1 in the first SVG icons because it was
> the color used in the tab bar icons.
> What's making the icons look brighter in the toolbar is the fact that no
> opacity is set.

For this change, I just grabbed the used color from the 'pause' icon in the debugger so it would match that (without needing to compute the correct opacity)
Comment on attachment 8607587 [details] [diff] [review]
svg-fill-colors.patch

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

LGTM -- I think this was the color ntim's been using, and I used the same for the power.svg
Attachment #8607587 - Flags: review?(jsantell) → review+
Component: Developer Tools: Inspector → Developer Tools: Framework
Summary: Add rule button color doesn't match other toolbar buttons → DevTools SVG toolbar icons color doesn't match png toolbar icons
Blocks: perf-polish
Converted to lower case
Attachment #8607177 - Attachment is obsolete: true
Attachment #8607587 - Attachment is obsolete: true
Attachment #8607630 - Flags: review+
remote:   https://hg.mozilla.org/integration/fx-team/rev/687eb2f66e38
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/687eb2f66e38
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
Flags: qe-verify+
Comment on attachment 8607630 [details] [diff] [review]
svg-fill-colors.patch


Approval Request Comment
[Feature/regressing bug #]: 1167252, the new performance tool
[User impact if declined]: Won't ship the performance tool
[Describe test coverage new/current, TreeHerder]: There are try pushes in Bug 1167252 with all patches needing uplift
[Risks and why]: Requesting uplift for the accumulated changes in the performance tool since the 40 merge date, so these changes haven't had the full 6 weeks to bake.  Risks are generally contained within devtools, specifically within the performance panel.
[String/UUID change made/needed]: None
Attachment #8607630 - Flags: approval-mozilla-aurora?
Note: I had verbal confirmation for these uplifts from Sylvestre even before he's flagged them as a+.  See https://bugzilla.mozilla.org/show_bug.cgi?id=1167252#c26
Comment on attachment 8607630 [details] [diff] [review]
svg-fill-colors.patch

Change approved to skip one train as part of the spring campaign.
Attachment #8607630 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
The button is not visible on Aurora 40.0a2 (2015-06-09). Tested with Windows 7 (x64), Ubuntu 12.04 (x64) & 14.04 (x64) and Mac OS X 10.6.8.

Brian, am I missing something here? I noticed that the button is displayed on Nightly 41.0a1 (2015-06-09), but not as a plus sign - see: http://i.imgur.com/YT7Dbs0.png.
Flags: needinfo?(bgrinstead)
(In reply to Andrei Vaida, QA [:avaida] from comment #21)
> The button is not visible on Aurora 40.0a2 (2015-06-09). Tested with Windows
> 7 (x64), Ubuntu 12.04 (x64) & 14.04 (x64) and Mac OS X 10.6.8.
> 
> Brian, am I missing something here? I noticed that the button is displayed
> on Nightly 41.0a1 (2015-06-09), but not as a plus sign - see:
> http://i.imgur.com/YT7Dbs0.png.

Hi, sorry for the confusion.  The 'add rule' (plus sign icon) is hidden by default while we fix a few things with it (see Bug 1165122).  So it's expected to not see this on either 40 or 41.  This uplift was needed for the SVGs used by the performance tool - they are in the top toolbar on that tool.  The other icon you are seeing in Nightly is used for a different feature entirely (Bug 987365).
Flags: needinfo?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #22)
> Hi, sorry for the confusion.  The 'add rule' (plus sign icon) is hidden by
> default while we fix a few things with it (see Bug 1165122).  So it's
> expected to not see this on either 40 or 41.  This uplift was needed for the
> SVGs used by the performance tool - they are in the top toolbar on that
> tool.  The other icon you are seeing in Nightly is used for a different
> feature entirely (Bug 987365).

Right, I thought the patch submitted here was only for the 'add rule' button. I had a look at the before/after screenshot from Comment 2 once again, but I'm not seeing any obvious differences between the two. Could you please point out exactly what icons do you want to be verified here? Also, if there's a specific color the icons should have or any particular traits, that'd also be helpful. Thanks!
(In reply to Andrei Vaida, QA [:avaida] from comment #23)
> (In reply to Brian Grinstead [:bgrins] from comment #22)
> > Hi, sorry for the confusion.  The 'add rule' (plus sign icon) is hidden by
> > default while we fix a few things with it (see Bug 1165122).  So it's
> > expected to not see this on either 40 or 41.  This uplift was needed for the
> > SVGs used by the performance tool - they are in the top toolbar on that
> > tool.  The other icon you are seeing in Nightly is used for a different
> > feature entirely (Bug 987365).
> 
> Right, I thought the patch submitted here was only for the 'add rule'
> button.

So the patch that was uplifted for 40 is only for the Performance tool icons.  Take a look at this screenshot, the icons should be #babec3 as specified in the patch.

> I had a look at the before/after screenshot from Comment 2 once
> again, but I'm not seeing any obvious differences between the two. Could you
> please point out exactly what icons do you want to be verified here? Also,
> if there's a specific color the icons should have or any particular traits,
> that'd also be helpful. Thanks!

It's very subtle, but on the dark toolbox 'Before' screenshot, the 'add' button is a more bright white color and doesn't really match with the other icons in the toolbox.  But in 'After' it's a bit more dulled out and matches the other icons.
Thanks a lot for clarifying this, Brian!

This is confirmed fixed on Aurora 40.0a2 (2015-06-10), using Windows 7 (x64), Mac OS X 10.9.5 and Ubuntu 14.04 (x64).

The Performance tools' icons color is #BABEC3 on dark theme and #403D3B on the light theme.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.