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)
Tracking
(firefox40 verified, firefox41 fixed)
VERIFIED
FIXED
Firefox 41
People
(Reporter: bgrins, Assigned: bgrins)
References
Details
Attachments
(2 files, 3 obsolete files)
6.36 KB,
patch
|
bgrins
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
60.79 KB,
image/png
|
Details |
The add.svg icon seems a little too light in the dark theme and dark in the light theme.
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
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 3•9 years ago
|
||
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.
Comment 4•9 years ago
|
||
(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.
Comment 5•9 years ago
|
||
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)
Comment 6•9 years ago
|
||
(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.
Comment 7•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8607181 -
Flags: ui-review?(shorlander) → ui-review+
Comment 8•9 years ago
|
||
(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>
Assignee | ||
Comment 9•9 years ago
|
||
(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.
Assignee | ||
Comment 10•9 years ago
|
||
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 11•9 years ago
|
||
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
Assignee | ||
Comment 12•9 years ago
|
||
(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 13•9 years ago
|
||
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+
Assignee | ||
Updated•9 years ago
|
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
Assignee | ||
Updated•9 years ago
|
Blocks: perf-polish
Assignee | ||
Comment 14•9 years ago
|
||
Converted to lower case
Attachment #8607177 -
Attachment is obsolete: true
Attachment #8607587 -
Attachment is obsolete: true
Attachment #8607630 -
Flags: review+
Assignee | ||
Comment 15•9 years ago
|
||
remote: https://hg.mozilla.org/integration/fx-team/rev/687eb2f66e38
Whiteboard: [fixed-in-fx-team]
Comment 16•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/687eb2f66e38
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
Assignee | ||
Updated•9 years ago
|
Blocks: perf-40-uplifts
Updated•9 years ago
|
Flags: qe-verify+
Assignee | ||
Comment 17•9 years ago
|
||
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?
Assignee | ||
Comment 18•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/11b76c3a915e
status-firefox40:
--- → fixed
Assignee | ||
Comment 19•9 years ago
|
||
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 20•9 years ago
|
||
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+
Comment 21•9 years ago
|
||
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)
Assignee | ||
Comment 22•9 years ago
|
||
(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)
Comment 23•9 years ago
|
||
(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!
Assignee | ||
Comment 24•9 years ago
|
||
(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.
Comment 25•9 years ago
|
||
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.
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•