Closed Bug 745961 Opened 9 years ago Closed 9 years ago

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

Categories

(DevTools :: Inspector, defect)

14 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 15

People

(Reporter: jaws, Assigned: dcamp)

References

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 file, 1 obsolete file)

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.
I think you have to click ON the closing brace.  Really, it should be anywhere on the same line as the closing brace...
Whiteboard: [good-first-bug][mentor=dcamp]
Whiteboard: [good-first-bug][mentor=dcamp] → [good-first-bug][mentor=dcamp][lang=css]
I also think we should add a "+" icon (maybe while hovering the bracket).
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.
Dave, could you link to the relevant source file that would require changing and give some context for the expected fix?
(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.
Can I get further info on this bug? I would like to see if I can give it a try.

Wilbert
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).
(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.
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?
I think you mean double-clicking? Otherwise it would not be possible to highlight anything for copy / paste.
(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.
Attached patch A possible patch (obsolete) — Splinter Review
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?
Assignee: nobody → dcamp
Attachment #626642 - Flags: feedback?
Depends on: 719832
Attachment #626642 - Flags: feedback? → feedback?(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 on attachment 626642 [details] [diff] [review]
A possible patch

For the underline, I suggest a dashed border instead.
Attachment #626642 - Flags: feedback?(paul) → feedback+
Depends on: 734365
No longer depends on: 719832
Attached patch v2Splinter Review
Attachment #626642 - Attachment is obsolete: true
Attachment #628526 - Flags: review?(paul)
(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 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.
Attachment #628526 - Flags: review?(paul) → review+
https://hg.mozilla.org/integration/fx-team/rev/13d899f25454
Whiteboard: [good-first-bug][mentor=dcamp][lang=css] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/13d899f25454
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
(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!
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.