934695 - Autocomplete "!important" and multiple css values

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Typing sucks. Let's make it so we have to do less of it.

We should autocomplete "!important" in css rules.

Comment 1

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

Hi Nick, I started looking at this bug. Hoping to figure out my way around the source code on my own this time. I will let you know if I have any questions.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Sweet!

Optimizer (cc'ing) did the initial css autocompletion, if you have questions you can ask him. I honestly know absolutely nothing about this part of the code.

Comment 3

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

Narrowed it down to inspector/selector-search.js#showSuggestions

Comment 4

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

Had better luck debugging autocomplete-popup.js and inplace-editor.js

Thanks for pointer, Nick!

Comment 5

[Girish Sharma [:Optimizer]]

Let me know if you need any help Gabriel!

Comment 6

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

Attachment: 934695.patch

Main idea:
- Check if the last character of the query is "!", and autocomplete for !important
- startCheckQuery is set to "" so it autocompletes

I think I might need to put the check inside when the content type has mixed css, but I don't know when the content type is considered mixed css. So, I have held off on that addition.
--- a/browser/devtools/shared/inplace-editor.js
+++ b/browser/devtools/shared/inplace-editor.js
@@ -999,17 +999,22 @@ InplaceEditor.prototype = {
>      } else if (this.contentType == CONTENT_TYPES.CSS_MIXED &&
>                 /^\s*style\s*=/.test(query)) { 

Thanks in advance for the review, Optimizer!

Comment 7

[Girish Sharma [:Optimizer]]

Comment on attachment 827894 [details] [diff] [review]
934695.patch

Review of attachment 827894 [details] [diff] [review]:
-----------------------------------------------------------------

looking good. Thanks Gabriel. Comments below.
You also need to update the test to test the "1important" case.

::: browser/devtools/shared/inplace-editor.js
@@ +1002,5 @@
>        let list = [];
>        if (this.contentType == CONTENT_TYPES.CSS_PROPERTY) {
>          list = CSSPropertyList;
>        } else if (this.contentType == CONTENT_TYPES.CSS_VALUE) {
> +        if (query.charAt(query.length - 1) === "!") {

What about when the user has typed "!im" or something in between of "!" and the whole word "!important" ?
How about we always add "!important" in the list in case of CSS_VALUE at the very beginning of the list array. That way, the existing logic itself will take care of everything ?

Comment 8

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

Attachment: 934695.patch

Updated with feedback:
- Added "!important" to the front of the list.
- Updated startCheckQuery to fetch the relevant query to autocomplete. For example, color: red !|, where | is the start of the input selection, then the relevant start query is "!". Consequence of this is that we can autocomplete multiple css values.
- Added checks to make sure startCheckQuery is not the empty string.

Unresolved behaviour that might be a separate bug:
If the css value is "red" and I move my input selection to "r|ed", and continue typing "e", then the autocomplete will fill in "reded".

Comment 9

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

Still need to add test cases. I won't get to them till the end of the week due to school work.

Comment 10

[Girish Sharma [:Optimizer]]

(In reply to Gabriel L (:gluong) from comment #8)
> Unresolved behaviour that might be a separate bug:
> If the css value is "red" and I move my input selection to "r|ed", and
> continue typing "e", then the autocomplete will fill in "reded".

bug 913955

Comment 11

[Girish Sharma [:Optimizer]]

Comment on attachment 828198 [details] [diff] [review]
934695.patch

Review of attachment 828198 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking good.
I think the same changes should be done in the CSS_MIXED else if block when we are in value completion (see a similar line of 
list = domUtils.getCSSValuesForProperty(propertyName);
there too.

Comments below:

::: browser/devtools/shared/inplace-editor.js
@@ +1006,5 @@
> +        // Get the latest start query
> +        let lastSpace = query.lastIndexOf(" ");
> +        if (lastSpace !== -1) {
> +          startCheckQuery = query.slice(lastSpace + 1, query.length);
> +        }

How about :
/([^\s,.\/]+$)/.exec(query.slice(0, input.selectionStart)[0]
?
This will split on even more things that can give added benifits :)
Also, change the comment to something like :
// Get the last query to be completed before the caret.

@@ +1011,2 @@
>          list = domUtils.getCSSValuesForProperty(this.property.name);
> +        list.unshift("!important");

better way :
list = ["!important;", ...domUtils.getCSSValuesForProperty(this.property.name)]

(I am adding the ';' (semicolon) here too, as people qould most likely want to close the input/ type ; after typing '!important'

@@ +1032,5 @@
>            }
>          }
>        }
>        list.some(item => {
> +        if (startCheckQuery !== "" && item.startsWith(startCheckQuery)) {

better way:
if (!startCheckQuery && ...)

Comment 12

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

Thanks for the feedback Girish. I will revise my patch when I get the chance.

Comment 13

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

Attachment: 934695.patch

Updates from previous patch:
- Revised comment on getting the last query before the caret
- Added test cases in style inspector and mixed css value autocompletion in markup view
- Adjusted the regex for CSS_MIXED content type to match the entire css value string, which is later broken into the last query before the caret.
- Note, "!important;" is autocompleted only to the CSS_MIXED content type since semi-colon is added automatically in the style inspector.

Thank you for the feedback and help on completing this bug Girish!

Comment 14

[Girish Sharma [:Optimizer]]

try : https://tbpl.mozilla.org/?tree=Try&rev=5e73700eab86

Comment 15

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

Attachment: 934695.patch

Noticed this in the try:
>Console message: [JavaScript Warning: "Error in parsing value for 'display'.  Declaration dropped." {file: "data:text/html,<html></html>" line: 0 column: 0 source: "blue"}]
which was dropping the priority "!important" for the css value. The original test case was working locally, but it seems my local and try checks the expected value at different times. In this case, try would check the expected value after the priority was dropped.

Reworked the test case for browser_bug894376_css_value_completion_existing_property_value_pair.js.

Comment 16

[Girish Sharma [:Optimizer]]

new try : https://tbpl.mozilla.org/?tree=Try&rev=af736207c82a

Comment 17

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

Attachment: 934695.patch with new commit message

I edited the patch commit message to reflect the changes made in the patch.

Comment 18

[Girish Sharma [:Optimizer]]

Comment on attachment 831281 [details] [diff] [review]
934695.patch with new commit message

Review of attachment 831281 [details] [diff] [review]:
-----------------------------------------------------------------

Looks all good. I will get a formal r+ from some real peer ;) and then land this.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/12ddd0365725

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/12ddd0365725