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.

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

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

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 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 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 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?

(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 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

(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 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 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 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 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

(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 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 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 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 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 on attachment 8771040 [details] Bug 1268591 - Support new invertIconForDarkTheme flag. https://reviewboard.mozilla.org/r/64296/#review63070

Pushed by ntim.bugs@gmail.com: 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

Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/fx-team/rev/0c5094d6f5d1 followup: correct fill of some white SVGs, and clean them. r=me DONTBUILD

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