[ruleview] Clicking on the left of checkbox should change its state, not creating new rule

RESOLVED FIXED in Firefox 64

Status

P3
normal
RESOLVED FIXED
3 years ago
7 days ago

People

(Reporter: arni2033, Assigned: aburone, Mentored)

Tracking

({ux-trust})

Trunk
Firefox 64
ux-trust

Firefox Tracking Flags

(firefox42 affected, firefox64 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
Created attachment 8642101 [details]
Proposal on clicking behavior in ruleview.png

STR:
1. Open devtools->Inspector->ruleview
2. Click on the left of checkbox (~ 3px away from checkbox)

RESULT:
New blank rule was created

EXPECTATIONS:
Clicking should just change state of checkbox. I believe that this can be solved with CSS styling (for that pane).
(Reporter)

Updated

3 years ago
See Also: → bug 699215
Summary: [ruleview] Clicking on the left of checkbox should change it's state, not creating new rule → [ruleview] Clicking on the left of checkbox should change its state, not creating new rule
(Reporter)

Updated

3 years ago
Keywords: ux-trust
Thanks for filing this, I agree this would make the rule-view easier to use. A bit like in gmail where, even if you click like 5 px away from the checkbox to select an email, it still checks/unchecks the box.
Mentor: pbrosset
(Reporter)

Comment 2

3 years ago
My screenshot also describes another area, for editing the rule (when there's no dropmarker)
However, it's less important that checkboxes, because rules are easy-to-hit with mouse anyway.
(Reporter)

Updated

3 years ago
Has STR: --- → yes
Component: Developer Tools: Inspector → Developer Tools: CSS Rules Inspector

Comment 3

3 years ago
I would like to work on it.
(In reply to Neel Dalsania from comment #3)
> I would like to work on it.
Sure, that'd be nice. Please go through our contribution guide first: https://wiki.mozilla.org/DevTools/Hacking
Once you have the environment ready and you know how to make changes/build/reload/test your changes, then you'll be ready to get started.

In terms of code, the checkbox is a DOM element that has the class "ruleview-enableproperty" and is created in this file: devtools\client\inspector\rules\views\text-property-editor.js
You'll see that in this file (in the '_create' method), we create the element and add a click event listener to it.

You need to find a way to expand the clickable area of the checkbox somehow.
I haven't investigated this too much yet, so please do try and come up with a solution, and let's discuss in this bug when you do, and/or if you need help.

Comment 5

3 years ago
I was trying to make a div surrounding the check box and make it clickable to change the state of the checkbox, but i wasn't able to place the checkbox correctly in the new div and getting images of one checked and another unchecked checkbox overlapping over each other, and clicking in the new div would also cause the change in the state of the drop down arrow. 

Any suggestion how to improvise my implementation method or there exist a better solution.
Hmm, this is indeed not a super simple change to make.
The checkbox is displayed immediately next to a 3px left border that we use to highlight properties that have been changed (you won't see it by default, but try to change a value in the rule-view, and you'll see the border turn green).
And immediately to the right, there's the twisty triangle icon.
So giving more space to the checkbox is going to push it away from these 2 things, which we don't want.

We could investigate some css hacks, maybe using position:relative;left:-3px; on the checkbox, to make it overlap the green border to the left, but then, make its width 3px larger to the right, and re-center the background image (possibly background-clip if that helps) so that nothing would change visually, but the the net effect is that the checkbox would extend for 3 more px to the left.

Another solution would be to make the left border clickable too. Keep the checkbox as it is now, but detect clicks on the area where the left border is supposed to be and when they happen, delegate that click to the checkbox handler instead.
In terms of code, you'll want to look at devtools\client\inspector\rules\views\text-property-editor.js (especially in `_create`).
`this.element` in here is the whole DIV that contains the property, and is the one that receives a 3px left border.
Just thinking out loud here, but maybe you could try adding a click event handler to that element, and detect if the click happens in the left border area using the mouse coordinates.
No need to hard-code the 3x border width in JS btw, you could subtract:
this.element.getBoxQuads({box: "border"})[0].bounds.left
from:
this.element.getBoxQuads({box: "padding"})[0].bounds.left
When that happens, just call `_onEnableClicked` with the event.

That's all I could think of right now, hopefully you can investigate those 2 solutions and see what works well/is the least complicated.
Priority: -- → P3

Updated

4 months ago
Product: Firefox → DevTools
(Assignee)

Comment 7

7 days ago
Created attachment 9015847 [details]
Summary: [ruleview] Clicking on the left of checkbox should change its state, not creating new rule
(Assignee)

Comment 8

7 days ago
Hi, I made the changes using css.

Is there any reason for the checkboxes being divs instead of proper checkboxes?
It would have been WAY easier to fix, not to mention that these can't be activated using the keyboard :/

Anyways. This patch gets the expected behavior? or is there something I miss?

Thanks
Thank you for working on this Ariel.

I don't know about the reasons to use divs instead of checkboxes. This predates my involvement with DevTools.
I agree that's not ideal because of keyboard navigation as you said, but also they look different than the other checkboxes displayed elsewhere in DevTools.
So I believe we should change this.

Now, having said this, your patch does seem to fix the issue described here. So, good work on this.
I'll take a look at the code changes, and I think we should land this.

We should also spend the time to do the switch to checkboxes (and try to understand why we didn't use them in the first place, maybe there's a reason I just don't know about).
Would you be interested in doing this?
Assignee: nobody → aburone
Status: NEW → ASSIGNED

Comment 10

7 days ago
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8efb5f963758
Summary: [ruleview] Clicking on the left of checkbox should change its state, not creating new rule r=pbro
(Assignee)

Comment 11

7 days ago
(In reply to Patrick Brosset <:pbro> from comment #9)
> Thank you for working on this Ariel.
> 
> I don't know about the reasons to use divs instead of checkboxes. This
> predates my involvement with DevTools.
> I agree that's not ideal because of keyboard navigation as you said, but
> also they look different than the other checkboxes displayed elsewhere in
> DevTools.
> So I believe we should change this.

Great

> 
> Now, having said this, your patch does seem to fix the issue described here.
> So, good work on this.
> I'll take a look at the code changes, and I think we should land this.
> 
> We should also spend the time to do the switch to checkboxes (and try to
> understand why we didn't use them in the first place, maybe there's a reason
> I just don't know about).
> Would you be interested in doing this?

In doing the change? yeah!
Understand why wheren't used in the first place? That would be hard to me. I'm not in any mail list/chat and I don't know anyone... I would not know where to ask.

Comment 12

7 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8efb5f963758
Status: ASSIGNED → RESOLVED
Last Resolved: 7 days ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
You need to log in before you can comment on or make changes to this bug.