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

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, defect, P3)

Description

[arni2033]

Attachment: 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).

Comment 1

[Patrick Brosset <:pbro>]

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.

Comment 2

[arni2033]

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.

Comment 3

[Neel Dalsania]

I would like to work on it.

Comment 4

[Patrick Brosset <:pbro>]

(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

[Neel Dalsania]

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.

Comment 6

[Patrick Brosset <:pbro>]

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.

Comment 7

[Ariel Burone]

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

Comment 8

[Ariel Burone]

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

Comment 9

[Patrick Brosset <:pbro>]

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?

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Ariel Burone]

(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

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/8efb5f963758

Comment 13

[Ariel Burone]

> 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?

Patrick:
Sorry for continuing this here but I'm not sure how to proceed.

I been looking at the ckeckbox problem... It's worse than expected. eg: the settings screen is created in the same way.
I fixed the inspector but broke the settings screen. If I fix one I have to fix every fake ckeckbox :(

Also. Even when the checkbox is present, pressing tab skips it... something is hijacking the keyboard. Shall I create a new bug that implies correcting ALL the checkboxes, and the tab skipping, and the absence of focus ring? :/

PS: I was in bed until yesterday, that's why I haven't raised this before.

Comment 14

[Patrick Brosset <:pbro>]

(In reply to Ariel Burone from comment #13)
> Patrick:
> Sorry for continuing this here but I'm not sure how to proceed.
No problem at all, it's fine to continue discussing here. Also, sorry for taking so long to reply.

> I been looking at the ckeckbox problem... It's worse than expected. eg: the
> settings screen is created in the same way.
How do you mean? When I inspect the setting screen, I see normal checkboxes (input elements, not divs).
> I fixed the inspector but broke the settings screen. If I fix one I have to
> fix every fake ckeckbox :(
Could you point me to places in the code where other fake checkboxes are being generated?
Doing a quick search in the code does seem to show they're only used in the rules-view: https://searchfox.org/mozilla-central/search?q=theme-checkbox&path=devtools (but maybe we use other classes elsewhere).
> Also. Even when the checkbox is present, pressing tab skips it... something
> is hijacking the keyboard. Shall I create a new bug that implies correcting
> ALL the checkboxes, and the tab skipping, and the absence of focus ring? :/
Yes, in any case we should be working off of a new bug because this one has been resolved already.
I've created one here: bug 1501194.