Closed Bug 1255116 Opened 6 years ago Closed 5 years ago

Simplify text-only button styling

Categories

(DevTools :: CSS and Themes, defect, P3)

defect

Tracking

(firefox54 fixed)

RESOLVED FIXED
Firefox 54
Tracking Status
firefox54 --- fixed

People

(Reporter: ntim, Assigned: ntim)

References

(Blocks 1 open bug)

Details

(Whiteboard: [btpp-backlog])

Attachments

(1 file, 1 obsolete file)

The netmonitor uses some much simpler CSS for its buttons, let's use that.
Priority: -- → P3
Whiteboard: [btpp-backlog]
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
Attachment #8735175 - Flags: review?(bgrinstead)
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?
Attachment #8735175 - Flags: review?(bgrinstead) → ui-review?(hholmes)
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?
Attachment #8735175 - Flags: ui-review?(hholmes) → feedback+
Duplicate of this bug: 1338295
Attachment #8735175 - Attachment is obsolete: true
Depends on: 1308449
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
Blocks: 1338377
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.
Attachment #8835755 - Flags: review?(jryans) → review+
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6eb03c463039
Simplify toolbar button styles. r=jryans
https://hg.mozilla.org/mozilla-central/rev/6eb03c463039
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Depends on: 1340539
Duplicate of this bug: 1296271
Blocks: 1341045
No longer blocks: 1341045
Depends on: 1341045
Depends on: 1412071
Product: Firefox → DevTools
Component: Framework → CSS and Themes
You need to log in before you can comment on or make changes to this bug.