965985 - DevTools light theme button followup

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Brian Grinstead [:bgrins]]

I've noticed that the active buttons have the normal hover effect when hovered, which makes it confusing when you click a toggle-able button like on the console.  Just need to do a little cleanup before we move on to Bug 952100.

Comment 2

[Brian Grinstead [:bgrins]]

Another issue (from Bug 966200):

1. Enable devtools.debugger.tracer
2. Open debugger

The stepping buttons have a different border color vs. the tracer button.
I think all the buttons should have the same border.

The [O|Import] button in the profiler has a darker border than the rest of the buttons.

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: theme-button-follow-up.patch

The issue with groups having different borders has been handled in debugger.inc.css and profiler.inc.css.  We should really have a 'devtools-button-group' class for this type of pattern so it can be reused, but this resolves the issue in its current state.

Also fixes the [checked]:hover issue on light theme buttons by applying certain rules specifically to the dark theme in toolbars.inc.css and moves some of the shared rules above the specific theme overrides.

Comment 4

[Victor Porof [:vporof][:vp]]

Comment on attachment 8368587 [details] [diff] [review]
theme-button-follow-up.patch

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

Is there a followup for creating a shared devtools-button-group class? If so, mind adding the bug in comments in the css files? If not, would it be super hard to fix it now? (I don't feel strongly either way)

::: browser/themes/shared/devtools/toolbars.inc.css
@@ +106,4 @@
>    background-color: transparent !important;
>  }
>  
> +.theme-dark .devtools-toolbarbutton[checked=true]:hover:active {

Do we need .theme-light equivalents for these colors? If so (and they're in the file, care to move them here?

Comment 5

[Brian Grinstead [:bgrins]]

Attachment: theme-button-follow-up.patch

(In reply to Victor Porof [:vp] from comment #4)
> Comment on attachment 8368587 [details] [diff] [review]
> theme-button-follow-up.patch
> 
> Review of attachment 8368587 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is there a followup for creating a shared devtools-button-group class? If
> so, mind adding the bug in comments in the css files? If not, would it be
> super hard to fix it now? (I don't feel strongly either way)
> 

I've updated the patch to go ahead and make the helper class and remove specific code for each group.  I've asked for review again just to cover these changes.

> ::: browser/themes/shared/devtools/toolbars.inc.css
> @@ +106,4 @@
> >    background-color: transparent !important;
> >  }
> >  
> > +.theme-dark .devtools-toolbarbutton[checked=true]:hover:active {
> 
> Do we need .theme-light equivalents for these colors? If so (and they're in
> the file, care to move them here?

In this case, the color is defined alongside a bunch of others: 
.theme-light .devtools-menulist[open=true],
.theme-light .devtools-toolbarbutton[open=true],
.theme-light .devtools-toolbarbutton[open=true]:hover,
.theme-light .devtools-toolbarbutton[open=true]:hover:active,
.theme-light .devtools-toolbarbutton[checked=true],
.theme-light .devtools-toolbarbutton[checked=true]:hover,
.theme-light .devtools-toolbarbutton[checked=true]:hover:active

The button UI (for both themes) is going to be changed in Bug 952100, so some of this will probably change soon.

Comment 6

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/ce2e31b34b1e
https://tbpl.mozilla.org/?tree=Fx-Team&rev=ce2e31b34b1e

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ce2e31b34b1e