Closed
Bug 1296271
Opened 8 years ago
Closed 7 years ago
All devtools buttons should have a background-color on hover like command-button so they appear more like buttons
Categories
(DevTools :: Framework, enhancement, P3)
Tracking
(Not tracked)
RESOLVED
DUPLICATE
of bug 1255116
People
(Reporter: nachtigall, Assigned: djmdeveloper060796)
References
Details
Attachments
(4 files, 4 obsolete files)
I noticed that the eyedropper button on the color choser looks more like a label than a button. Then I saw that many more buttons miss some kind of visualization that would make them appear more button-like. There's already this which does add a nice background on hover, but only for .command-button elements: .command-button:hover { background-color: var(--toolbar-tab-hover); } This is marked "green" in the attached screenshot. I would propose to add something like this so that other elements also get a darker background when hovered, so they look/appear more like a button - also for UX consistency: .devtools-button:hover, .devtools-toolbarbutton:hover #toolbox-dock-buttons > button:hover { background-color: var(--toolbar-tab-hover); } This would work for most of the "red" buttons in the screenshot. I could not find out what class the eyedropper in the color choser is, but probably something similar.
Comment 2•8 years ago
|
||
I agree we should definitely have this! It would be considered an accessibility update too, which is nice. I think it would be safe to add the styling from .command-button:hover to the red instances Jens has illustrated so that styling changes (if there are any in the future) can all be done at once.
Flags: needinfo?(hholmes)
Severity: normal → enhancement
Priority: -- → P3
Comment 4•8 years ago
|
||
If this is not too complicated and someone is eager to mentor, I hope I can take this :) Thanks
Comment 5•8 years ago
|
||
(In reply to [:Towkir] Ahmed from comment #4) > If this is not too complicated and someone is eager to mentor, I hope I can > take this :) > Thanks The code is here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/common.css#363 With a bit of CSS knowledge, you should be able to figure it out.
For sake of completeness, the box model's "Repositioner" also lacks background-color on hover. So easy to miss as button. See screenshot. (PS Was very nice meeting some of you at ViewSource Conf)
Assignee | ||
Comment 7•8 years ago
|
||
Thsi is a test patch. I have added background-color to devtools-button hover state.Now the buttons marked red by jens has a background shade attached to their hover state.
Attachment #8800542 -
Flags: review?(dtownsend)
Updated•8 years ago
|
Attachment #8800542 -
Flags: review?(dtownsend) → review?(ntim.bugs)
Comment 8•8 years ago
|
||
Comment on attachment 8800542 [details] [diff] [review] bug1296271.patch Review of attachment 8800542 [details] [diff] [review]: ----------------------------------------------------------------- This looks really odd with the performance tool buttons, other than that I think we want the hover state to "expand" to the whole button, and not just around the icon.
Attachment #8800542 -
Flags: review?(ntim.bugs)
Assignee | ||
Comment 9•8 years ago
|
||
I have updated my previous patch and expanded the hover state to the entire button.Hope this helps.
Attachment #8800542 -
Attachment is obsolete: true
Attachment #8801578 -
Flags: review?(ntim.bugs)
Comment 10•8 years ago
|
||
Comment on attachment 8801578 [details] [diff] [review] bug1296271.patch Review of attachment 8801578 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/themes/common.css @@ +376,5 @@ > opacity: 1; > } > > +.devtools-button:hover:not(:disabled), > +.devtools-toolbarbutton:not([disabled=true]):hover { This rule should only apply on icon buttons, so: .devtools-button:not(:empty):not(:disabled):hover, .devtools-toolbarbutton:not([label]):not([standalone]):not([disabled=true]):hover {
Attachment #8801578 -
Flags: review?(ntim.bugs)
Comment 11•8 years ago
|
||
(In reply to Tim Nguyen :ntim (use needinfo?) from comment #10) > This rule should only apply on icon buttons, so: > > .devtools-button:not(:empty):not(:disabled):hover, > .devtools-toolbarbutton:not([label]):not([standalone]):not([disabled=true]): > hover { Sorry, I meant .devtools-button:empty:not(:disabled):hover, .devtools-toolbarbutton:not([label]):not([standalone]):not([disabled=true]):hover {
Comment 12•8 years ago
|
||
As Mondal has submitted several patches and working on this actively, assigning it on.
Assignee: nobody → djmdeveloper060796
Status: NEW → ASSIGNED
Assignee | ||
Comment 13•8 years ago
|
||
Updated my previous patch as pointed in Comment 11 .
Attachment #8801578 -
Attachment is obsolete: true
Attachment #8802875 -
Flags: review?(ntim.bugs)
Updated•8 years ago
|
Attachment #8802875 -
Flags: ui-review?(hholmes)
Updated•8 years ago
|
Attachment #8802875 -
Flags: review?(ntim.bugs) → review?(bgrinstead)
Comment 14•7 years ago
|
||
A few issues I see with the patch applied: 1) the hover state on the docking controls is too narrow (see screenshot). The width and/or padding on the `#toolbox-controls > button, #toolbox-dock-buttons > button` selector might need to be updated so that they fill up all of the space they are taking 2) There is an inconsistent hover state on the options panel icon (it becomes more opaque when hovering instead of choosing a background color) 3) The 'pick element' icon hover doesn't fill up all of the vertical space
Comment 15•7 years ago
|
||
Comment on attachment 8802875 [details] [diff] [review] bug1296271.patch Review of attachment 8802875 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay, clearing review as per comment 14
Attachment #8802875 -
Flags: ui-review?(hholmes)
Attachment #8802875 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 16•7 years ago
|
||
Added background to the hover state of the options panel icon, updated the padding of the docking controls,the pick element icon now takes up the vertical space
Attachment #8802875 -
Attachment is obsolete: true
Attachment #8833056 -
Flags: review?(jryans)
Comment 17•7 years ago
|
||
Comment on attachment 8833056 [details] [diff] [review] bug1296271.patch Review of attachment 8833056 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/themes/common.css @@ +376,5 @@ > opacity: 1; > } > > +#toolbox-controls > button, > +#toolbox-dock-buttons > button { Please apply common classes (.command-button) rather than applying extra CSS. https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/components/toolbox-toolbar.js#219 toolbox-dock-button -> command-button Also, if there is a need for these rules, I'd rather change them here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/toolbox.css#262 @@ +389,5 @@ > + > +#toolbox-picker-container > button { > + margin-top: -1px; > + margin-bottom: -1px; > +} This block is not needed. @@ +392,5 @@ > + margin-bottom: -1px; > +} > + > +.devtools-button:empty:not(:disabled):hover, > +.devtools-toolbarbutton:not([label]):not([standalone]):not([disabled=true]):hover { Makes sense for the checked state to also have the same background.
Attachment #8833056 -
Flags: review?(jryans) → review-
Assignee | ||
Comment 18•7 years ago
|
||
I moved the code related to dock buttons and toolbox-options-container to toolbox.css.
> #toolbox-picker-container > button {
> margin-top: -1px;
> margin-bottom: -1px;
> }
I have removed this block but on removing this the background of the picker option is not covering entire vertical space.
Attachment #8833056 -
Attachment is obsolete: true
Attachment #8836307 -
Flags: review?(jryans)
Comment on attachment 8836307 [details] [diff] [review] bug1296271.patch :ntim should continue reviewing here.
Attachment #8836307 -
Flags: review?(jryans) → review?(ntim.bugs)
Comment 20•7 years ago
|
||
I fixed this as part of bug 1255116.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Updated•7 years ago
|
Attachment #8836307 -
Flags: review?(ntim.bugs)
Comment 21•7 years ago
|
||
Let me know if you'd like me to suggest other bugs to work on.
Assignee | ||
Comment 22•7 years ago
|
||
@Tim Yeah sure, I am interested in working on other bugs
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(ntim.bugs)
Comment 23•7 years ago
|
||
Here are a few bug suggestions: JS: Bug 1323454, Bug 1312687, bug 1327121, bug 1108288 CSS: bug 1242223, bug 1339558
Flags: needinfo?(ntim.bugs)
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•