1028252 - Allow toolbox command buttons to be text-only

Status: RESOLVED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

Description

[Joe Walker [:jwalker]]

This allows you to put any command in the toolbox command button area.

Comment 1

[Joe Walker [:jwalker]]

Attachment: 0003-Bug-1028252-Allow-toolbox-command-buttons-to-be-text.patch

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 8443546 [details] [diff] [review]
0003-Bug-1028252-Allow-toolbox-command-buttons-to-be-text.patch

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

I love that this lets you add a command directly on the toolbarSpec, but I think the CSS could be handled more cleanly.

::: browser/devtools/shared/DeveloperToolbar.jsm
@@ +113,4 @@
>          }
> +        else {
> +          button.setAttribute("label", command.name);
> +          button.className = "devtools-toolbarbutton devtools-toolbarbutton-textonly";

I don't think this is the best way to handle it with CSS - I'd remove the devtools-toolbarbutton-textonly class and target any devtools-toolbarbutton in the #toolbox-buttons element (there aren't any others)

::: browser/themes/shared/devtools/toolbars.inc.css
@@ +825,4 @@
>    background-color: inherit;
>  }
>  
> +.devtools-toolbarbutton.devtools-toolbarbutton-textonly {

I think this would be preferable (doesn't add a new class / matches dark theme for console buttons / also gets rid of the min width so it doesn't take up as much space):


#toolbox-buttons .devtools-toolbarbutton {
  -moz-padding-start: 5px;
  -moz-padding-end: 5px;
  min-width: inherit;
}

.theme-dark #toolbox-buttons .devtools-toolbarbutton[checked=true] {
  background: rgba(29, 79, 115, .7); /* Select highlight blue */
  color: #f5f7fa;
}
.theme-light #toolbox-buttons .devtools-toolbarbutton[checked=true] {
  background: rgba(76, 158, 217, .2); /* Select highlight blue */
}

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8443546 [details] [diff] [review]
0003-Bug-1028252-Allow-toolbox-command-buttons-to-be-text.patch

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

I love that this lets you add a command directly on the toolbarSpec, but I think the CSS could be handled more cleanly.

