1268591 - Make icons black by default and invert them for the dark theme

Status: RESOLVED FIXED

(DevTools :: Shared Components, enhancement)

Description

[Tim Nguyen :ntim]

This would avoid having to maintain a blacklist of non-inverted icons for both the Firebug theme and the Light theme. With this change, we can just apply filter: invert(1) to all dark theme icons without any blacklist.

Comment 1

[Tim Nguyen :ntim]

Attachment: Bug 1268591 - Make devtools icons black by default and invert them for dark theme.

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

Comment 2

[Tim Nguyen :ntim]

Attachment: Bug 1268591 - Support new invertIconForDarkTheme flag.

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

Comment 3

[Tim Nguyen :ntim]

Comment on attachment 8771039 [details]
Bug 1268591 - Make devtools icons black by default and invert them for dark theme.

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

Comment 4

[Tim Nguyen :ntim]

Comment on attachment 8771040 [details]
Bug 1268591 - Support new invertIconForDarkTheme flag.

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

Comment 5

[Tim Nguyen :ntim]

Comment on attachment 8771040 [details]
Bug 1268591 - Support new invertIconForDarkTheme flag.

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

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8771040 [details]
Bug 1268591 - Support new invertIconForDarkTheme flag.

https://reviewboard.mozilla.org/r/64296/#review61626

Looking at exclusively the code, this seems fine, but should probably also remove the invert Light icon code in addon-sdk, as well as update those docs are necessary.

But I don't have the context if this is a plan for something else, and the "why"s behind it, so someone working on devtools styles should probably give a thumbs up about this. Also, are there any extensions that would be affected by this change?

Comment 7

[Tim Nguyen :ntim]

(In reply to Jordan Santell [:jsantell] [@jsantell] (Away from Bugzilla for awhile) from comment #6)
> Comment on attachment 8771040 [details]
> Bug 1268591 - Support new invertIconForDarkTheme flag.
> 
> https://reviewboard.mozilla.org/r/64296/#review61626
> 
> Looking at exclusively the code, this seems fine, but should probably also
> remove the invert Light icon code in addon-sdk, as well as update those docs
> are necessary.
I kept the light theme flag for existing add-ons. There are 12 add-ons using it right now.

> But I don't have the context if this is a plan for something else, and the
> "why"s behind it, so someone working on devtools styles should probably give
> a thumbs up about this.
I'm doing this because it prevents bugs like bug 1285449 from happening over and over again.
I'll make sure to warn on MDN that invertIconForLightTheme is deprecated.

> Also, are there any extensions that would be affected by this change?
There are no effects on extensions since I've kept support for the old flag (the CSS needed is trivial).

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8771039 [details]
Bug 1268591 - Make devtools icons black by default and invert them for dark theme.

https://reviewboard.mozilla.org/r/64294/#review61708

Confirmed with Helen that she's good with this approach.  Just a couple issues I see.  Also please clean up commit message before landing (it's got some extra lines at the end)

::: devtools/client/themes/firebug-theme.css
(Diff revision 2)
> -.theme-firebug .command-button-invertable::before,
> -.theme-firebug #sources-toolbar image,
> -.theme-firebug [id$="pane-toggle"] > image,
> -.theme-firebug [id$="pane-toggle"]::before,
> -.theme-firebug #global-toolbar .devtools-button::before,
> -.theme-firebug #element-picker::before,

I think at least some of these need to still be here to match previous styles. For instance, try activating picker mode or the split console and see the full blue applied to the icons.

::: devtools/client/themes/toolbars.css:610
(Diff revision 2)
> -.theme-light #breadcrumb-separator-normal,
> -.theme-light .scrollbutton-up > .toolbarbutton-icon,
> -.theme-light .scrollbutton-down > .toolbarbutton-icon,
> -.theme-light #black-boxed-message-button .button-icon,
> -.theme-light #requests-menu-perf-notice-button .button-icon,
> -.theme-light #requests-menu-network-summary-button .button-icon,
> +#breadcrumb-separator-normal,
> +.scrollbutton-up > .toolbarbutton-icon,
> +.scrollbutton-down > .toolbarbutton-icon,
> +#black-boxed-message-button .button-icon,
> +#requests-menu-perf-notice-button .button-icon,
> +#canvas-debugging-empty-notice-button .button-icon,

Why were these two removed:

.theme-light #requests-menu-network-summary-button .button-icon,
.theme-light #toggle-breakpoints[checked] > image,

And this one added?

#canvas-debugging-empty-notice-button .button-icon,

In the dark theme when clicking toggle breakpoints the icon becomes black

