The gap in browser styles checkbox misses pointer events.

RESOLVED FIXED in Firefox 52

Status

()

Firefox
Developer Tools: Computed Styles Inspector
P3
trivial
RESOLVED FIXED
10 months ago
6 months ago

People

(Reporter: johngraciliano, Assigned: Towkir, Mentored)

Tracking

({good-first-bug, regression})

52 Branch
Firefox 52
good-first-bug, regression
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

10 months ago
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.
(Reporter)

Comment 1

10 months ago
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.
Severity: normal → trivial
Component: Untriaged → Developer Tools: Computed Styles Inspector

Comment 2

10 months ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression

Comment 3

10 months ago
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.
Flags: needinfo?(jdescottes)
Priority: -- → P3
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
Mentor: jdescottes@mozilla.com
Flags: needinfo?(jdescottes)
Keywords: good-first-bug
(Assignee)

Comment 5

10 months ago
Created attachment 8808528 [details] [diff] [review]
paddingfrommargin.patch

Hope the patch helps :)
Assignee: nobody → 3ugzilla
Status: NEW → ASSIGNED
Attachment #8808528 - Flags: review?(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.
Attachment #8808528 - Flags: review?(jdescottes) → review+
Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cc0e477bc17e5db136c3cd0b73a6c9b61dacebe
(Assignee)

Comment 8

10 months ago
Created attachment 8808628 [details] [diff] [review]
paddingfrommargin.patch

Here is the updated patch.
Attachment #8808528 - Attachment is obsolete: true
Attachment #8808628 - Flags: review+
(Assignee)

Updated

10 months ago
Keywords: checkin-needed

Comment 9

10 months ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 10

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d9aa5a3ed0da
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
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

7 months ago
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]
You need to log in before you can comment on or make changes to this bug.