1311795 - The gap in browser styles checkbox misses pointer events.

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

[johngraciliano]

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

[johngraciliano]

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

[Patrick Brosset <:pbro>]

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

[Patrick Brosset <:pbro>]

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

[Julian Descottes [:jdescottes]]

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.

[1] http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/devtools/client/themes/computed.css#39
[2] https://developer.mozilla.org/en-US/docs/Web/CSS/padding-inline-start

Comment 5

[[:Towkir] Ahmed]

Attachment: paddingfrommargin.patch

Hope the patch helps :)

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8808528 [details] [diff] [review]
paddingfrommargin.patch

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.

[1] https://addons.mozilla.org/en-GB/firefox/addon/force-rtl/

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

[Julian Descottes [:jdescottes]]

Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cc0e477bc17e5db136c3cd0b73a6c9b61dacebe

Comment 8

[[:Towkir] Ahmed]

Attachment: paddingfrommargin.patch

Here is the updated patch.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9aa5a3ed0da
Use padding instead of margin for the gap in browser styles checkbox and label. r=jdescottes

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Md Maruf Rahman]

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

[testday-20170203]

Comment 12

[Md. Majedul islam]

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

[testday-20170203]