Closed
Bug 1311795
Opened 8 years ago
Closed 8 years ago
The gap in browser styles checkbox misses pointer events.
Categories
(DevTools :: Inspector, defect, P3)
Tracking
(firefox52 fixed)
RESOLVED
FIXED
Firefox 52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: johngraciliano, Assigned: Towkir, Mentored)
Details
(Keywords: good-first-bug, regression)
Attachments
(1 file, 1 obsolete file)
735 bytes,
patch
|
Towkir
:
review+
|
Details | Diff | Splinter Review |
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•8 years 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•8 years 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.
Comment 3•8 years 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
Comment 4•8 years ago
|
||
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
Assignee | ||
Comment 5•8 years ago
|
||
Hope the patch helps :)
Comment 6•8 years ago
|
||
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+
Comment 7•8 years ago
|
||
Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cc0e477bc17e5db136c3cd0b73a6c9b61dacebe
Assignee | ||
Comment 8•8 years ago
|
||
Here is the updated patch.
Attachment #8808528 -
Attachment is obsolete: true
Attachment #8808628 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d9aa5a3ed0da
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Comment 11•7 years ago
|
||
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 years 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]
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•3 years ago
|
Component: Inspector: Computed → Inspector
You need to log in
before you can comment on or make changes to this bug.
Description
•