Closed Bug 1268591 Opened 4 years ago Closed 3 years ago

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

Categories

(DevTools :: CSS and Themes, enhancement)

enhancement
Not set

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Tracking Status
firefox50 --- fixed

People

(Reporter: ntim, Assigned: ntim)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
Severity: normal → enhancement
Depends on: 1260523
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Blocks: 1286913
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/
Attachment #8771040 - Flags: review?(jsantell) → review+
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).
Attachment #8771039 - Flags: review?(bgrinstead)
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`
Attachment #8771040 - Flags: review?(bgrinstead)
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/
Attachment #8771039 - Flags: review?(bgrinstead)
Attachment #8771040 - Flags: review?(bgrinstead)
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
Attachment #8771039 - Flags: review?(bgrinstead)
(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
Attachment #8771040 - Flags: review?(bgrinstead)
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/
Attachment #8771039 - Flags: review?(bgrinstead)
Attachment #8771040 - Flags: review?(bgrinstead)
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
Attachment #8771039 - Flags: review?(bgrinstead) → review+
Comment on attachment 8771040 [details]
Bug 1268591 - Support new invertIconForDarkTheme flag.

https://reviewboard.mozilla.org/r/64296/#review63070
Attachment #8771040 - Flags: review?(bgrinstead) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Depends on: 1288888
Depends on: 1288897
Depends on: 1288963
Product: Firefox → DevTools
Component: Framework → CSS and Themes
You need to log in before you can comment on or make changes to this bug.