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

VERIFIED FIXED in Firefox 56

Status

()

Firefox
Developer Tools
P5
normal
VERIFIED FIXED
8 months ago
7 months ago

People

(Reporter: Md.Majedul islam, Assigned: Towkir)

Tracking

56 Branch
Firefox 56
Points:
---

Firefox Tracking Flags

(firefox56 verified)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

8 months ago
Created attachment 8878910 [details]
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.
(Assignee)

Comment 1

8 months ago
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 ?
Flags: needinfo?(ntim.bugs)
Flags: needinfo?(jryans)
See Also: → bug 1334126

Comment 2

8 months ago
(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.
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 3

8 months ago
(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>
(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...
Flags: needinfo?(jryans)
(Assignee)

Comment 5

8 months ago
Created attachment 8879614 [details] [diff] [review]
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.
Assignee: nobody → 3ugzilla
Status: NEW → ASSIGNED
Attachment #8879614 - Flags: feedback?(ntim.bugs)

Comment 6

8 months ago
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>
Attachment #8879614 - Flags: feedback?(ntim.bugs)
Summary: "Screenshort behavior " camera icon on Toolbox options is not correctly aligned → "Screenshot behavior" camera icon on Toolbox options is not correctly aligned
(Assignee)

Comment 7

8 months ago
Created attachment 8879824 [details] [diff] [review]
camera-icon-alignment.patch

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

Hope this helps.
Attachment #8879614 - Attachment is obsolete: true
Attachment #8879824 - Flags: review?(ntim.bugs)

Updated

8 months ago
Attachment #8879824 - Flags: review?(ntim.bugs) → review?(bgrinstead)
DevTools bug triage (filter on CLIMBING SHOES).
Priority: -- → P5
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
Attachment #8879824 - Flags: review?(bgrinstead) → review?(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.
Attachment #8879824 - Flags: review?(gl) → review+
(Assignee)

Comment 11

8 months ago
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 ?
Flags: needinfo?(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!
Flags: needinfo?(gl)
(Assignee)

Comment 13

7 months ago
Created attachment 8885588 [details] [diff] [review]
camera-icon-alignment.patch

Here is the updated patch
Attachment #8879824 - Attachment is obsolete: true
Attachment #8885588 - Flags: review+
(Assignee)

Comment 14

7 months ago
I hope it's ready to land now
Keywords: checkin-needed
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
Flags: needinfo?(3ugzilla)
Keywords: checkin-needed

Comment 16

7 months ago
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
(Assignee)

Comment 17

7 months ago
Created attachment 8885799 [details] [diff] [review]
camera-icon-alignment.patch

I think I had some issues with my mercurial setup on earlier patch.
Here is the updated one.
Attachment #8885588 - Attachment is obsolete: true
Flags: needinfo?(3ugzilla)
Attachment #8885799 - Flags: review+
(Assignee)

Updated

7 months ago
Keywords: checkin-needed

Comment 18

7 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b778f1f3c2d
camera icon on 'Screenshort behavior' label is now correctly aligned; r=gl
Keywords: checkin-needed

Comment 19

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9b778f1f3c2d
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56

Comment 20

7 months ago
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
QA Whiteboard: [bugday-20170712]

Updated

7 months ago
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
You need to log in before you can comment on or make changes to this bug.