Closed Bug 894376 Opened 7 years ago Closed 7 years ago

Autocomplete CSS values in Style Inspector

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 25

People

(Reporter: Optimizer, Assigned: Optimizer)

References

Details

(Whiteboard: [ruleview])

Attachments

(1 file, 7 obsolete files)

This bug handles the autocompletion of values in the style inspector.
Assignee: nobody → scrapmachines
Status: NEW → ASSIGNED
Depends on: 893965
Attached patch patch v0.1 (obsolete) — Splinter Review
This also works. Needs tests.
Attachment #777317 - Flags: feedback?(fayearthur)
Depends on: 895076
(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.
Attached patch patch v1.0 (obsolete) — Splinter Review
Fixes the previous issue that I mentioned. Also, popup suggestions.

Needs tests.
Attachment #777317 - Attachment is obsolete: true
Attachment #777317 - Flags: feedback?(fayearthur)
Attachment #777929 - Flags: review?(fayearthur)
Summary: Autocomplete CSS values in Style Inspector → Autocomplete CSS property names in Style Inspector
This one is for values!
Summary: Autocomplete CSS property names in Style Inspector → Autocomplete CSS values in Style Inspector
Blocks: 896181
Attached patch Patch v2 with tests (obsolete) — Splinter Review
Adds tests.

Please can someone pass it through try? (no net connectivity)
Attachment #777929 - Attachment is obsolete: true
Attachment #777929 - Flags: review?(fayearthur)
Attachment #779329 - Flags: review?(fayearthur)
(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
so my tests need to be updated as bug 895076 got fixed
Attached patch patch v2.0 (obsolete) — Splinter Review
Fixes the tests.
Attachment #779329 - Attachment is obsolete: true
Attachment #779329 - Flags: review?(fayearthur)
Attachment #780585 - Flags: review?(mratcliffe)
Attached patch patch v2.1 (obsolete) — Splinter Review
just a small && to if() conversion.
Attachment #780585 - Attachment is obsolete: true
Attachment #780585 - Flags: review?(mratcliffe)
Attachment #780590 - Flags: review?(mratcliffe)
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)) {
Attachment #780590 - Flags: review?(mratcliffe) → review+
(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.
Attached patch rebased patch v2.1 (obsolete) — Splinter Review
rebased on top of 893965 and addressed review comments.

try push : https://tbpl.mozilla.org/?tree=Try&rev=8af93aa97b87
Attachment #780590 - Attachment is obsolete: true
Attachment #781289 - Flags: review+
Some oranges ... I will look into them today.
Depends on: 898455
Attached patch patch v3.0 (obsolete) — Splinter Review
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
Attachment #781289 - Attachment is obsolete: true
Attachment #782284 - Flags: review?(mratcliffe)
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.
Attachment #782284 - Flags: review?(mratcliffe)
(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 on attachment 782284 [details] [diff] [review]
patch v3.0

In that case I will review it now.
Attachment #782284 - Flags: review?(mratcliffe)
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.
Attachment #782284 - Flags: review?(mratcliffe) → review+
hrmm. Weird, my ST2 should have removed any trailing spaces :(
There is a plugin for ST2 & 3 called TrailingSpaces that highlights all trailing spaces and makes it easier to avoid them.
Ideally there is a setting in ST2 to automatically remove trailing spaces on file save.. Anyways, I will update the patch before landing.
landed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/ea93ffd484cf
Whiteboard: [ruleview] → [ruleview][fixed-in-fx-team]
Patch that landed
Attachment #782284 - Attachment is obsolete: true
Attachment #782642 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ea93ffd484cf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [ruleview][fixed-in-fx-team] → [ruleview]
Target Milestone: --- → Firefox 25
Duplicate of this bug: 895618
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.