Comment 9

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> Comment on attachment 8771039 [details]
> Bug 1268591 - Make devtools icons black by default and invert them for dark
> theme.
> 
> https://reviewboard.mozilla.org/r/64294/#review61708
> 
> Confirmed with Helen that she's good with this approach.  Just a couple
> issues I see.  Also please clean up commit message before landing (it's got
> some extra lines at the end)
hg doesn't seem very smart when folding patches, will fix that.

> ::: devtools/client/themes/firebug-theme.css
> (Diff revision 2)
> > -.theme-firebug .command-button-invertable::before,
> > -.theme-firebug #sources-toolbar image,
> > -.theme-firebug [id$="pane-toggle"] > image,
> > -.theme-firebug [id$="pane-toggle"]::before,
> > -.theme-firebug #global-toolbar .devtools-button::before,
> > -.theme-firebug #element-picker::before,
> 
> I think at least some of these need to still be here to match previous
> styles. For instance, try activating picker mode or the split console and
> see the full blue applied to the icons.
Ah, good catch, will fix that.

> ::: devtools/client/themes/toolbars.css:610
> (Diff revision 2)
> > -.theme-light #breadcrumb-separator-normal,
> > -.theme-light .scrollbutton-up > .toolbarbutton-icon,
> > -.theme-light .scrollbutton-down > .toolbarbutton-icon,
> > -.theme-light #black-boxed-message-button .button-icon,
> > -.theme-light #requests-menu-perf-notice-button .button-icon,
> > -.theme-light #requests-menu-network-summary-button .button-icon,
> > +#breadcrumb-separator-normal,
> > +.scrollbutton-up > .toolbarbutton-icon,
> > +.scrollbutton-down > .toolbarbutton-icon,
> > +#black-boxed-message-button .button-icon,
> > +#requests-menu-perf-notice-button .button-icon,
> > +#canvas-debugging-empty-notice-button .button-icon,
> 
> Why were these two removed:
> 
> .theme-light #requests-menu-network-summary-button .button-icon,
> .theme-light #toggle-breakpoints[checked] > image,
> 
> And this one added?
> 
> #canvas-debugging-empty-notice-button .button-icon,
#requests-menu-network-summary-button was changed to be a .devtools-toolbarbutton when I moved the netmonitor toolbar to the top, so the filter is already covered by the .devtools-toolbarbutton styles.

#canvas-debugging-empty-notice-button .button-icon is to fix bug 1286913.

> In the dark theme when clicking toggle breakpoints the icon becomes black
Good catch, I had removed the line accidently.

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8771040 [details]
Bug 1268591 - Support new invertIconForDarkTheme flag.

https://reviewboard.mozilla.org/r/64296/#review61712

Agreed that the code looks fine, challenge is around explanation / documentation as pointed out in Comment 6

::: devtools/client/themes/toolbox.css:138
(Diff revision 3)
>    max-height: 16px;
>    width: 16px; /* Prevents collapse during theme switching */
>  }
>  
> +/* Support invertable icon flags and make icon white when it's on a blue background */
> +.theme-light .devtools-tab[icon-invertable="light-theme"]:not([selected]) > image,

Why `:not([selected])` - this will get picked up by the more specific selector below

::: devtools/client/themes/toolbox.css:139
(Diff revision 3)
>    width: 16px; /* Prevents collapse during theme switching */
>  }
>  
> +/* Support invertable icon flags and make icon white when it's on a blue background */
> +.theme-light .devtools-tab[icon-invertable="light-theme"]:not([selected]) > image,
> +.devtools-tab[icon-invertable="dark-theme"][selected] > image {

Shouldn't this be `.theme-dark .devtools-tab[icon-invertable="dark-theme"][selected] > image`

::: devtools/client/themes/toolbox.css:145
(Diff revision 3)
> +  filter: invert(1);
> +}
> +
> +/* Since "highlighted" icons are green, we should omit the filter */
> +.theme-dark .devtools-tab[icon-invertable="light-theme"] > image,
> +.devtools-tab[icon-invertable="light-theme"][selected] > image,

Shouldn't this be `.devtools-tab[icon-invertable="light-theme"][selected] > image`

Comment 11

[Tim Nguyen :ntim]

Comment on attachment 8771039 [details]
Bug 1268591 - Make devtools icons black by default and invert them for dark theme.

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

Comment 12

[Tim Nguyen :ntim]

Comment on attachment 8771040 [details]
Bug 1268591 - Support new invertIconForDarkTheme flag.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64296/diff/3-4/

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8771039 [details]
Bug 1268591 - Make devtools icons black by default and invert them for dark theme.

https://reviewboard.mozilla.org/r/64294/#review63046

A couple of comments.  Also, this needs rebasing around the firebug theme

