1255116 - Simplify text-only button styling

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect, P3)

Description

[Tim Nguyen :ntim]

The netmonitor uses some much simpler CSS for its buttons, let's use that.

Comment 1

[Tim Nguyen :ntim]

Attachment: Patch

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 8735175 [details] [diff] [review]
Patch

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

Overall I'm happy with moving the rules out of the individual tool's style sheets and into the shared one.  I think this needs a UX review.  Clearing review flag for now but left some notes inline

::: devtools/client/themes/toolbars.css
@@ +62,5 @@
>    background: transparent;
>    min-height: 18px;
>    text-shadow: none;
>    border: none;
> +  border-radius: 2px;

Requesting UX review from Helen for this change - do we want all of our buttons to have slightly rounded corners?

@@ +114,5 @@
>  .devtools-button[standalone],
>  .devtools-button[data-standalone],
>  .devtools-toolbarbutton[standalone],
>  .devtools-toolbarbutton[data-standalone] {
> +  border: 1px solid rgba(120,120,120,0.2);

Should we introduce a new variable for this border instead of using this hardcoded neutral color?  Then we could customize it for dark and light theme

@@ -120,2 @@
>    min-height: 32px;
> -  background-color: var(--theme-toolbar-background);

This is also changing the background color for toolbar buttons.  Helen can you check with this patch applied and see what you think?

@@ +196,4 @@
>    opacity: 0.8;
>  }
>  
> +.devtools-button:not(:disabled):not([disabled]):-moz-any(:hover,[checked],[open]):empty::before,

This selector is pretty complex.. It could use at least a comment above it explaining what this is trying to do if it's not possible to simplify

@@ +206,5 @@
>  .devtools-toolbarbutton[disabled] {
>    opacity: 0.5 !important;
>  }
>  
> +.devtools-button:-moz-any([checked],[open]):empty::before,

I preferred the previous way of doing it instead of using -moz-any since it's more familiar to people and each selector is pretty simple

@@ +242,5 @@
> +.devtools-toolbarbutton:not([disabled]):-moz-any([label], [data-text-only])[checked=true],
> +.devtools-toolbarbutton:not([type=menu-button]):not([disabled]):-moz-any([label], [data-text-only])[open],
> +.devtools-button:not(:empty)[checked=true],
> +#toolbox-buttons .devtools-toolbarbutton[text-as-image][checked=true] {
> +  background: var(--theme-selection-background);

This is a really big visual change (see selected background for webconsole filter icons which are now same color as the toolbox tabs). Is this the intent?

Comment 3

[Brian Grinstead [:bgrins]]

Mozscreenshots try push with patch applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2730b1e2b8ef

Comment 4

[Brian Grinstead [:bgrins]]

Here's a link to the screenshot artifacts on that try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2730b1e2b8ef&exclusion_profile=false&filter-tier=1&filter-tier=2&filter-tier=3

Comment 5

[Brian Grinstead [:bgrins]]

In particular, here's the webconsole buttons I'm talking about:

http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/2f79c5dcbc76d376c57d4d311032c2979b03b2451265d76da449298d5bcd5a4b6b02b4ad21ba645ddcba4e5cbf5ff4669ca05c9a150a74f33607f156610dd24d

Comment 6

[Helen V. Holmes (:helenvholmes) (:✨)]

Comment on attachment 8735175 [details] [diff] [review]
Patch

(In reply to Brian Grinstead [:bgrins] from comment #2)
> ::: devtools/client/themes/toolbars.css
> @@ +62,5 @@
> >    background: transparent;
> >    min-height: 18px;
> >    text-shadow: none;
> >    border: none;
> > +  border-radius: 2px;
> 
> Requesting UX review from Helen for this change - do we want all of our
> buttons to have slightly rounded corners?
Yep, I'm fine with this.

I'm not a huge fan of making the buttons that blue from the semi-transparent, though. When we hide the buttons in a dropdown menu (in the future designs here: http://cl.ly/2c0X3v143r0B) I'll be fine with it but I think that bgrins is right that they look strange right underneath the toolbar tabs. Can we roll back that change?

Comment 8

[Tim Nguyen :ntim]

Attachment: Bug 1255116 - Simplify toolbar button styles.

Review commit: https://reviewboard.mozilla.org/r/111376/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/111376/

Comment 9

[Tim Nguyen :ntim]

Comment on attachment 8835755 [details]
Bug 1255116 - Simplify toolbar button styles.

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

Comment 10

[Tim Nguyen :ntim]

Comment on attachment 8835755 [details]
Bug 1255116 - Simplify toolbar button styles.

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

Comment 11

[Tim Nguyen :ntim]

Summary of changes:
    - Consolidated all HTML button classes (.devtools-button, .tool-button, .menu-filter-button) into one single .devtools-button class
    - Removed support for the button checked attribute (use class names instead)
    - Added :hover state to .devtools-button icon (approved by Helen in bug 1296271)
    - Done some small tweaks that Helen approved in comment 6
    - The old console button style has been preserved and has been moved into webconsole.css

Comment 12

[Tim Nguyen :ntim]

Comment on attachment 8835755 [details]
Bug 1255116 - Simplify toolbar button styles.

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

Comment 13

[Tim Nguyen :ntim]

Comment on attachment 8835755 [details]
Bug 1255116 - Simplify toolbar button styles.

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

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8835755 [details]
Bug 1255116 - Simplify toolbar button styles.

https://reviewboard.mozilla.org/r/111376/#review113416

I noticed that with the dark theme:

* Command buttons use a different background color on hover than tabs for panels do
* Settings cog has the hover background color that's used for panels (which I guess makes sense because it will show a panel...) which is different from its neighboring command buttons

With both themes:

* The settings cog icon has less contrast compared to neighboring command buttons

These seem like small issues.  If they are not expected, please file follow ups for them if you don't want to address them here.

Comment 15

[Tim Nguyen :ntim]

Comment on attachment 8835755 [details]
Bug 1255116 - Simplify toolbar button styles.

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6eb03c463039
Simplify toolbar button styles. r=jryans

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6eb03c463039

Comment 19

[Matthew N. [:MattN]]

Automation screenshots of the changes for review: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=0a7831d838f793a263456df62f95a615472a7f95&newProject=mozilla-central&newRev=5e691d87bed93f740094604be686567b34a6f29d