1254070 - markup-view: style attribute autocomplete works only when caret is at the end of the input

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Julian Descottes [:jdescottes]]

STRs:
- open the markup view
- add an attribute on any node
- type `style="color: red;"`

Expected: 
- should have autocompletion both for the property name and the property value

Actual:
- autocompletion is not working

Comment 1

[Julian Descottes [:jdescottes]]

This regression was caused by Bug 913955 .

The fix is quite simple, the test at https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/inplace-editor.js#1155 

should change from:

> input.selectionStart < input.value.length

to

> input.selectionStart < input.value.length - 1

pbro: This feature has been missing for 2 years and no one complained. Can you confirm we should resurrect it?

Comment 2

[Julian Descottes [:jdescottes]]

As discussed on IRC, the autocompletion only works if you are editing the last character of the attribute. If you are writing first `style=""` and then typing between the quotes, the autocompletion is not triggered. 

and input.selectionStart < input.value.length is actually a valid test, true only when the selection is after the last character of the input.

Comment 3

[Patrick Brosset <:pbro>]

Filter on CLIMBING SHOES

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: bug1254070.wip.patch

Gabriel: The patch summary is a bit confusing, so I rephrase it here. The goal is to make the autocomplete of the style attribute a bit more flexible. Right now it works only when we are at the end of the input or if the next character is a space.

I think you added the condition checking for end of input or space in Bug 913955, so I'd like your feedback here.

This patch proposes to activate the autocompletion if the next character is not a "word" caracter or a dash. We could further restrict this behaviour to the MIXED_CONTENT type (ie. style attribute only) but I think it can be useful in the rule view as well.

Comment 6

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Julian, this bug was reviewed in the Platform triage meeting and it seems the wip patch got stuck somewhere along the way. Do you want to find another reviewer?

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: Bug 1254070 - inspector: trigger autocomplete if nextchar is not a word character;

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

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8728400 [details] [diff] [review]
bug1254070.wip.patch

Let's try with a mozreview request! Would really prefer :gl to have a look here.

Try push for the rebased patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dec28360df75

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8756866 [details]
Bug 1254070 - inspector: trigger autocomplete if nextchar is not a word character;

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

Comment 10

[Julian Descottes [:jdescottes]]

Test needed updating after rebase. New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cd71e06b265

Comment 11

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

Comment on attachment 8756866 [details]
Bug 1254070 - inspector: trigger autocomplete if nextchar is not a word character;

https://reviewboard.mozilla.org/r/55454/#review52418

::: devtools/client/inspector/markup/test/browser_markup_css_completion_style_attribute_03.js:51
(Diff revision 2)
> +  ["VK_RETURN", "style=\"color:beige;padding:0;\"",
> +   -1, -1, false]
> +];
> +
> +add_task(function* () {
> +  info("Opening the inspector on the test URL");

Remove this info() since it is repeating openInspectorForURL()

Comment 12

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

https://reviewboard.mozilla.org/r/55454/#review54680

::: devtools/client/shared/inplace-editor.js:1305
(Diff revision 2)
>        // If nothing is selected and there is a non-space character after the
>        // cursor, do not autocomplete.
>        if (input.selectionStart == input.selectionEnd &&
> -          input.selectionStart < input.value.length &&
> -          input.value.slice(input.selectionStart)[0] != " ") {
> +          input.selectionStart < input.value.length) {
> +        let nextChar = input.value.slice(input.selectionStart)[0];
> +        // Check the next character if editing inside a word.

Can we also add a bit more detail on this comment as well?

Comment 13

[Julian Descottes [:jdescottes]]

Comment on attachment 8756866 [details]
Bug 1254070 - inspector: trigger autocomplete if nextchar is not a word character;

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

Comment 14

[Julian Descottes [:jdescottes]]

https://reviewboard.mozilla.org/r/55454/#review52418

Thanks for the review! Try (https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cd71e06b265) is green, landing.

> Remove this info() since it is repeating openInspectorForURL()

Done (also removed from the two similar test files browser_markup_css_completion_style_attribute_01 & 02)

Comment 15

[Julian Descottes [:jdescottes]]

https://reviewboard.mozilla.org/r/55454/#review54680

> Can we also add a bit more detail on this comment as well?

Done

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/a48604c3b495
inspector: trigger autocomplete if nextchar is not a word character;r=gl

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Julien, Gabriel, are you going to ask for an uplift to fix 48 and 49?
Thanks

Comment 19

[Julian Descottes [:jdescottes]]

No uplift will be requested, marking as wontfix for 48 and 49.