1374088 - "Screenshot behavior" camera icon on Toolbox options is not correctly aligned

Status: VERIFIED FIXED

(DevTools :: General, defect, P5)

Description

[Md. Majedul islam]

Attachment: camera icon.png

OS:Ubuntu 16.04 ,64Bit!
Build ID   20170618100241
User Agent Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0

STR:

1.Open the Developer Tools
2.Go to Toolbox options  
3. look at "Screenshort behavior " camera icon

Actual result :
"Screenshort behavior " camera icon is not correctly aligned.

Expected result: 
"Screenshort behavior " camera icon should correctly aligned.

Comment 1

[[:Towkir] Ahmed]

There was a previous bug regarding this icon. bug 1334126
Rev: https://hg.mozilla.org/integration/mozilla-inbound/rev/da8fb81b4038#l2.12

I have some thoughts on this. Why are we using a (disabled) button when it is never going to be clicked. I mean, span was a good idea, we could style the span.

On another thought, do we really need this ?

Jryans, do you plan to put icons beside other options titles? 
https://bugzilla.mozilla.org/show_bug.cgi?id=1334126#c3
Otherwise, why keep this one ?

Tim, any suggestions on this ?

Comment 2

[Tim Nguyen :ntim]

(In reply to [:Towkir] Ahmed from comment #1)
> There was a previous bug regarding this icon. bug 1334126
> Rev:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/da8fb81b4038#l2.12
> 
> I have some thoughts on this. Why are we using a (disabled) button when it
> is never going to be clicked. I mean, span was a good idea, we could style
> the span.
You could use the ::after pseudo element and stop relying on the .devtools-button class.
...::after {
  content: "";
  display: inline-block;
  background-image: url(...);
  width: 16px;
  height: 16px;
}

> On another thought, do we really need this ?
I wouldn't remove it now until we have a new spec

> Jryans, do you plan to put icons beside other options titles? 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1334126#c3
> Otherwise, why keep this one ?
There were plans by Helen to redesign the options panel, but I'm not sure what's the status now.

Comment 3

[[:Towkir] Ahmed]

(In reply to Tim Nguyen :ntim from comment #2)
> You could use the ::after pseudo element and stop relying on the
> .devtools-button class.
> ...::after {
>   content: "";
> ...............
>   height: 16px;
> }

Use ::after on which node / id / class ?

there is a DOM structure like: 

<legend xmlns="http://www.w3.org/1999/xhtml">
    Screenshot Behavior
    <button id="screenshot-icon" disabled="true" class="devtools-button"></button>
</legend>

Comment 4

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

(In reply to [:Towkir] Ahmed from comment #1)
> I have some thoughts on this. Why are we using a (disabled) button when it
> is never going to be clicked. I mean, span was a good idea, we could style
> the span.
> 
> On another thought, do we really need this ?
> 
> Jryans, do you plan to put icons beside other options titles? 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1334126#c3
> Otherwise, why keep this one ?

I believe the idea was to help build a mental link between the icon and the meaning in the section header.  However, I haven't heard of plans to do with other section headers.

I agree with :ntim that it's probably fine to leave it in there for now until there is new design spec for options.

It would be great to fix the alignment.  Also, was it always the disabled gray color?  I can't remember now...

Comment 5

[[:Towkir] Ahmed]

Attachment: camera-icon.patch

Hey Tim,
That is the least changes done to achieve the alignment. Looks good on my local build, I am confused about the view on windows (could not check)

Previously I found it a bit more misaligned on windows, Feedback requested.
If it is good to go, you can review too.

Comment 6

[Tim Nguyen :ntim]

Comment on attachment 8879614 [details] [diff] [review]
camera-icon.patch

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

::: devtools/client/framework/options-panel.css
@@ +115,3 @@
>  }
>  
> +#screenshot-options legend::after {

Seems like the indentation on this block is off. It uses tab indentation when it should use 2 spaces.

@@ +115,4 @@
>  }
>  
> +#screenshot-options legend::after {
> +	content: '';

nit: use double quotes

@@ +116,5 @@
>  
> +#screenshot-options legend::after {
> +	content: '';
> +	display: inline-block;
> +	background-image: url('chrome://devtools/skin/images/command-screenshot.svg');

nit: use double quotes

@@ +121,5 @@
> +	width: 16px;
> +	height: 16px;
> +	position: absolute;
> +	top: 5px;
> +	left: calc(100% + 5px);

Can we not use absolute positioning ?

I would largely prefer using margin-inline-start: 5px; and vertical-align

::: devtools/client/framework/toolbox-options.xhtml
@@ +113,1 @@
>          </legend>

This fits in one line now:

<legend>&options.screenshot.label;</legend>

Comment 7

[[:Towkir] Ahmed]

Attachment: camera-icon-alignment.patch

Looks like "vertical-align: bottom" aligns it more appropriately than "vertical-align: middle"

Hope this helps.

Comment 8

[Patrick Brosset <:pbro>]

DevTools bug triage (filter on CLIMBING SHOES).

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8879824 [details] [diff] [review]
camera-icon-alignment.patch

Forwarding to Gabe, who I believe has been looking into the options UI lately

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8879824 [details] [diff] [review]
camera-icon-alignment.patch

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

::: devtools/client/framework/options-panel.css
@@ +115,5 @@
> +  display: inline-block;
> +  background-image: url("chrome://devtools/skin/images/command-screenshot.svg");
> +  width: 16px;
> +  height: 16px;
> +  vertical-align: bottom;

When I appear a 1px solid red border around the legend, "vertical-align: sub;" seems to look best in terms of centering. Otherwise, I would remove the vertical-align and do margin-bottom: -2px, but this seems equivalent to "sub". I think this is due to how the svg is laid out since I would typically go with "vertical-align: middle;". 

I think going with the "vertical-align: sub;" is your best bet. r+ with changes to make it more centered.

Comment 11

[[:Towkir] Ahmed]

should I submit the patch again with "vertical-align: sub;" ?
looks like you've r+ed, if I make the changes and submit again, I should r+ myself and add checkin-needed. right ?

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to [:Towkir] Ahmed from comment #11)
> should I submit the patch again with "vertical-align: sub;" ?
> looks like you've r+ed, if I make the changes and submit again, I should r+
> myself and add checkin-needed. right ?

I would re-upload the new patch with the changes, and carry over the r+ by r+'ing it yourself and add a checkin-needed. Once a patch is r+, it doesn't need a new review because the changes that are recommended are usually somewhat trivial and would not need another review. So, you can carryover any r+ at that point.

Thanks for the patch again Towkir!

Comment 13

[[:Towkir] Ahmed]

Attachment: camera-icon-alignment.patch

Here is the updated patch

Comment 14

[[:Towkir] Ahmed]

I hope it's ready to land now

Comment 15

[Carsten Book [:Tomcat]]

has conflicts when trying to land on inbound:

patching file devtools/client/framework/options-panel.css
Hunk #1 FAILED at 109
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/framework/options-panel.css.rej
patching file devtools/client/framework/toolbox-options.xhtml
Hunk #1 FAILED at 108
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/framework/toolbox-options.xhtml.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh camera-icon-alignment.patch

Comment 16

[Tim Nguyen :ntim]

Comment on attachment 8885588 [details] [diff] [review]
camera-icon-alignment.patch

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

Please change the commit message to include the bug number and r=gl instead of r=ntim

Comment 17

[[:Towkir] Ahmed]

Attachment: camera-icon-alignment.patch

I think I had some issues with my mercurial setup on earlier patch.
Here is the updated one.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b778f1f3c2d
camera icon on 'Screenshort behavior' label is now correctly aligned; r=gl

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9b778f1f3c2d

Comment 20

[Sajedul Islam]

I have reproduced this bug with Nightly 56.0a1 (2017-06-18) on Windows 8.1 , 64 Bit ! 

This bug's fix is verified with latest Nightly 56.0a1 !

Build   ID    20170714030205
User Agent    Mozilla/5.0 (Windows NT 6.3; WOW64; rv:56.0) Gecko/20100101 Firefox/56.0