Closed Bug 1334126 Opened 5 years ago Closed 5 years ago

Screenshot icon beside Screenshot Behavior label looks inconsistent

Categories

(DevTools :: Framework, defect, P3)

54 Branch
defect

Tracking

(firefox54 verified)

VERIFIED FIXED
Firefox 54
Tracking Status
firefox54 --- verified

People

(Reporter: Towkir, Assigned: jbhoosreddy)

References

Details

Attachments

(3 files, 1 obsolete file)

The icon should not be there. Even if it should, the hover effect (increasing opacity) should be removed.
Tim, what do you think ?
Flags: needinfo?(ntim.bugs)
(In reply to [:Towkir] Ahmed from comment #1)
> Tim, what do you think ?

I agree. Needinfo'ing some people to get their opinions.
Flags: needinfo?(ntim.bugs)
Flags: needinfo?(jryans)
Flags: needinfo?(jaideepb)
Hmm, I think it's fine to have the icon there...  It helps build a mental connection between the various buttons and these settings.  (I would be fine with appropriate icons for other tools appearing next to their option sections, if someone wants to add them.)

It does seem like a bug that the icon has a hover state, so that seems worth removing.
Flags: needinfo?(jryans)
Component: Developer Tools → Developer Tools: Framework
Priority: -- → P3
Apologies for the late response. I screenshot icon was put there on purpose by Helen's request. I can't seem to recall the exact reasons for it. But I don't think putting a hover state was ever part of the plan, that might have crept in by mistake.

It took some figuring out but I set up my local repo again. Hopefully, I can submit a patch soon.
Flags: needinfo?(jaideepb)
Assignee: nobody → jaideepb
Attached patch 1334126.patch (obsolete) — Splinter Review
This seems to only visibly affect in dark mode, which has a pronounced hover state on buttons.

I noticed common.css already has useful css for handling this situation. Therefore, I replaced my original implementation using a <span> tag with a <button> disabled. And adjusted the CSS accordingly.
Attachment #8831801 - Flags: review?(jryans)
Attached image 1334126.gif
Here is the modified UI.
Attachment #8831804 - Flags: ui-review?(jryans)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> Hmm, I think it's fine to have the icon there...  It helps build a mental
> connection between the various buttons and these settings.  (I would be fine
> with appropriate icons for other tools appearing next to their option
> sections, if someone wants to add them.)
That is why I wrote the summary about inconsistency, icons are good. but that is the only one in the settings tab. It would be nice to add icon beside all the labels(if there is such a plan), otherwise remove the only one
Comment on attachment 8831801 [details] [diff] [review]
1334126.patch

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

::: devtools/client/framework/options-panel.css
@@ +108,5 @@
>  
>  #screenshot-icon::before {
>    background-image: url(chrome://devtools/skin/images/command-screenshot.svg);
> +  margin-left: -15px;
> +  margin-top: -14px;

We should avoid arbitrary margin values.

It seems like setting vertical-align: middle; min-width: 0; (or top) on #screenshot-icon (not ::before), fixes the issue.
Attachment #8831801 - Flags: feedback-
Based on your suggestion, I modified my patch, but left a 5px margin to give some spacing between the button and label.
Attachment #8831801 - Attachment is obsolete: true
Attachment #8831801 - Flags: review?(jryans)
Attachment #8831826 - Flags: review?(ntim.bugs)
Attachment #8831826 - Flags: review?(ntim.bugs) → review+
@ntim, since there is no code change or tests for this, I'll add checkin-needed for this. Let me know if I should do something else instead.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da8fb81b4038
Fix "Screenshot icon beside Screenshot Behavior label looks inconsistent". r=ntim.bugs
Keywords: checkin-needed
Comment on attachment 8831804 [details]
1334126.gif

Looks like it already landed, but anyway, looks reasonable from the picture.
Attachment #8831804 - Flags: ui-review?(jryans) → ui-review+
https://hg.mozilla.org/mozilla-central/rev/da8fb81b4038
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
I've seen this issue on nightly 54.0a1 (2017-01-26) in windows 10 (32 bit).

Hover effect is no more.This Bug is now verified as fixed on Latest Firefox nightly 54.0a1

Build id :  20170203030204

User agent : Mozilla/5.0 (Windows NT 10.0; rv:54.0) Gecko/20100101 Firefox/54.0
Status: RESOLVED → VERIFIED
See Also: → 1374088
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.