Last Comment Bug 1311795 - The gap in browser styles checkbox misses pointer events.
: The gap in browser styles checkbox misses pointer events.
: good-first-bug, regression
Product: Firefox
Classification: Client Software
Component: Developer Tools: Computed Styles Inspector (show other bugs)
: 52 Branch
: Unspecified Unspecified
P3 trivial (vote)
: Firefox 52
Assigned To: [:Towkir] Ahmed
: Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ)
Mentors: Julian Descottes [:jdescottes]
Depends on:
  Show dependency treegraph
Reported: 2016-10-20 12:20 PDT by johngraciliano
Modified: 2017-02-03 06:24 PST (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

paddingfrommargin.patch (753 bytes, patch)
2016-11-08 01:10 PST, [:Towkir] Ahmed
jdescottes: review+
Details | Diff | Splinter Review
paddingfrommargin.patch (735 bytes, patch)
2016-11-08 07:09 PST, [:Towkir] Ahmed
3ugzilla: review+
Details | Diff | Splinter Review

Description User image johngraciliano 2016-10-20 12:20:41 PDT
User Agent: Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20161020030211

Steps to reproduce:

In Firefox 52, open the Inspector Tool.
Select the Computed tab.
Position the pointer between the box and the label 'browser styles' checkbox.

Actual results:

The box is not highlighted and mouse 'clicks' have no effect.

Expected results:

The box should be highlighted and the mouse 'click' should toggle the presence of a check mark in the box.
Comment 1 User image johngraciliano 2016-10-20 12:23:57 PDT
This is because the 'input[id=browser-style-checkbox]' precedes 'label[id=browser-style-checkbox-label]' (it was inside).  The gap between these is a margin and the pointer events are received by the container 'div[id=#computedview-toolbar]'. This problem can be fixed swapping the values of 'margin-inline-end' of 'input[id=browser-style-checkbox]' and 'padding-inline-start' of 'label[id=browser-style-checkbox-label]', which are '5px' and '0' respectively.
Comment 2 User image Patrick Brosset <:pbro> 2016-10-21 05:19:15 PDT
This is a regression. It used to work before when the checkbox was a child of the label element.
Now the checkbox and the label are siblings.
I think this was changed in bug 1294186.
Comment 3 User image Patrick Brosset <:pbro> 2016-10-28 07:25:27 PDT
Julian, do you mind checking if this is a simple enough bug for being goof-first and mentored? If yes, please give a pointer to the file that needs changing so someone can start working on it.
Comment 4 User image Julian Descottes [:jdescottes] 2016-11-07 02:16:11 PST
Sure, looks like a good first bug!

I think the easiest way to go here is to update the CSS for the input and label in order to make the gap clickable.
Right now we have a 5px margin at the right of the input, defined in devtools/client/themes/computed.css (see [1]).

We should set the margin-inline-end to 0 and compensate with a 5px padding on the left of the label (#browser-style-checkbox-label). This should work in both left-to-right and right-to-left directions, so we should use logical padding properties here (see [2]). 

On a sidenote, the label currently has a `margin-right` property defined. It would be nice to replace this by the logical property equivalent: `margin-inline-end`.

I don't think it's worth adding a test here, because it would have to simulate a click on a very specific position and would be both fragile and too tied to the implementation.

Comment 5 User image [:Towkir] Ahmed 2016-11-08 01:10:10 PST
Created attachment 8808528 [details] [diff] [review]

Hope the patch helps :)
Comment 6 User image Julian Descottes [:jdescottes] 2016-11-08 01:34:19 PST
Comment on attachment 8808528 [details] [diff] [review]

Review of attachment 8808528 [details] [diff] [review]:

Looks good to me, thanks.
Works well in both LTR and RTL directions. 
By the way you can use the ForceRTL addon [1] to test this, forgot to mention it earlier.


Two things to address before landing:
- use a shorter commit summary
- small nit in computed.css below

Feel free to set r+ directly on your follow up patch addressing this and add the checkin-needed keyword.

Thanks for taking this one Towkir!

::: devtools/client/themes/computed.css
@@ +40,1 @@

nit: let's remove this blank line while we're at it.
Comment 7 User image Julian Descottes [:jdescottes] 2016-11-08 01:43:43 PST
Pushed to try:
Comment 8 User image [:Towkir] Ahmed 2016-11-08 07:09:38 PST
Created attachment 8808628 [details] [diff] [review]

Here is the updated patch.
Comment 9 User image Pulsebot 2016-11-08 12:36:18 PST
Pushed by
Use padding instead of margin for the gap in browser styles checkbox and label. r=jdescottes
Comment 10 User image Carsten Book [:Tomcat] 2016-11-09 07:41:16 PST
Comment 11 User image Maruf Rahman[:mMARUF] 2017-02-03 01:11:13 PST
I have reproduced this bug with Nightly 52.0a1 (2016-10-20) (64-bit) on Windows 7 , 64 Bit !

This bug's fix is verified with latest Beta !

Build   ID  : 20170202101509
User Agent  : Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0

Comment 12 User image Md.Majedul isalm 2017-02-03 06:24:06 PST
I have reproduced this bug with Nightly 52.0a1 (2016-10-20) (64-bit) on Ubuntu 16.10 , 64 Bit !

This bug's fix is verified with latest Beta !

Build   ID  : 20170202101509
User Agent  : Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0


Note You need to log in before you can comment on or make changes to this bug.