745961 - Very hard to find the clickable region for adding a new CSS property in the Style Inspector

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

When trying to add a CSS property in the Style Inspector, I often have to click > 5 times in various places before the input will appear.

I usually succeed when clicking in the column of the expander arrows. I expect to be able to click anywhere to the right of existing property values or underneath the last property value for a given ruleset, but this expectation is never met.

Comment 1

[Dave Camp (:dcamp)]

I think you have to click ON the closing brace.  Really, it should be anywhere on the same line as the closing brace...

Comment 2

[Paul Rouget [:paul]]

I also think we should add a "+" icon (maybe while hovering the bracket).

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

If we add a '+' icon, then it should be inside of the braces, in which case it would seem awkward if the position of the '+' icon moved while hovering over an element.

I think we should just allow double-clicking anywhere inside of a ruleview-propertylist that is not on top of the checkbox or preexisting rule.

Comment 4

[Josh Matthews [:jdm]]

Dave, could you link to the relevant source file that would require changing and give some context for the expected fix?

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Dave Camp (:dcamp) from comment #1)
> I think you have to click ON the closing brace.  Really, it should be
> anywhere on the same line as the closing brace...

Yeah, but then it becomes almost impossible to select text for copying. We could always add context menu items for "Add Property", "Delete Property" and "Delete Rule." Like Jared, ideally I think edits should be double-clicks but we would need to remove the reliability on focus in order to do that, which isn't a bad idea.

Comment 6

[Wilbert Sequeira]

Can I get further info on this bug? I would like to see if I can give it a try.

Wilbert

Comment 7

[Dave Camp (:dcamp)]

The code for the rule view is in browser/devtools/styleinspector/CssRuleView.jsm

The cross-platform styling for the rule view is in browser/devtools/styleinspector/styleinspector.css (there's some theme-specific styling elsewhere but I don't think it would be involved in a bugfix here).

Comment 8

[Dave Camp (:dcamp)]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #5)
> (In reply to Dave Camp (:dcamp) from comment #1)
> Yeah, but then it becomes almost impossible to select text for copying. We
> could always add context menu items for "Add Property", "Delete Property"
> and "Delete Rule." Like Jared, ideally I think edits should be double-clicks
> but we would need to remove the reliability on focus in order to do that,
> which isn't a bad idea.

We'd just need to reuse the fix we did for editableField, right?

It should work to separate out the code in editableField() after the "focus on click instead" comment into its own method.  Call that from editableField(), and also call it for this.closeBrace in RuleEditor._create()

Then we just need the right css to extend the length of the closing brace.

Comment 9

[Dave Camp (:dcamp)]

Another option would be to not use the closing brace at all, but to allow clicking anywhere in the rule to add a new item.  That would involve two steps I think:

* Handle click on RuleEditor.element instead of RuleEditor.closeBrace
* I think we'd want to set a min-width on .ruleview-propertyname, so that small property names are easy to edit (this would be useful either way).

Does that make sense?

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I think you mean double-clicking? Otherwise it would not be possible to highlight anything for copy / paste.

Comment 11

[Dave Camp (:dcamp)]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #10)
> I think you mean double-clicking? Otherwise it would not be possible to
> highlight anything for copy / paste.

No, a single-click works fine, you don't process the click if a selection started between mouseup/mousedown.

Comment 12

[Dave Camp (:dcamp)]

Attachment: A possible patch

This ended up being harder than I expected, mostly due to bug 734365.

This patch makes a few changes:
a) Editable fields are underlined when clicking on them will start editing.
b) I added a bit of extra padding/active area extending a few pixels past the end of property values and their associated semicolon, to help with smaller targets.
b) Same with property names, the colon is now part of their active area.
d) Clicking anywhere in the rule area will add a new property.

Can people play with this and let me know what you think?

Comment 13

[Paul Rouget [:paul]]

(In reply to Dave Camp (:dcamp) from comment #12)
> Created attachment 626642 [details] [diff] [review]
> A possible patch
> 
> This ended up being harder than I expected, mostly due to bug 734365.
> 
> This patch makes a few changes:
> a) Editable fields are underlined when clicking on them will start editing.

Hot!

> b) I added a bit of extra padding/active area extending a few pixels past
> the end of property values and their associated semicolon, to help with
> smaller targets.

ok.

> b) Same with property names, the colon is now part of their active area.

ok.

> d) Clicking anywhere in the rule area will add a new property.

This is a very good idea. I love that. Especially for the element{} block.

Also, not sure if we should do that here or not, but it would be cool if the new editor shows up on top of the other declarations when we click on the first bracket.

Problem: selecting part of the selector with the mouse brings the editor.

Comment 14

[Paul Rouget [:paul]]

Comment on attachment 626642 [details] [diff] [review]
A possible patch

For the underline, I suggest a dashed border instead.

Comment 15

[Dave Camp (:dcamp)]

Attachment: v2

Comment 16

[Dave Camp (:dcamp)]

(In reply to Paul Rouget [:paul] from comment #13)

> Also, not sure if we should do that here or not, but it would be cool if the
> new editor shows up on top of the other declarations when we click on the
> first bracket.

This is a bit more work, I'll file a followup.

> Problem: selecting part of the selector with the mouse brings the editor.

Fixed.

> For the underline, I suggest a dashed border instead.

Done.

Comment 17

[Paul Rouget [:paul]]

Comment on attachment 628526 [details] [diff] [review]
v2

> +.ruleview-namecontainer:hover > .ruleview-propertyname {
> +  border-bottom: 1px dotted;
> +}
> 
> […]
> 
> +.ruleview-propertycontainer:hover > .ruleview-propertyvalue {
> +  border-bottom: 1px dotted;
> +}

You add a border on over. This will trigger a reflow, and might shift the following content. Instead, move the code to theme/ and change the color (and merge the rules):

> .ruleview-namecontainer > .ruleview-propertyname,
> .ruleview-propertycontainer > .ruleview-propertyvalue {
>   border-bottom: 1px dotted transparent;
> }
> 
> .ruleview-namecontainer:hover > .ruleview-propertyname,
> .ruleview-propertycontainer:hover > .ruleview-propertyvalue {
>   border-bottom-color: black;
> }

r+ with this change.

Comment 18

[Dave Camp (:dcamp)]

https://hg.mozilla.org/integration/fx-team/rev/13d899f25454

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/mozilla-central/rev/13d899f25454

Comment 20

[Andrea]

(In reply to Dave Camp (:dcamp) from comment #12)
> Created attachment 626642 [details] [diff] [review]
> A possible patch
> 
> This ended up being harder than I expected, mostly due to bug 734365.
> 
> This patch makes a few changes:
> a) Editable fields are underlined when clicking on them will start editing.
> b) I added a bit of extra padding/active area extending a few pixels past
> the end of property values and their associated semicolon, to help with
> smaller targets.
> b) Same with property names, the colon is now part of their active area.
> d) Clicking anywhere in the rule area will add a new property.
> 
> Can people play with this and let me know what you think?

It's much more usable now!