CSS autocompletes should add closing parenthesis automatically

RESOLVED FIXED in Firefox 61

Status

enhancement
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jdescottes, Assigned: jdescottes)

Tracking

58 Branch
Firefox 61
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment)

Follow up to bug 1422635, we should add the closing parenthesis automatically when the user types "var(" to avoid autocompleting and committing invalid CSS values.
Of course, that should not only work for the var() function but for all CSS functions like calc() or the different filter functions.
Also, probably obvious but I want to mention it anyway, when the closing parenthesis is inserted the text cursor must be placed between the two parentheses.

If possible, the closing parenthesis should be skipped (the text cursor be placed after it) when the text cursor is placed before a closing parenthesis and a closing parenthesis is inserted. This would give the best UX, as you can just type the value without using the mouse or the arrow keys to navigate the text cursor after the closing parenthesis.

Sebastian
Still needs a test + reviewing more carefully in which case we enable this autoclose. We could extend this behavior to other characters later (quotes, brackets etc...).

(In reply to Sebastian Zartner [:sebo] from comment #1)
> Of course, that should not only work for the var() function but for all CSS
> functions like calc() or the different filter functions.
> Also, probably obvious but I want to mention it anyway, when the closing
> parenthesis is inserted the text cursor must be placed between the two
> parentheses.
> 
> If possible, the closing parenthesis should be skipped (the text cursor be
> placed after it) when the text cursor is placed before a closing parenthesis
> and a closing parenthesis is inserted. This would give the best UX, as you
> can just type the value without using the mouse or the arrow keys to
> navigate the text cursor after the closing parenthesis.
> 
> Sebastian

Thanks for the feedback! I think that's what I implemented so far, feel free to give it a spin to check.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ba78b0d5586eb82400e347fa40c27ebabcac403
(In reply to Julian Descottes [:jdescottes][:julian] from comment #3)
> (In reply to Sebastian Zartner [:sebo] from comment #1)
> > Of course, that should not only work for the var() function but for all CSS
> > functions like calc() or the different filter functions.
> > Also, probably obvious but I want to mention it anyway, when the closing
> > parenthesis is inserted the text cursor must be placed between the two
> > parentheses.
> > 
> > If possible, the closing parenthesis should be skipped (the text cursor be
> > placed after it) when the text cursor is placed before a closing parenthesis
> > and a closing parenthesis is inserted. This would give the best UX, as you
> > can just type the value without using the mouse or the arrow keys to
> > navigate the text cursor after the closing parenthesis.
> > 
> > Sebastian
> 
> Thanks for the feedback! I think that's what I implemented so far, feel free
> to give it a spin to check.

I'd like to, though I'm on Windows and in the middle of a move, so I can't compile it myself right now. Is it possible you make a try build for Windows, so I can test it?

Sebastian
Comment hidden (mozreview-request)
Won't land in 59, but we can get started on reviews. Can easily be extended to support more characters.

(In reply to Sebastian Zartner [:sebo] from comment #4)
> 
> I'd like to, though I'm on Windows and in the middle of a move, so I can't
> compile it myself right now. Is it possible you make a try build for
> Windows, so I can test it?
> 

Sure here's one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bc3310d01a2d48c046cf755857abfa4c88b6a03
(In reply to Julian Descottes [:jdescottes][:julian] from comment #6)
> Won't land in 59, but we can get started on reviews. Can easily be extended
> to support more characters.
> 
> (In reply to Sebastian Zartner [:sebo] from comment #4)
> > 
> > I'd like to, though I'm on Windows and in the middle of a move, so I can't
> > compile it myself right now. Is it possible you make a try build for
> > Windows, so I can test it?
> > 
> 
> Sure here's one:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=4bc3310d01a2d48c046cf755857abfa4c88b6a03

Sorry for bothering you again but I don't find the link where I can download it. Shouldn't it be where it says "Build:"?

Sebastian
ni for two things.

1. A try build for Windows 10, as there doesn't seem to be a download link in the previous try build.
2. Is this issue meant to also cover the addition of parentheses for entered function names? For example, Firebug autocompleted 'ca' to 'calc()' and by pressing the Right Arrow key the text cursor was placed between the parentheses.
If not, I'll create a new bug for that.

Sebastian
Flags: needinfo?(jdescottes)
(In reply to Sebastian Zartner [:sebo] from comment #9)
> ni for two things.
> 
> 1. A try build for Windows 10, as there doesn't seem to be a download link
> in the previous try build.

I assume you can use the Windows 2012 build artifacts? That's presumably what the Windows 10 & 7 jobs are using to run their tests.

https://queue.taskcluster.net/v1/task/Y4_JdG4pSeazkZCFXPXIxA/runs/0/artifacts/public/build/install/sea/target.installer.exe worked fine on a Win7 machine here.

> 2. Is this issue meant to also cover the addition of parentheses for entered
> function names? For example, Firebug autocompleted 'ca' to 'calc()' and by
> pressing the Right Arrow key the text cursor was placed between the
> parentheses.

Yes it does cover autoclosing any open parenthesis. But we do not autocomplete "ca" to "calc" ( or to "calc(" or "calc()" for that matter). So you can file issues about that last part.
Flags: needinfo?(jdescottes)
See Also: → 1432452
(In reply to Julian Descottes [:jdescottes][:julian] from comment #10)
> (In reply to Sebastian Zartner [:sebo] from comment #9)
> > ni for two things.
> > 
> > 1. A try build for Windows 10, as there doesn't seem to be a download link
> > in the previous try build.
> 
> I assume you can use the Windows 2012 build artifacts? That's presumably
> what the Windows 10 & 7 jobs are using to run their tests.
> 
> https://queue.taskcluster.net/v1/task/Y4_JdG4pSeazkZCFXPXIxA/runs/0/
> artifacts/public/build/install/sea/target.installer.exe worked fine on a
> Win7 machine here.

Ah, I missed that. That worked for me, thanks!
And from a quick test the feature works as expected. So, double-thanks! :-)

One additional idea came to my mind while trying it out. It would be great if setting the parentheses worked like in an IDE. That means, when the cursor is placed before an opening parenthesis and you enter '(', it should also be skipped, i.e. the cursor should be placed after the opening parenthesis without adding a second one, like you did it for the closing one.
Also, when the cursor is placed right after the opening parenthesis and you press Backspace or right before it and press Delete, both parentheses should be removed.

Still in the scope of this issue? If not, worth filing a new one?

> > 2. Is this issue meant to also cover the addition of parentheses for entered
> > function names? For example, Firebug autocompleted 'ca' to 'calc()' and by
> > pressing the Right Arrow key the text cursor was placed between the
> > parentheses.
> 
> Yes it does cover autoclosing any open parenthesis. But we do not
> autocomplete "ca" to "calc" ( or to "calc(" or "calc()" for that matter). So
> you can file issues about that last part.

Created bug 1432452.

Sebastian

Comment 12

a year ago
mozreview-review
Comment on attachment 8942647 [details]
Bug 1430558 - add closing parenthesis automatically in CSS autocompletes;

https://reviewboard.mozilla.org/r/212912/#review221778

::: devtools/client/inspector/markup/test/browser_markup_css_completion_style_attribute_02.js:87
(Diff revision 2)
>    [":", "style=\"background:aliceblue", 18, 27, true],
>    ["u", "style=\"background:unset", 19, 23, true],
>    ["r", "style=\"background:url", 20, 21, false],
>    ["l", "style=\"background:url", 21, 21, false],
> -  ["(", "style=\"background:url(", 22, 22, false],
> -  ["'", "style=\"background:url('", 23, 23, false],
> +  ["(", "style=\"background:url()", 22, 22, false],
> +  ["'", "style=\"background:url(')", 23, 23, false],

We should probably autoclose all these brackets and quotations in general. Seems like the right thing to do.

::: devtools/client/shared/inplace-editor.js:50
(Diff revision 2)
> +const WORD_REGEXP = /\w/;
> +const isWordChar = function (str) {
> +  return str && WORD_REGEXP.test(str);
> +};
> +
>  const EventEmitter = require("devtools/shared/old-event-emitter");

It looked weird to me that we are defining constants before require(), but turns out we should probably just move these require() below KeyCodes
Attachment #8942647 - Flags: review?(gl) → review+
I totally forgot about landing this!
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Assignee

Comment 14

a year ago
mozreview-review-reply
Comment on attachment 8942647 [details]
Bug 1430558 - add closing parenthesis automatically in CSS autocompletes;

https://reviewboard.mozilla.org/r/212912/#review221778

Thanks for the review Gabriel, and thanks for the feedback Sebastian. I prefer to log another bug to agree on what characters we should autocomplete and/or skip. For instance about skipping the opening paren, my current editors don't do that so I'd prefer to check with others before taking decisions here.

> We should probably autoclose all these brackets and quotations in general. Seems like the right thing to do.

I agree, let's check with UX/product to refine that. I will log a follow up!

> It looked weird to me that we are defining constants before require(), but turns out we should probably just move these require() below KeyCodes

Fixed, thanks!
Comment hidden (mozreview-request)
I rebased this on top of Bug 1431949 since it will touch the same test and blocked it on it.
Here's a try push with the last version https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ba6986172c5e8de1085f56c9d53a0d6fa6c2bd3
Depends on: 1431949
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
hg error in cmd: hg pull gecko -r c081b53042dcc14c6f7bb022b35edab12a5deff0: pulling from https://reviewboard-hg.mozilla.org/gecko
abort: HTTP Error 504: Gateway Timeout

Comment 21

a year ago
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b318d775a2c8
add closing parenthesis automatically in CSS autocompletes;r=gl

Comment 22

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b318d775a2c8
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61

Updated

a year ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.