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

Fixed filter button in perf tool not being inverted

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.

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

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

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

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

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

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

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

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.

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

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

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/.

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

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

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

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

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

(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

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

(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 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 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 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 on attachment 8684691 [details] MozReview Request: Bug 1173397 - Remove blue variants of devtools icons. r=bgrins Not sure why reviewboard re-flagged this

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.

(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 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 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 on attachment 8684691 [details] MozReview Request: Bug 1173397 - Remove blue variants of devtools icons. r=bgrins Mozreview, mozreview...

Comment on attachment 8684692 [details] MozReview Request: Bug 1173397 - Refactor devtools toolbar button code. r=bgrins https://reviewboard.mozilla.org/r/24625/#review22503 Thanks!

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

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