1399886 - Stop using SVG filters for devtools icons

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[Julian Descottes [:jdescottes]]

Follow up to Bug 1399028. In this bug we introduced some new SVG filters in order to highlight the selected tool icon. 

We could achieve the same effect only using context-fill, see https://developer.mozilla.org/en-US/docs/Web/CSS/-moz-context-properties

This would be easier to maintain because we wouldn't have to define filter matrices, that need to remain in sync with our CSS variables. Also this would have no effect for themes that are using real images instead of SVGs , such as the Firebug theme.

In order to do that we need to first modify our SVGs to use fill="context-fill", and then instead of using 

> filter: var(--theme-icon-checked-filter); 

(or similar) we need to use

> fill: var(--theme-icon-checked-color);
--theme-icon-checked-color being a new CSS variable pointing to the appropriate color we need to use.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1399886 - use fill from CSS to set the fill color of devtools icons;

Review commit: https://reviewboard.mozilla.org/r/185606/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/185606/

Comment 2

[Julian Descottes [:jdescottes]]

I had a got at this, and the current patch switches most of our filter usage to context-fill. A few things though:
- we have to keep the checked filter as it is also applied to a png (chrome://devtools/skin/images/vview-open-inspector.png)
- context-fill is only supported in Firefox (and even then, not for webcontent), which means we need an alternative for tools running inside launchpad

On the plus side, this allows to get rid of 4 filters, 3 "highlighted" icons + the logic linked to it. And most importantly allows to reuse the same variables for colors rather than maintaining transformation matrices. 

Using context-fill also makes it easier to work with "non-SVG" icons. Since it only applies on SVG, no need to explicitly protect icons that don't use SVG.

Comment 3

[Julian Descottes [:jdescottes]]

(In reply to Julian Descottes [:jdescottes] from comment #2)
> - context-fill is only supported in Firefox (and even then, not for
> webcontent), which means we need an alternative for tools running inside
> launchpad
> 

Nevermind, the debugger only uses inline SVG, so they already use fill in CSS to set the color of their SVGs.

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1399886 - modify devtools SVG files to use fill=context-fill;

Review commit: https://reviewboard.mozilla.org/r/186402/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186402/

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1399886 - use fill to highlight devtools toolbar icons;

Instead of duplicating SVGs to apply a different fill color, we
can use fill from css + fill="context-fill" to achieve the same
effect. This applies to all tools that have a highlighted state
for their icon: debugger, memory and performance tools.

Review commit: https://reviewboard.mozilla.org/r/186472/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186472/

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1399886 - remove unnecessary invertable CSS classes on devtools icons;

Using fill instead of filter we don't need to define each icon as
invertable or not. If the icon is a SVG and supports fill="context-fill"
then it will be inverted/highlighted etc... as expected.

If not then it won't be impacted by DevTools themes.

Review commit: https://reviewboard.mozilla.org/r/186494/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186494/

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8914325 [details]
Bug 1399886 - use fill from CSS to set the fill color of devtools icons;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185606/diff/1-2/

Comment 8

[Julian Descottes [:jdescottes]]

Try https://treeherder.mozilla.org/#/jobs?repo=try&revision=059c8fec4ebe0df513db55952ddf24edae985d69

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8915270 [details]
Bug 1399886 - modify devtools SVG files to use fill=context-fill;

https://reviewboard.mozilla.org/r/186402/#review191708

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8914325 [details]
Bug 1399886 - use fill from CSS to set the fill color of devtools icons;

https://reviewboard.mozilla.org/r/185606/#review191710

Comment 11

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8915271 [details]
Bug 1399886 - use fill to highlight devtools toolbar icons;

https://reviewboard.mozilla.org/r/186472/#review191712

::: devtools/client/themes/variables.css:30
(Diff revision 1)
>    --theme-tab-toolbar-background: var(--grey-10);
>    --theme-toolbar-background: var(--grey-10);
>    --theme-toolbar-color: var(--grey-90);
>    --theme-toolbar-selected-color: var(--blue-60);
>    --theme-toolbar-checked-color: var(--blue-60);
> +  --theme-toolbar-highlighted-color: #5FC749;

I think we might want to use a photon green color here.

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8915272 [details]
Bug 1399886 - remove unnecessary invertable CSS classes on devtools icons;

https://reviewboard.mozilla.org/r/186494/#review191714

Comment 13

[Julian Descottes [:jdescottes]]

Thanks for the reviews! 

(In reply to Gabriel [:gl] (ΦωΦ) from comment #11)
> Comment on attachment 8915271 [details]
> Bug 1399886 - use fill to highlight devtools toolbar icons;
> 
> https://reviewboard.mozilla.org/r/186472/#review191712
> 
> ::: devtools/client/themes/variables.css:30
> (Diff revision 1)
> >    --theme-tab-toolbar-background: var(--grey-10);
> >    --theme-toolbar-background: var(--grey-10);
> >    --theme-toolbar-color: var(--grey-90);
> >    --theme-toolbar-selected-color: var(--blue-60);
> >    --theme-toolbar-checked-color: var(--blue-60);
> > +  --theme-toolbar-highlighted-color: #5FC749;
> 
> I think we might want to use a photon green color here.

Yep, thought we could handle that as a follow up. The current fill color for highlighted icons is hardcoded to #5FC749. The closest photon colors to #5FC749 are
- Green 50 (#30e60b) -> a bit lighter
- Green 60 (#12bc00) -> a bit darker

My proposal would be to go for:
- Green 60 in light theme
- Green 50 in dark theme

I tried a few combinations, you can see the summary at: https://docs.google.com/a/mozilla.com/document/d/1OtYNxvmcuTbsidnSA-NsakSf_cCo8VbH7MwJwePTMDI/edit?usp=sharing

Victoria: are you okay with those colors?

Comment 14

[Julian Descottes [:jdescottes]]

Attachment: Bug 1399886 - use photon colors for highlighted devtools toolbar icons;

Review commit: https://reviewboard.mozilla.org/r/186804/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186804/

Comment 15

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8915612 [details]
Bug 1399886 - use photon colors for highlighted devtools toolbar icons;

https://reviewboard.mozilla.org/r/186804/#review191912

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1229b8a6eefc
modify devtools SVG files to use fill=context-fill;r=gl
https://hg.mozilla.org/integration/autoland/rev/62cc82c624ea
use fill from CSS to set the fill color of devtools icons;r=gl
https://hg.mozilla.org/integration/autoland/rev/4b4f0c7ccd9b
use fill to highlight devtools toolbar icons;r=gl
https://hg.mozilla.org/integration/autoland/rev/534f2a0caa2c
remove unnecessary invertable CSS classes on devtools icons;r=gl

Comment 17

[Ionuț Goldan [:igoldan]]

This brought noticeable perf improvements! Good work!
== Change summary for alert #9850 (as of October 05 2017 18:19 UTC) ==

Improvements:

  4%  damp summary windows7-32 opt e10s     285.47 -> 272.78
  4%  damp summary windows10-64 opt e10s    266.57 -> 256.51
  4%  damp summary windows7-32 pgo e10s     227.06 -> 218.65
  3%  damp summary windows10-64 pgo e10s    223.46 -> 216.43
  3%  damp summary linux64 opt e10s         268.86 -> 260.92
  3%  damp summary linux64 pgo e10s         240.37 -> 234.20
  2%  damp summary osx-10-10 opt e10s       310.35 -> 304.26

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9850

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1229b8a6eefc
https://hg.mozilla.org/mozilla-central/rev/62cc82c624ea
https://hg.mozilla.org/mozilla-central/rev/4b4f0c7ccd9b
https://hg.mozilla.org/mozilla-central/rev/534f2a0caa2c

Comment 19

[Victoria Wang [:victoria]]

Thanks for the clear screenshots, Julian! I agree with your suggestions of Green 60 in light theme and Green 50 in dark theme. 

Two other changes I want to make, which should probably made in separate bugs/discussions: remove background color of those tabs unless there is some reasoning I don't understand there. Also, maybe some kind of "stopped" icon appearing would be make the Debugger indicator more clear.

Comment 21

[Tim Nguyen :ntim]

The invertIconForLightTheme/DarkTheme classes was introduced for add-ons. If support for it is removed, the webextension references should also be removed: https://dxr.mozilla.org/mozilla-central/search?q=invertIconFor&redirect=false

Comment 22

[Tim Nguyen :ntim]

Attachment: Style Editor bug

Julian, Gabe, have you considered using fill: currentColor everywhere (with different opacities if plain black/white is too strong) ?

Trying to figure out what the right variable for the icon fill in every case, when the information is already in the text color (most of the time), seems a bit odd and causes bugs like the one shown in the screenshot.

On the Firefox UI side, using this approach for the close icons has proved very effective, by removing many many custom rules deciding whether the icon should white/black in different situations: bug 1385702.

Comment 23

[Julian Descottes [:jdescottes]]

(In reply to Tim Nguyen :ntim from comment #21)
> The invertIconForLightTheme/DarkTheme classes was introduced for add-ons. If
> support for it is removed, the webextension references should also be
> removed:
> https://dxr.mozilla.org/mozilla-central/search?q=invertIconFor&redirect=false

Will file a follow up, thanks.

(In reply to Tim Nguyen :ntim from comment #22)
> Created attachment 8917043 [details]
> Style Editor bug
> 
> Julian, Gabe, have you considered using fill: currentColor everywhere (with
> different opacities if plain black/white is too strong) ?
> 

Not really, my goal was to stop using filters to be able to reuse variables. I overlooked the usage of the -1 filter in the netmonitor and will log a follow up to fix it.

We can experiment with currentColor in another bug.

Comment 24

[Julian Descottes [:jdescottes]]

(In reply to Julian Descottes [:jdescottes] from comment #23)
> Not really, my goal was to stop using filters to be able to reuse variables.
> I overlooked the usage of the -1 filter in the netmonitor and will log a
> follow up to fix it.

s/netmonitor/style editor ...