::: devtools/client/themes/toolbars.css:620
(Diff revision 3)
> -.theme-light .event-tooltip-debugger-icon {
> +.event-tooltip-debugger-icon {
>    filter: var(--icon-filter);
>  }
>  
> -/* Reset the filter defined above */
> -.theme-light .command-button:not(.command-button-invertable) {
> +/* Since the tab selected background is blue, we should invert the icon */
> +.devtools-tab[icon-invertable][selected] > image {

The not(.command-button-invertable) case is lost in this patch, is that intentional?

::: devtools/client/themes/toolbars.css:626
(Diff revision 3)
> -  filter: none !important;
> +  filter: invert(1);
>  }
>  
> -/* Since selected backgrounds are blue, we want to use the normal
> - * (light) icons. */
> -.theme-light .devtools-tab[icon-invertable][selected] > image,
> +/* Since "highlighted" icons are green, we should omit the filter */
> +.devtools-tab[icon-invertable][highlighted] > image {
> +  filter: none;

This is leading to a state where where a paused *and* selected debugger icon is losing the expected filter (see http://imgur.com/a/ffcDC).  The filter should only be set to none if it's not selected in this case

Comment 14

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #13)
> > -/* Since selected backgrounds are blue, we want to use the normal
> > - * (light) icons. */
> > -.theme-light .devtools-tab[icon-invertable][selected] > image,
> > +/* Since "highlighted" icons are green, we should omit the filter */
> > +.devtools-tab[icon-invertable][highlighted] > image {
> > +  filter: none;
> 
> This is leading to a state where where a paused *and* selected debugger icon
> is losing the expected filter (see http://imgur.com/a/ffcDC).  The filter
> should only be set to none if it's not selected in this case

nm, I see that the second patch in the series fixes this case up

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8771040 [details]
Bug 1268591 - Support new invertIconForDarkTheme flag.

https://reviewboard.mozilla.org/r/64296/#review63054

A couple of questions still - unless if there's a good reason to generally apply these filters to all themes, I'd rather be very specific about the root on which it's applied to make it easier to reason about later on.

::: devtools/client/themes/toolbox.css:139
(Diff revision 4)
>    width: 16px; /* Prevents collapse during theme switching */
>  }
>  
> +/* Support invertable icon flags and make icon white when it's on a blue background */
> +.theme-light .devtools-tab[icon-invertable="light-theme"]:not([selected]) > image,
> +.devtools-tab[icon-invertable="dark-theme"][selected] > image {

Can you remind me when this isn't 

`.theme-dark .devtools-tab[icon-invertable="dark-theme"][selected] > image`

I know we had a discussion about this, but I can't find it now.  Since this is specifically targeting invertIconForDarkTheme I don't see why it isn't prefixed with .theme-dark here.

::: devtools/client/themes/toolbox.css:145
(Diff revision 4)
> +  filter: invert(1);
> +}
> +
> +/* Since "highlighted" icons are green, we should omit the filter */
> +.theme-dark .devtools-tab[icon-invertable="light-theme"] > image,
> +.devtools-tab[icon-invertable="light-theme"][selected] > image,

Shouldn't this be `.theme-light .devtools-tab[icon-invertable="light-theme"][selected] > image` (for the same reason as above)

Comment 16

[Tim Nguyen :ntim]

Comment on attachment 8771039 [details]
Bug 1268591 - Make devtools icons black by default and invert them for dark theme.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64294/diff/3-4/

Comment 17

[Tim Nguyen :ntim]

Comment on attachment 8771040 [details]
Bug 1268591 - Support new invertIconForDarkTheme flag.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64296/diff/4-5/

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8771039 [details]
Bug 1268591 - Make devtools icons black by default and invert them for dark theme.

https://reviewboard.mozilla.org/r/64294/#review63068

Comment 19

[Brian Grinstead [:bgrins]]

Comment on attachment 8771040 [details]
Bug 1268591 - Support new invertIconForDarkTheme flag.

https://reviewboard.mozilla.org/r/64296/#review63070

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/ef44d06bef2c
Make devtools icons black by default and invert them for dark theme. r=bgrins a=kwierso CLOSED TREE
https://hg.mozilla.org/integration/fx-team/rev/f055f19a7829
Support new invertIconForDarkTheme flag. r=bgrins, jsantell a=kwierso CLOSED TREE

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/0c5094d6f5d1
followup: correct fill of some white SVGs, and clean them. r=me DONTBUILD

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ef44d06bef2c
https://hg.mozilla.org/mozilla-central/rev/f055f19a7829
https://hg.mozilla.org/mozilla-central/rev/0c5094d6f5d1