Closed Bug 1259475 Opened 8 years ago Closed 8 years ago

Remove hover state from disabled icon buttons

Categories

(DevTools :: Framework, defect, P2)

Product:
DevTools
Component:
Framework
Type:
defect

Tracking

(firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: ntim, Assigned: djmdeveloper060796)

References

Details

(Keywords: good-first-bug, Whiteboard: [btpp-fix-later][lang=css])

Attachments

(1 file, 3 obsolete files)

bug1259475.patch
709 bytes, patch
ntim
: review+
Details | Diff | Splinter Review 
Example: the diff snapshots button in the memory tool.
Priority: -- → P2
Whiteboard: [btpp-fix-later]
Keywords: good-first-bug
Whiteboard: [btpp-fix-later] → [btpp-fix-later][lang=css]
Hi Tim,
Can you please provide a screenshot, so that I can start with this ? 
Thanks
Flags: needinfo?(ntim.bugs)
(In reply to [:Towkir] Ahmed from comment #1)
> Hi Tim,
> Can you please provide a screenshot, so that I can start with this ? 
> Thanks

In the memory tool, the third button (the one that looks like [0]) is disabled, but when you hover it, you can see the opacity of the button change (it's subtle, but visible).


[0]: https://hg.mozilla.org/mozilla-central/raw-file/938ce16be25f9c551c19ef8938e8717ed3d41ff5/devtools/client/themes/images/diff.svg
Flags: needinfo?(ntim.bugs)
See Also: → 1301815
Thanks Tim, I will submit a patch soon, meanwhile you can have a look at the other one, may be mentor too if possible :)
See Also: → 1301819
See Also: 1301819 → 
Hi Tim, does it look good on today's nightly ? 
I found this okay on linux.
Flags: needinfo?(ntim.bugs)
Still happening on macOS latest Nightly for me.
Flags: needinfo?(ntim.bugs)
Attached patch bug1259475.patch (obsolete) — — Splinter Review 
This is a test patch. I have added css rules to check whether the selected button is disabled or not. The diff-snapshot button does not change its opacity anymore on hovering mouse pointer over it when its disabled. hope this helps.
Attachment #8797570 - Flags: review?(jwalker)
Comment on attachment 8797570 [details] [diff] [review]
bug1259475.patch

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

::: devtools/client/themes/common.css
@@ +366,4 @@
>    opacity: 0.8;
>  }
>  
> +.devtools-button:hover:empty:not([disabled]):before,

:not(:disabled) is more standard. Can you use that?
Attachment #8797570 - Flags: review?(jwalker) → feedback+
Attached patch bug1259475.patch (obsolete) — — Splinter Review 
I have updated my previous patch and replaced :not([disabled]) with :not(:disabled).
Attachment #8797611 - Flags: review?(ntim.bugs)
Attachment #8797570 - Attachment is obsolete: true
Comment on attachment 8797611 [details] [diff] [review]
bug1259475.patch

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

::: devtools/client/themes/common.css
@@ +370,3 @@
>  .devtools-button[checked]:empty::before,
>  .devtools-button[open]:empty::before,
> +.devtools-toolbarbutton:not([label]):not(:disabled):hover > image,

So this (.devtools-toolbarbutton) should use :not([disabled=true]) while .devtools-button should use :not(:disabled)

Sorry if I wasn't clear enough.
Attachment #8797611 - Flags: review?(ntim.bugs) → feedback+
Attached patch bug1259475.patch (obsolete) — — Splinter Review 
Updated my previous patch.
Attachment #8798129 - Flags: review?(ntim.bugs)
Attachment #8797611 - Attachment is obsolete: true
Comment on attachment 8798129 [details] [diff] [review]
bug1259475.patch

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

Thanks!
Attachment #8798129 - Flags: review?(ntim.bugs) → review+
Assignee: nobody → djmdeveloper060796
Keywords: checkin-needed
has problems to apply:

patching file devtools/client/themes/common.css
Hunk #1 FAILED at 365
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/themes/common.css.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug1259475.patch
Flags: needinfo?(djmdeveloper060796)
Keywords: checkin-needed
Attached patch bug1259475.patch — — Splinter Review 
I have updated my local repository. Hope this helps.
Attachment #8798129 - Attachment is obsolete: true
Flags: needinfo?(djmdeveloper060796)
Attachment #8798353 - Flags: review?(ntim.bugs)
Comment on attachment 8798353 [details] [diff] [review]
bug1259475.patch

You don't need to re-request review once you've been granted an r+
Attachment #8798353 - Flags: review?(ntim.bugs) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/97d32293dec4
Remove hover state from disabled icon buttons. r=ntim
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/97d32293dec4
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
I have reproduced this bug with Nightly 48.0a1 (2016-03-24) on Windows 7, 64 Bit !

This bug's fix is verified with latest Nightly

Build ID    20161109030210
User Agent  Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0

 [bugday-20161109]
cool :)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: