894376 - Autocomplete CSS values in Style Inspector

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Girish Sharma [:Optimizer]]

This bug handles the autocompletion of values in the style inspector.

Comment 1

[Girish Sharma [:Optimizer]]

Attachment: patch v0.1

This also works. Needs tests.

Comment 2

[Girish Sharma [:Optimizer]]

(In reply to Girish Sharma [:Optimizer] from comment #1)
> Created attachment 777317 [details] [diff] [review]
> patch v0.1
> 
> This also works. Needs tests.

Well, this does not work in the case when you edit the property first in an already existing property-value pair.

Comment 3

[Girish Sharma [:Optimizer]]

Attachment: patch v1.0

Fixes the previous issue that I mentioned. Also, popup suggestions.

Needs tests.

Comment 4

[Girish Sharma [:Optimizer]]

This one is for values!

Comment 5

[Girish Sharma [:Optimizer]]

Attachment: Patch v2 with tests

Adds tests.

Please can someone pass it through try? (no net connectivity)

Comment 6

[Heather Arthur [:harth]]

(In reply to Girish Sharma [:Optimizer] from comment #5) 
> Please can someone pass it through try? (no net connectivity)

https://tbpl.mozilla.org/?tree=Try&rev=79fbbbf2e1cf

Comment 7

[Girish Sharma [:Optimizer]]

so my tests need to be updated as bug 895076 got fixed

Comment 8

[Girish Sharma [:Optimizer]]

Attachment: patch v2.0

Fixes the tests.

Comment 9

[Girish Sharma [:Optimizer]]

Attachment: patch v2.1

just a small && to if() conversion.

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 780590 [details] [diff] [review]
patch v2.1

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

Great job.

r+ with these two nits addressed. We need a green try before landing this.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=c38bf70fe5a2

::: browser/devtools/styleinspector/test/browser_bug894376_css_value_completion_existing_property_value_pair.js
@@ +75,5 @@
> +  state = index;
> +
> +  info("pressing key " + key + " to get result: [" + testData[index].slice(2) +
> +       "] for state " + state);
> +  if (key.match(/tab/ig)) {

if (/tab/ig.test(key)) {

::: browser/devtools/styleinspector/test/browser_bug894376_css_value_completion_new_property_value_pair.js
@@ +77,5 @@
> +  state = index;
> +
> +  info("pressing key " + key + " to get result: [" + testData[index].slice(2) +
> +       "] for state " + state);
> +  if (key.match(/tab/ig)) {

if (/tab/ig.test(key)) {

Comment 11

[Girish Sharma [:Optimizer]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #10)
> Comment on attachment 780590 [details] [diff] [review]
> patch v2.1
> 
> Review of attachment 780590 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great job.
> 
> r+ with these two nits addressed. We need a green try before landing this.
> 
> Try:
> https://tbpl.mozilla.org/?tree=Try&rev=c38bf70fe5a2
> 
I am myself tackling some debug build only failures. (have pushed many trys henceforth)

Will post the green try along with updated patch when I get it.

Comment 12

[Girish Sharma [:Optimizer]]

Attachment: rebased patch v2.1

rebased on top of 893965 and addressed review comments.

try push : https://tbpl.mozilla.org/?tree=Try&rev=8af93aa97b87

Comment 13

[Girish Sharma [:Optimizer]]

Some oranges ... I will look into them today.

Comment 14

[Girish Sharma [:Optimizer]]

Attachment: patch v3.0

Asking for review again as I have changed a lot of things for the tests to be perfectly green.

Turns out that in the previous approach, there were two setTimeout . One inside the _mayBeSuggest method with delay of 0 and on in the test with a delay of 200ms. Ideally first, the _mayBeSuggest timeout should happen and then the test one, but due to some reasons, in some rare situations, the very first call of _mayBeSuggest's timeout was happening after the call of the timeout in the first iteration of checkState in the test. This was causing intermittents in debug build, all be it very less frequent : https://tbpl.mozilla.org/?tree=Try&rev=40515ff53ee3

Thus I removed all the timeouts in the tests and now am relying on events. So the inline-editor emits "after-suggest" event telling that it has completed autocompletion related tasks. This time, try was green : https://tbpl.mozilla.org/?tree=Try&rev=cd4742cc046b

Comment 15

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 782284 [details] [diff] [review]
patch v3.0

There are a huge amount of oranges here. Even though they don't seem to be caused by your patch we need to be sure.

Please pull, update and submit this patch to try again. I will review it when it is green.

Comment 16

[Girish Sharma [:Optimizer]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #15)
> Comment on attachment 782284 [details] [diff] [review]
> patch v3.0
> 
> There are a huge amount of oranges here. Even though they don't seem to be
> caused by your patch we need to be sure.
> 
> Please pull, update and submit this patch to try again. I will review it
> when it is green.

Those oranges are existing oranges, still not fixed.. see bug 851349

I have personally looked into the logs and my tests run correctly.

Comment 17

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 782284 [details] [diff] [review]
patch v3.0

In that case I will review it now.

Comment 18

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 782284 [details] [diff] [review]
patch v3.0

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

I am much happier with this.

The number of trailing spaces in your patch appears to be growing... r+ if you remove them:

::: browser/devtools/shared/inplace-editor.js
@@ +800,5 @@
>  
>        let input = this.input;
>  
>        this._apply();
> + 

I hate myself but please remove the trailing space.

::: browser/devtools/styleinspector/test/browser_bug893965_css_property_completion_existing_property.js
@@ +102,5 @@
> +        editor.input.removeEventListener("keypress", onKeypress);
> +      }
> +      info("inside event listener");
> +      checkState();
> +    }) 

I hate myself but please remove the trailing space.

::: browser/devtools/styleinspector/test/browser_bug893965_css_property_completion_new_property.js
@@ +88,5 @@
> +        editor.input.removeEventListener("keypress", onKeypress);
> +      }
> +      info("inside event listener");
> +      checkState();
> +    }) 

I hate myself but please remove the trailing space.

::: browser/devtools/styleinspector/test/browser_bug894376_css_value_completion_existing_property_value_pair.js
@@ +90,5 @@
> +        editor.input.removeEventListener("keypress", onKeypress);
> +      }
> +      info("inside event listener");
> +      checkState();
> +    }) 

I hate myself but please remove the trailing space.

::: browser/devtools/styleinspector/test/browser_bug894376_css_value_completion_new_property_value_pair.js
@@ +90,5 @@
> +        editor.input.removeEventListener("keypress", onKeypress);
> +      }
> +      info("inside event listener");
> +      checkState();
> +    }) 

I hate myself but please remove the trailing space.

Comment 19

[Girish Sharma [:Optimizer]]

hrmm. Weird, my ST2 should have removed any trailing spaces :(

Comment 20

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

There is a plugin for ST2 & 3 called TrailingSpaces that highlights all trailing spaces and makes it easier to avoid them.

Comment 21

[Girish Sharma [:Optimizer]]

Ideally there is a setting in ST2 to automatically remove trailing spaces on file save.. Anyways, I will update the patch before landing.

Comment 22

[Girish Sharma [:Optimizer]]

landed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/ea93ffd484cf

Comment 23

[Girish Sharma [:Optimizer]]

Attachment: patch that landed

Patch that landed

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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