1334126 - Screenshot icon beside Screenshot Behavior label looks inconsistent

Status: VERIFIED FIXED

(DevTools :: Framework, defect, P3)

Description

[[:Towkir] Ahmed]

Attachment: inconsistent-screenshot-icon.gif

The icon should not be there. Even if it should, the hover effect (increasing opacity) should be removed.

Comment 1

[[:Towkir] Ahmed]

Tim, what do you think ?

Comment 2

[Tim Nguyen :ntim]

(In reply to [:Towkir] Ahmed from comment #1)
> Tim, what do you think ?

I agree. Needinfo'ing some people to get their opinions.

Comment 3

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

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.

Comment 4

[Jaideep Bhoosreddy [:jbhoosreddy]]

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.

Comment 5

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1334126.patch

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.

Comment 6

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1334126.gif

Here is the modified UI.

Comment 7

[[:Towkir] Ahmed]

(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 8

[Tim Nguyen :ntim]

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.

Comment 9

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1334126.patch [2.0]

Based on your suggestion, I modified my patch, but left a 5px margin to give some spacing between the button and label.

Comment 10

[Jaideep Bhoosreddy [:jbhoosreddy]]

@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.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da8fb81b4038
Fix "Screenshot icon beside Screenshot Behavior label looks inconsistent". r=ntim.bugs

Comment 12

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

Comment on attachment 8831804 [details]
1334126.gif

Looks like it already landed, but anyway, looks reasonable from the picture.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/da8fb81b4038

Comment 14

[Nawaf Chowdhury Akash]

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