1173397 - Some icons should not be inverted in light theme when checked

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect)

Description

[Tim Nguyen :ntim]

Icons I have in mind : Pseudo class lock panel icon, debugger toolbar icons, perf filter and record icons.

Comment 1

[Tim Nguyen :ntim]

Attachment: Patch

Comment 2

[Tim Nguyen :ntim]

Attachment: Patch v1.1

Fixed filter button in perf tool not being inverted

Comment 3

[Brian Grinstead [:bgrins]]

We looked at the toggle pseudo class button, and confirmed that the icon isn't being inverted.  We need to add `.theme-light .devtools-button[checked]::before` to the list for filter: none.  The issue is that we also should remove the blue background color for .devtools-button[checked], but we can't do that right now because the performance tool is using the background color for it's toolbar buttons.

Ideally, we would remove the blue checked color and hover color, then use opacity when hovering/active like the command buttons in the tab bar.  To get the blue background when checked (like the perf tool is doing), we would probably use a different attribute - maybe kind of like [text-as-image] or [standalone] is doing to control styling.

Comment 4

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> We looked at the toggle pseudo class button, and confirmed that the icon
> isn't being inverted.  We need to add `.theme-light
> .devtools-button[checked]::before` to the list for filter: none.  The issue
> is that we also should remove the blue background color for
> .devtools-button[checked], but we can't do that right now because the
> performance tool is using the background color for it's toolbar buttons.
> 
> Ideally, we would remove the blue checked color and hover color, then use
> opacity when hovering/active like the command buttons in the tab bar.  To
> get the blue background when checked (like the perf tool is doing), we would
> probably use a different attribute - maybe kind of like [text-as-image] or
> [standalone] is doing to control styling.

Can we do those changes in another bug ? Then we can have someone from UX re-evaluating all the toolbar button styling altogether.

Comment 5

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

Attachment: Icon-Light-Theme

There seems to be some problems with icons in the light theme. That needs to be addressed.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Gabriel Luong [:gl] from comment #5)
> Created attachment 8624806 [details]
> Icon-Light-Theme
> 
> There seems to be some problems with icons in the light theme. That needs to
> be addressed.

This is with the patch from this bug applied?

Comment 7

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

(In reply to Brian Grinstead [:bgrins] from comment #6)
> (In reply to Gabriel Luong [:gl] from comment #5)
> > Created attachment 8624806 [details]
> > Icon-Light-Theme
> > 
> > There seems to be some problems with icons in the light theme. That needs to
> > be addressed.
> 
> This is with the patch from this bug applied?

Ya, that was what I saw with the patch applied on the light theme.

Comment 8

[Tim Nguyen :ntim]

Comment on attachment 8621711 [details] [diff] [review]
Patch v1.1

Review of attachment 8621711 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, some of these debugger changes are wrong. Will update the patch later.

Comment 9

[Tim Nguyen :ntim]

Attachment: MozReview Request: Bug 1173397 - Remove blue variants of devtools icons. r=bgrins

Bug 1173397 - Remove blue variants of devtools icons. r=bgrins

Comment 10

[Tim Nguyen :ntim]

Attachment: MozReview Request: Bug 1173397 - Refactor devtools toolbar button code. r=bgrins

Bug 1173397 - Refactor devtools toolbar button code. r=bgrins

Comment 11

[Tim Nguyen :ntim]

Comment on attachment 8684691 [details]
MozReview Request: Bug 1173397 - Remove blue variants of devtools icons. r=bgrins

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

Comment 12

[Tim Nguyen :ntim]

Comment on attachment 8684692 [details]
MozReview Request: Bug 1173397 - Refactor devtools toolbar button code. r=bgrins

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

Comment 13

[Tim Nguyen :ntim]

Comment on attachment 8684692 [details]
MozReview Request: Bug 1173397 - Refactor devtools toolbar button code. r=bgrins

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

Comment 14

[Tim Nguyen :ntim]

Comment on attachment 8684691 [details]
MozReview Request: Bug 1173397 - Remove blue variants of devtools icons. r=bgrins

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

Comment 15

[Tim Nguyen :ntim]

Comment on attachment 8684692 [details]
MozReview Request: Bug 1173397 - Refactor devtools toolbar button code. r=bgrins

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

Comment 16

[Tim Nguyen :ntim]

I decided to go with a SVG filter to apply the blue color on the checked button icons, this approach will automatically work on all icon buttons that are added in the future, which removes the inconsistencies across tools. Note that the blue color has been slightly changed to match the command button blue color (the colors that were in the SVG was sampled from an icon that existed before the UX refresh, while the command button are from the UX refresh).

Part 1 removes the blue variants from each devtools icon, and also removes a few unused images (newtab-*.png and commandline.png).

Part 2 refactors the toolbar button code, without any big visual change and also fixes this bug using the method described above.

Note that I've only applied the approach for the devtools toolbar icons, not the command buttons or the developer toolbar.

Comment 17

[Tim Nguyen :ntim]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=83422d362827

Comment 18

[Tim Nguyen :ntim]

Comment on attachment 8684692 [details]
MozReview Request: Bug 1173397 - Refactor devtools toolbar button code. r=bgrins

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

Comment 19

[Tim Nguyen :ntim]

Comment on attachment 8684692 [details]
MozReview Request: Bug 1173397 - Refactor devtools toolbar button code. r=bgrins

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

Comment 20

[Tim Nguyen :ntim]

Comment on attachment 8684692 [details]
MozReview Request: Bug 1173397 - Refactor devtools toolbar button code. r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24625/diff/6-7/

Comment 21

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=22ad79e50bb4

Comment 22

[Brian Grinstead [:bgrins]]

Helen, can you take a look at the UI with the patches applied and give some feedback?  In particular, the debugger play / pause button has changed so that there isn't a background color on it anymore when the play button is visible, instead the icon color changes to blue.  You can import them both by using this URL: https://reviewboard.mozilla.org/r/24621/diff/raw/.

Comment 23

[Brian Grinstead [:bgrins]]

Attachment: debugger-icons.png

Screenshot of the particular change I'm talking about.  Tim, is there anything else that's changing visually as a result of these patches?

Comment 24

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #23)
> Created attachment 8685500 [details]
> debugger-icons.png
> 
> Screenshot of the particular change I'm talking about.  Tim, is there
> anything else that's changing visually as a result of these patches?

The icon not being blue in the before screenshot is a bug, if you check the dark theme, the icon is blue. That's what the bug was originally about.

Anyway here are the other visual changes with these patches:
- No hover/active state background for icon-buttons, only the opacity changes (the most obvious change)
- The new blue color for the pseudo class lock and stopwatch icons is a bit lighter (as mentioned in comment 16), it's now consistent with the command buttons and the debugger icons. The old blue was sampled from a PNG image from before the UX refresh
- The options button icon now turns blue when its menu is opened (affects debugger, style editor and perf tool)

Comment 25

[Brian Grinstead [:bgrins]]

Attachment: different-blue-pickers.png

I'm fine with the 'new blue', although maybe the main element picker's active state should also be updated to match (see the different between the state in animation inspector and the devtools tab strip in this screenshot).

Comment 26

[Brian Grinstead [:bgrins]]

Comment on attachment 8684692 [details]
MozReview Request: Bug 1173397 - Refactor devtools toolbar button code. r=bgrins

https://reviewboard.mozilla.org/r/24625/#review22347

Looks like a great cleanup overall, thanks!  Here are a couple of things I've noticed with the patch applied

::: devtools/client/themes/debugger.css:65
(Diff revision 7)
> +#sources-toolbar .devtools-toolbarbutton[checked] {

Something is wrong with these icons with this patch applied.  They disappear in the light theme when they are checked.

Shouldn't .theme-light #sources-toolbar .devtools-toolbarbutton[checked] be removed from the list in toolbars.css, instead of specifically removing the filter here?  Anyway, the specificity is lower here compared to that one.

::: devtools/client/themes/webconsole.css:530
(Diff revision 7)
> -  background-position: -32px 0;
> +  filter: url(images/filters.svg#checked-icon-state);

This pattern of using:

`filter: url(images/filters.svg#checked-icon-state)`

for hover and:

`filter: url(images/filters.svg#checked-icon-state) brightness(0.9);`

for active could be simplified by introducing a state in the filter file for hover directly: images/filters.svg#hover-icon-state.

Comment 27

[Brian Grinstead [:bgrins]]

Comment on attachment 8684691 [details]
MozReview Request: Bug 1173397 - Remove blue variants of devtools icons. r=bgrins

https://reviewboard.mozilla.org/r/24623/#review22349

First part seems fine (obv we need to finalize second part before landing anything)

Comment 28

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #26)
> Comment on attachment 8684692 [details]
> MozReview Request: Bug 1173397 - Refactor devtools toolbar button code.
> r=bgrins
> 
> https://reviewboard.mozilla.org/r/24625/#review22347
> 
> Looks like a great cleanup overall, thanks!  Here are a couple of things
> I've noticed with the patch applied
> 
> ::: devtools/client/themes/debugger.css:65
> (Diff revision 7)
> > +#sources-toolbar .devtools-toolbarbutton[checked] {
> 
> Something is wrong with these icons with this patch applied.  They disappear
> in the light theme when they are checked.
> 
> Shouldn't .theme-light #sources-toolbar .devtools-toolbarbutton[checked] be
> removed from the list in toolbars.css, instead of specifically removing the
> filter here?  Anyway, the specificity is lower here compared to that one.
The icon issue isn't about specificity, filter: invert(1) is applied, but the problem is that it's applied twice, once on the button image, once on the button itself, anyway, I'll fix this issue by applying the blue filter on the button image instead of the button itself.

> ::: devtools/client/themes/webconsole.css:530
> (Diff revision 7)
> > -  background-position: -32px 0;
> > +  filter: url(images/filters.svg#checked-icon-state);
> 
> This pattern of using:
> 
> `filter: url(images/filters.svg#checked-icon-state)`
> 
> for hover and:
> 
> `filter: url(images/filters.svg#checked-icon-state) brightness(0.9);`
> 
> for active could be simplified by introducing a state in the filter file for
> hover directly: images/filters.svg#hover-icon-state.
I guess you meant #checked-active-icon-state here. 

Anyway, I'm too happy with doing this because these styles are really specific to the variables-view icons. I also don't think it simplifies things a lot, the filter value would be a tad bit shorter (9 less chars), but we'd have to introduce a new filter.

I'd ideally want to clean up these styles later as well, and possibly re-investigate those styles to be more consistent with other icon buttons.
What do you think ?

Comment 29

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #25)
> Created attachment 8685551 [details]
> different-blue-pickers.png
> 
> I'm fine with the 'new blue', although maybe the main element picker's
> active state should also be updated to match (see the different between the
> state in animation inspector and the devtools tab strip in this screenshot).

The issue here is that we're using the icon with the lowest opacity in the icon sprite which causes the blue to have lower opacity. Bug 1223701 will fix the issue, but if you like, I can hardcode the background-position so we use the icon with the most opacity. What do you prefer ?

Comment 30

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen [:ntim] from comment #29)
> (In reply to Brian Grinstead [:bgrins] from comment #25)
> > Created attachment 8685551 [details]
> > different-blue-pickers.png
> > 
> > I'm fine with the 'new blue', although maybe the main element picker's
> > active state should also be updated to match (see the different between the
> > state in animation inspector and the devtools tab strip in this screenshot).
> 
> The issue here is that we're using the icon with the lowest opacity in the
> icon sprite which causes the blue to have lower opacity. Bug 1223701 will
> fix the issue, but if you like, I can hardcode the background-position so we
> use the icon with the most opacity. What do you prefer ?

Do you think it's likely Bug 1223701 will ship in the same release as this one?  I'm fine with doing them separately if so but don't want to ship it in DE with the different colors.  I know it's subtle but it also sounds like it's an easy fix

Comment 31

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen [:ntim] from comment #28)
> > ::: devtools/client/themes/webconsole.css:530
> > (Diff revision 7)
> > > -  background-position: -32px 0;
> > > +  filter: url(images/filters.svg#checked-icon-state);
> > 
> > This pattern of using:
> > 
> > `filter: url(images/filters.svg#checked-icon-state)`
> > 
> > for hover and:
> > 
> > `filter: url(images/filters.svg#checked-icon-state) brightness(0.9);`
> > 
> > for active could be simplified by introducing a state in the filter file for
> > hover directly: images/filters.svg#hover-icon-state.
> I guess you meant #checked-active-icon-state here. 
> 
> Anyway, I'm too happy with doing this because these styles are really
> specific to the variables-view icons. I also don't think it simplifies
> things a lot, the filter value would be a tad bit shorter (9 less chars),
> but we'd have to introduce a new filter.
> 
> I'd ideally want to clean up these styles later as well, and possibly
> re-investigate those styles to be more consistent with other icon buttons.
> What do you think ?

Hm I didn't realize this was just for the vview icons.. Ideally .ruleview-selectorhighlighter, .animation-target .node-highlighter, .elementNode, and .open-inspector would instead be using a shared class for styling.  That alone would make things much better.  Probably scope creep here, so I'm fine with doing this in another bug - can you file a bug for that please?

Comment 32

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #31)
> (In reply to Tim Nguyen [:ntim] from comment #28)
> > > ::: devtools/client/themes/webconsole.css:530
> > > (Diff revision 7)
> > > > -  background-position: -32px 0;
> > > > +  filter: url(images/filters.svg#checked-icon-state);
> > > 
> > > This pattern of using:
> > > 
> > > `filter: url(images/filters.svg#checked-icon-state)`
> > > 
> > > for hover and:
> > > 
> > > `filter: url(images/filters.svg#checked-icon-state) brightness(0.9);`
> > > 
> > > for active could be simplified by introducing a state in the filter file for
> > > hover directly: images/filters.svg#hover-icon-state.
> > I guess you meant #checked-active-icon-state here. 
> > 
> > Anyway, I'm too happy with doing this because these styles are really
> > specific to the variables-view icons. I also don't think it simplifies
> > things a lot, the filter value would be a tad bit shorter (9 less chars),
> > but we'd have to introduce a new filter.
> > 
> > I'd ideally want to clean up these styles later as well, and possibly
> > re-investigate those styles to be more consistent with other icon buttons.
> > What do you think ?
> 
> Hm I didn't realize this was just for the vview icons.. Ideally
> .ruleview-selectorhighlighter, .animation-target .node-highlighter,
> .elementNode, and .open-inspector would instead be using a shared class for
> styling.  That alone would make things much better.  Probably scope creep
> here, so I'm fine with doing this in another bug - can you file a bug for
> that please?

Filed bug 1223845.

(In reply to Brian Grinstead [:bgrins] from comment #30)
> (In reply to Tim Nguyen [:ntim] from comment #29)
> > (In reply to Brian Grinstead [:bgrins] from comment #25)
> > > Created attachment 8685551 [details]
> > > different-blue-pickers.png
> > > 
> > > I'm fine with the 'new blue', although maybe the main element picker's
> > > active state should also be updated to match (see the different between the
> > > state in animation inspector and the devtools tab strip in this screenshot).
> > 
> > The issue here is that we're using the icon with the lowest opacity in the
> > icon sprite which causes the blue to have lower opacity. Bug 1223701 will
> > fix the issue, but if you like, I can hardcode the background-position so we
> > use the icon with the most opacity. What do you prefer ?
> 
> Do you think it's likely Bug 1223701 will ship in the same release as this
> one?  I'm fine with doing them separately if so but don't want to ship it in
> DE with the different colors.  I know it's subtle but it also sounds like
> it's an easy fix
I have some patches in my queue for bug 1223701, I'm just waiting for this bug to land before finishing those patches.

Comment 33

[Tim Nguyen :ntim]

Comment on attachment 8684691 [details]
MozReview Request: Bug 1173397 - Remove blue variants of devtools icons. r=bgrins

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

Comment 34

[Tim Nguyen :ntim]

Comment on attachment 8684692 [details]
MozReview Request: Bug 1173397 - Refactor devtools toolbar button code. r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24625/diff/7-8/

Comment 35

[Brian Grinstead [:bgrins]]

Comment on attachment 8684692 [details]
MozReview Request: Bug 1173397 - Refactor devtools toolbar button code. r=bgrins

https://reviewboard.mozilla.org/r/24625/#review22487

We discussed this on channel earlier, the only thing I would want to block on here is that this makes the checked state look more disabled for the source buttons (in particular black box and prettify).  The recommendation was to remove the image-region that's being set on those 2 when checked, and then only get rid of the blue filter for the toggle BP button.  And to not bother updating the image at this point so we can decide to go back to the crossed out icon later if we want.

Comment 36

[Brian Grinstead [:bgrins]]

Comment on attachment 8684691 [details]
MozReview Request: Bug 1173397 - Remove blue variants of devtools icons. r=bgrins

Not sure why reviewboard re-flagged this

Comment 37

[Brian Grinstead [:bgrins]]

We discussed with Helen and are going to proceed with this as a technical refactor even though it also changes some visuals as in Comment 24, and then revisit UX treatment on all buttons together as follow up work, particularly focusing on the hover / active states of all buttons.

Comment 38

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #37)
> We discussed with Helen and are going to proceed with this as a technical
> refactor even though it also changes some visuals as in Comment 24, and then
> revisit UX treatment on all buttons together as follow up work, particularly
> focusing on the hover / active states of all buttons.

Filed Bug 1223924 for this work

Comment 39

[Tim Nguyen :ntim]

Comment on attachment 8684691 [details]
MozReview Request: Bug 1173397 - Remove blue variants of devtools icons. r=bgrins

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

Comment 40

[Tim Nguyen :ntim]

Comment on attachment 8684692 [details]
MozReview Request: Bug 1173397 - Refactor devtools toolbar button code. r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24625/diff/8-9/

Comment 41

[Tim Nguyen :ntim]

Comment on attachment 8684691 [details]
MozReview Request: Bug 1173397 - Remove blue variants of devtools icons. r=bgrins

Mozreview, mozreview...

Comment 42

[Brian Grinstead [:bgrins]]

Comment on attachment 8684692 [details]
MozReview Request: Bug 1173397 - Refactor devtools toolbar button code. r=bgrins

https://reviewboard.mozilla.org/r/24625/#review22503

Thanks!

Comment 43

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/00170e908aa8
https://hg.mozilla.org/integration/fx-team/rev/6aff4aa8f3de

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/00170e908aa8
https://hg.mozilla.org/mozilla-central/rev/6aff4aa8f3de