::: browser/devtools/shared/DeveloperToolbar.jsm
@@ +113,4 @@
>          }
> +        else {
> +          button.setAttribute("label", command.name);
> +          button.className = "devtools-toolbarbutton devtools-toolbarbutton-textonly";

I don't think this is the best way to handle it with CSS - I'd remove the devtools-toolbarbutton-textonly class and target any devtools-toolbarbutton in the #toolbox-buttons element (there aren't any others)

::: browser/themes/shared/devtools/toolbars.inc.css
@@ +825,4 @@
>    background-color: inherit;
>  }
>  
> +.devtools-toolbarbutton.devtools-toolbarbutton-textonly {

Of course, I gave you this suggestion, but now I'm thinking that you won't need the two [checked=true] rules (since these are defined earlier in the file).  Try running it without those two and see if it works

Comment 4

[Joe Walker [:jwalker]]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> ::: browser/themes/shared/devtools/toolbars.inc.css
> @@ +825,4 @@
> >    background-color: inherit;
> >  }
> >  
> > +.devtools-toolbarbutton.devtools-toolbarbutton-textonly {
> 
> Of course, I gave you this suggestion, but now I'm thinking that you won't
> need the two [checked=true] rules (since these are defined earlier in the
> file).  Try running it without those two and see if it works

The are needed because these rules get in the way:

.theme-light .devtools-toolbarbutton[label]:not([text-as-image]):not([type=menu-button]):hover:active

But if we add text-as-image=true to our toolbar buttons, then all is good.
I also added [text-as-image] to the padding adding rule because that seemed more correct than just saying that these happened to be the only ones that matched. Patch next.

Comment 5

[Joe Walker [:jwalker]]

Attachment: 0003-Bug-1028252-Allow-toolbox-command-buttons-to-be-text.patch

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8444377 [details] [diff] [review]
0003-Bug-1028252-Allow-toolbox-command-buttons-to-be-text.patch

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

Your changes look good. There is one thing that needs to change in toolbars.inc.css for dark theme support - there is a color: inherit on line 112 - it shouldn't be there.  That is causing dark text on the button in dark theme.

::: browser/themes/shared/devtools/toolbars.inc.css
@@ +827,5 @@
>  
> +#toolbox-buttons .devtools-toolbarbutton[text-as-image] {
> +  -moz-padding-start: 5px;
> +  -moz-padding-end: 5px;
> +  min-width: inherit;

If we want these to match the tab bar background, we should also set the color for light/dark theme to Tab Toolbar color at https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors

Comment 7

[Joe Walker [:jwalker]]

(In reply to Brian Grinstead [:bgrins] from comment #6)
> Comment on attachment 8444377 [details] [diff] [review]
> 0003-Bug-1028252-Allow-toolbox-command-buttons-to-be-text.patch
> 
> Review of attachment 8444377 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Your changes look good. There is one thing that needs to change in
> toolbars.inc.css for dark theme support - there is a color: inherit on line
> 112 - it shouldn't be there.  That is causing dark text on the button in
> dark theme.

Removed.
Also I moved the "#toolbox-buttons .devtools-toolbarbutton[text-as-image]" rule up in the file under the "/* Text-only buttons */" comment.

> ::: browser/themes/shared/devtools/toolbars.inc.css
> @@ +827,5 @@
> >  
> > +#toolbox-buttons .devtools-toolbarbutton[text-as-image] {
> > +  -moz-padding-start: 5px;
> > +  -moz-padding-end: 5px;
> > +  min-width: inherit;
> 
> If we want these to match the tab bar background, we should also set the
> color for light/dark theme to Tab Toolbar color at
> https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors

I don't think we do, or at least I intentionally didn't do it that way. It needed a hint that it was clickable not just some random text, and if the text button background was the same as the tab bar then buttons would run into each other.

Comment 8

[Brian Grinstead [:bgrins]]

> > If we want these to match the tab bar background, we should also set the
> > color for light/dark theme to Tab Toolbar color at
> > https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors
> 
> I don't think we do, or at least I intentionally didn't do it that way. It
> needed a hint that it was clickable not just some random text, and if the
> text button background was the same as the tab bar then buttons would run
> into each other.

OK, sounds good - that's the reasoning behind using different colors for text buttons in the toolbox as well.  I was wondering because the frame selection dropdown in Bug 977043 will use the same styling as these.  We can always revisit the colors later on.

Comment 9

[Joe Walker [:jwalker]]

So this looks like it works

/* Text-only buttons */
.theme-light .devtools-toolbarbutton[label]:not([text-as-image]):not([type=menu-button]),
.theme-light .devtools-toolbarbutton[text-as-image] {
  background-color: rgba(170, 170, 170, .2); /* Splitter */
}
.theme-dark .devtools-toolbarbutton[label]:not([text-as-image]):not([type=menu-button]),
.theme-dark .devtools-toolbarbutton[text-as-image] {
  background-color: rgba(0, 0, 0, .2); /* Splitter */
}

The specificity of the added selectors is low enough that the :hover{blue} still works.
Sound OK to you?

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Joe Walker [:jwalker] from comment #9)
> So this looks like it works
> 
> /* Text-only buttons */
> .theme-light
> .devtools-toolbarbutton[label]:not([text-as-image]):not([type=menu-button]),
> .theme-light .devtools-toolbarbutton[text-as-image] {
>   background-color: rgba(170, 170, 170, .2); /* Splitter */
> }
> .theme-dark
> .devtools-toolbarbutton[label]:not([text-as-image]):not([type=menu-button]),
> .theme-dark .devtools-toolbarbutton[text-as-image] {
>   background-color: rgba(0, 0, 0, .2); /* Splitter */
> }
> 
> The specificity of the added selectors is low enough that the :hover{blue}
> still works.
> Sound OK to you?

We don't want a background color on all .devtools-toolbarbutton[text-as-image] (see the debugger '{}' button for pretty printing).  We really only want to set it to text-as-image in this special case when it is a child of #toolbox-buttons (we could probably turn that into a class if that would help).  So we could either copy this selector including #toolbox-buttons plus :hover/active for the next two rules, or we could do something like this (which is a kind of silly selector):

.theme-dark #toolbox-buttons .devtools-toolbarbutton[text-as-image]:not(:hover):not(:active)

Comment 11

[Joe Walker [:jwalker]]

Attachment: 0003-Bug-1028252-Allow-toolbox-command-buttons-to-be-text.patch

I think this touches everything correctly...

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8445411 [details] [diff] [review]
0003-Bug-1028252-Allow-toolbox-command-buttons-to-be-text.patch

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

Almost - one more state to cover

::: browser/themes/shared/devtools/toolbars.inc.css
@@ +142,4 @@
>    background-color: rgba(0, 0, 0, .2); /* Splitter */
>  }
>  
> +#toolbox-buttons .devtools-toolbarbutton[text-as-image] {

Please move this rule above in the file, by the rule where min-width is set for:

.devtools-toolbarbutton:not([label]),
.devtools-toolbarbutton[text-as-image] {}

@@ +154,5 @@
>  .theme-dark .devtools-toolbarbutton[label]:not([text-as-image]):not([type=menu-button]):hover {
>    background: rgba(0, 0, 0, .3); /* Splitters */
>  }
>  .theme-light .devtools-toolbarbutton:hover,
> +.theme-light #toolbox-buttons .devtools-toolbarbutton[text-as-image]:hover,

There is one more state below for :hover:active

Comment 13

[Joe Walker [:jwalker]]

Attachment: 0003-Bug-1028252-Allow-toolbox-command-buttons-to-be-text.patch

Comment 14

[Joe Walker [:jwalker]]

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

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/9af5fd72176c