1430558 - CSS autocompletes should add closing parenthesis automatically

Status: RESOLVED FIXED

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

Description

[Julian Descottes [:jdescottes]]

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.

Comment 1

[Sebastian Zartner [:sebo]]

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

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1430558 - add closing parenthesis automatically in CSS autocompletes;

Review commit: https://reviewboard.mozilla.org/r/212912/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212912/

Comment 3

[Julian Descottes [:jdescottes]]

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

Comment 4

[Sebastian Zartner [:sebo]]

(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 5

[Julian Descottes [:jdescottes]]

Comment on attachment 8942647 [details]
Bug 1430558 - add closing parenthesis automatically in CSS autocompletes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212912/diff/1-2/

Comment 6

[Julian Descottes [:jdescottes]]

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

Comment 7

[Julian Descottes [:jdescottes]]

(and here's an updated green try on Linux: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6b506ea6a18130cec61480a91a70b11f6fdb29c )

Comment 8

[Sebastian Zartner [:sebo]]

(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

Comment 9

[Sebastian Zartner [:sebo]]

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

Comment 10

[Julian Descottes [: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.

Comment 11

[Sebastian Zartner [:sebo]]

(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

[Gabriel Luong [:gl] (ΦωΦ)]

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

Comment 13

[Julian Descottes [:jdescottes]]

I totally forgot about landing this!

Comment 14

[Julian Descottes [:jdescottes]]

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 15

[Julian Descottes [:jdescottes]]

Comment on attachment 8942647 [details]
Bug 1430558 - add closing parenthesis automatically in CSS autocompletes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212912/diff/2-3/

Comment 16

[Julian Descottes [:jdescottes]]

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

Comment 17

[Julian Descottes [:jdescottes]]

Comment on attachment 8942647 [details]
Bug 1430558 - add closing parenthesis automatically in CSS autocompletes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212912/diff/3-4/

Comment 18

[Julian Descottes [:jdescottes]]

New try with the same fix as for Bug 1431949 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9226c852981ca26fe1474de162384b6997eb04b

Comment 19

[Julian Descottes [:jdescottes]]

Comment on attachment 8942647 [details]
Bug 1430558 - add closing parenthesis automatically in CSS autocompletes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212912/diff/4-5/

Comment 20

[Mozilla Autoland]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b318d775a2c8
add closing parenthesis automatically in CSS autocompletes;r=gl

Comment 22

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/b318d775a2c8