Bug 17374 – Web Inspector: auto-completion for CSS property names in Styles pane

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Adam Roben (:aroben)]

The Inspector should support tab completion while editing CSS. It could tab-complete both properties and keywords. We could even intelligently tab-complete values (e.g., when a length is needed, we could cycle through "em" "ex" "px" "%" etc.)

[Adam Roben (:aroben)]

<rdar://problem/5745643>

[Joseph Pecoraro]

I see someone is interested in working on this (on Twitter). I'm going to put some pointers here to help them out. You don't necessarily have to follow my advice, but just so you aren't lost in the code!

1. The list of all CSS Properties that WebKit supports can be found in ".in" files spread throughout WebKit's source. You want a similar list in the inspector to use for auto-completion. Some approaches are:

  - You can dynamically compute this list when the inspector launches
    (using something like getComputedStyle(document.body). This is a
    very future proof approach, so as new properties are added, they
    may automatically be available to autocompletion.
  
  - Use an existing computed list of all properties. There is such a
    list available in SourceCSSTokenizer.js [1]. It appears that this
    list contains some differences to the getComputedStyle approach.
    I've attached a diff of comparatively sorted lists.
  
  - Some combination of the above two options!

2. For the autocompletion you want to be as consistent as possible. The Console already includes autocompletion, and the way it does it has already been factored out to a generic class. Minor changes may be needed. See TextPrompt.js [2] and how it is used in ConsoleView.js [3].

3. Editing style properties happens on WebInspector.StylePropertyTreeElement in StylesSidebarPane.js [4]. It uses inspector.js' [5] generic editing. It might be useful to add a hook for an autocompletion controller/handler in this.

4. This ahead! Autocompletion is useful all over the inspector. Style Properties, style values, style selectors, element tag names / attributes! Editing of all of these is currently possible, and is all done the same way (through WebInspector.startEditing). Approach this with a solution that can be replicated for all of the above cases.


[1]: http://trac.webkit.org/browser/trunk/WebCore/inspector/front-end/SourceCSSTokenizer.js#L48
[2]: http://trac.webkit.org/browser/trunk/WebCore/inspector/front-end/TextPrompt.js
[3]: http://trac.webkit.org/browser/trunk/WebCore/inspector/front-end/ConsoleView.js#L334
[4]: http://trac.webkit.org/browser/trunk/WebCore/inspector/front-end/StylesSidebarPane.js#L1283
[5]: http://trac.webkit.org/browser/trunk/WebCore/inspector/front-end/inspector.js#L1816

[Joseph Pecoraro]

Created attachment 53063 [details]
Differing Properties Between getComputedStyle and CSS Tokenizer's list

Here is the differing list. Ideally we would have a list containing the best of both (red and green).

[Timothy Hatcher]

getComputedStyle does not return shorthands, so that is the majority of the differences.

[Nikita Vasilyev]

What about CSS value keywords? I see array of them in SourceCSSTokenizer.js [1], but I don't think it's a good data structure to me. I need to know somehow, what "hidden" belongs to "overflow", "no-repeat" part of "background" and so on. 

Furthermore, "background" property needs something more, than just flat list of keyword. This code is borrowed from Firebug [2]:

background = ["bgRepeat", "bgAttachment", "bgPosition", "color", "systemColor", "none"]

"bgRepeat": [
        "repeat",
        "repeat-x",
        "repeat-y",
        "no-repeat"]
"bgAttachment": [
        "scroll",
        "fixed"]
...


[1]: http://trac.webkit.org/browser/trunk/WebCore/inspector/front-end/SourceCSSTokenizer.js#L100
[2]: http://code.google.com/p/fbug/source/browse/trunk/content/firebug/lib.js#4091

[Joseph Pecoraro]

I don't know the best solution or this problem. I agree something like that would be awesome to have. Its mostly just a large list of things to keep track of, and to be calculated. I thought that would be something great to do in a follow up patch (keep individual patches small and managable). But its great you're thinking ahead!

If you give it a shot, we can comment on it once you put up a patch. I think a big pre-compiled hash table (which it looks like you were showing) would be acceptable. That might be a lot of duplication of strings, so maybe instead of duplicating the strings have them be the indexes into the big list of values. (Although you would need some way to keep these two in sync if you use indexes).

[Timothy Hatcher]

I agree you only want to complete values for the properties they make sense for.

Firebug's approch is neat since the value can expand into more values. They seem to look up the edited property, then expands all the values in the array that also have value lists. If a value does not have a list it ise used as-is and not further expanding is done. Firebug's solution is obviously elegant and less duplication of lists for shorthand properties.

[Joseph Pecoraro]

Be aware that Bug 37582 "Web Inspector: Console: Shift-Tab does not cycle autocompletions in the reverse order" was just implemented. This may affect you slightly since that change added a new parameter (reverse) to some of the completion functions. You may want to update your checkout when that patch lands (it hasn't yet) to make merging easier and so you can take advantage of the changes!

[Joseph Pecoraro]

Nikita, how are things going on this? I'm pretty interested in seeing this getting added. Do you have a work in progress you could put up?

[Nikita Vasilyev]

I didn't do much. I was experimenting with two different types of autocomplete suggestions http://elv1s.ru/files/web-inspector/
 
Right now I'm trying to hook up autocomplete to Inspector with no luck. 
WebInspector.StylePropertyTreeElement.prototype.editingKeyDown looks like the way to go. It even has note "// FIXME: this should cycle through known keywords for the current property name.". The problem is, it never fires for me while I'm editing CSS property.

[Joseph Pecoraro]

(In reply to comment #10)
> I didn't do much. I was experimenting with two different types of autocomplete
> suggestions http://elv1s.ru/files/web-inspector/

Neat! Great work so far.


> Right now I'm trying to hook up autocomplete to Inspector with no luck. 
> WebInspector.StylePropertyTreeElement.prototype.editingKeyDown looks like the
> way to go. It even has note "// FIXME: this should cycle through known keywords
> for the current property name.". The problem is, it never fires for me while
> I'm editing CSS property.

Yes, I think you're in the right spot. Right now it doesn't reach that code because it returns early. Right at the start of the function it breaks unless the key was up/down/pageup/pagedown:

>    editingKeyDown: function(event)
>    {
>        var arrowKeyPressed = (event.keyIdentifier === "Up" || event.keyIdentifier === "Down");
>        var pageKeyPressed = (event.keyIdentifier === "PageUp" || event.keyIdentifier === "PageDown");
>        if (!arrowKeyPressed && !pageKeyPressed)
>            return;

If you want to do autocompletion on key down (like your demo) you will want to call your own function here, or remove the break and handle things later. Or you could add your own keyup listener.

Your demo is nice. It does a setTimeout on keydown so that when given function runs it will have access to the complete text. I don't think there are any problems with this approach, because of JavaScript's nature. The current editKeyDown code uses ranges to find the current word the cursor is in. That might be useful as well. You'll also want to know if you are before / after the initial ":" meaning you're editing the property / value and so you'll need to know more anyways.

Keep up the great work!

[Nikita Vasilyev]

(In reply to comment #11)

> Yes, I think you're in the right spot. Right now it doesn't reach that code
> because it returns early. Right at the start of the function it breaks unless
> the key was up/down/pageup/pagedown:
> 
> >    editingKeyDown: function(event)
> >    {
> >        var arrowKeyPressed = (event.keyIdentifier === "Up" || event.keyIdentifier === "Down");
> >        var pageKeyPressed = (event.keyIdentifier === "PageUp" || event.keyIdentifier === "PageDown");
> >        if (!arrowKeyPressed && !pageKeyPressed)
> >            return;


Actually, it returns even earlier:

    WebInspector.documentKeyDown = function(event)
    {
    
        if (WebInspector.isEditingAnyField())
          return;

Seems like WebInspector.StylePropertyTreeElement.prototype.editingKeyDown never fires at all, which at least strange.

[Joseph Pecoraro]

Hmm, it was working for me. But, that could explain why up/down have been broken sometimes.

[Nikita Vasilyev]

Up/down doesn't work for me on WebKit r58391. I suppose, it was broken by http://trac.webkit.org/changeset/57909/trunk/WebCore/inspector/front-end/inspector.js#file0

[Joseph Pecoraro]

(In reply to comment #14)
> Up/down doesn't work for me on WebKit r58391. I suppose, it was broken by
> http://trac.webkit.org/changeset/57909/trunk/WebCore/inspector/front-end/inspector.js#file0

Looks like someone just opened a bug on this:
https://bugs.webkit.org/show_bug.cgi?id=38325

[Nikita Vasilyev]

I've made autocomplete for CSS properties. http://github.com/NV/web-inspector/commit/fdb02923ac52d . Sometimes it stops working. I've a problem with markup:

    <li title="font-size: 12px" class="selected">
      <span class="name">font-size</span>:
      <span class="value">12px</span>;
    </li>

While <li> is contentEditable, <span class="name"> might be deleted. If it happens, error occurs:

    StylesSidebarPane.js:1358
    TypeError: Result of expression 'name' [null] is not an object.


I'll keep developing in http://github.com/NV/web-inspector/tree/css_autocomplete , until I've got a solid solution.

[Joseph Pecoraro]

> While <li> is contentEditable, <span class="name"> might be deleted. If it
> happens, error occurs:

I'd have to look into this deeper if you want a good comment. I think this might be expected and you could just handle that with an if statement =)


(In reply to comment #16)
> I'll keep developing in
> http://github.com/NV/web-inspector/tree/css_autocomplete , until I've got a
> solid solution.

Very Nice!! I'll put some comments here (and some in GitHub). I think you're heading in the right direction.

There isn't anything particularly wrong with your patch, but I feel like I should raise some concerns so you can provide a high quality patch. WebKit is very keen on keeping a consistent style, and the Web Inspector has had an exceptional record in this regard. Here are some style issues that would should address.

> properties: (function getCSSProperties(){
Space between () and {. There are a few of these in your patch.

> +        // Add shorthands
> +    // convert array-like object to array
Comments should be sentences starting with a capital and ending in a period.

> + var s = ...
> + var j = ...
> + var prop = ...
Use full names except in rare cases.  It really helps readability and readers like me!

> +                var prop = s.slice(0, j).join('-');
Unless there is a really good reason, always use double quoted strings ("-").

> +                if (typeof document.documentElement.style[prop] !== 'undefined' && properties.indexOf(prop) < 0) {
> +                    properties.push(prop);
> +                }
Single line if statements should not have braces. There are a few of these in your patch.

> +        return prop.indexOf(str) === 0
Add a semicolon. I think there are few exceptions to the semicolon rule.

> if (!str) return '';
Don't single line this. Make it two lines and add a blank line after it.

> +    for (var i=0; i<this.length; i++) {
Should be => for (var i = 0; i < this.length; ++i) {

> \ No newline at end of file
We don't like that!

> +                var new_property = WebInspector.CSS.properties.firstStartsWith(property);
We prefer camelCase => newProperty

> if (new_property != property) {
Unless there is a really good reason, always use the strict equality comparisons (=== and !==)

> +Array.convert = function convert(list)
> + ...
> +};
Here would be an exception, I don't think we normally put the semicolon here. 


You extend a number of core types:

    KeyboardEvent.prototype.character (getter)
    Text.prototype.select
    Array.convert
    Array.prototype.last (getter/setter)

I think some of these are nice but unnecessary. For instance the use of Array.convert in your patch is mostly just extra processing. I'll hold off on commenting on the others. I'm a fan, but we typically hold off on this.

[Nikita Vasilyev]

(In reply to comment #17)

> > While <li> is contentEditable, <span class="name"> might be deleted. If it
> > happens, error occurs:
> 
> I'd have to look into this deeper if you want a good comment. I think this
> might be expected and you could just handle that with an if statement =)

Okay, if <span> is missing I'll just add it.


> > properties: (function getCSSProperties(){
> Space between () and {. There are a few of these in your patch.

Line break before { isn't necessary, right? (I hate it!)


> You extend a number of core types:
> 
>     KeyboardEvent.prototype.character (getter)
>     Text.prototype.select
>     Array.convert
>     Array.prototype.last (getter/setter)

Array.* is just a syntax sugar, but KeyboardEvent.prototype.character and Text.prototype.select is really useful in my opinion.

[Nikita Vasilyev]

Addition to comment #16:

<div contenteditable>
    <span class="name">font-size</span>:
</div>

Firefox: I delete all text, then empty span still remains.
WebKit: I delete all text, then span got deleted.

I've been looking for Firefox behavior in WebKit, but I haven't found it. So, I've made dirty check via regexp http://github.com/NV/web-inspector/commit/c010093cfad

[Joseph Pecoraro]

> Firefox: I delete all text, then empty span still remains.
> WebKit: I delete all text, then span got deleted.

I can't replicate this behavior right now. Is this due to your changes? Do you think you have enough to put a patch up on Bugzilla? Also a Jing video / screenshots would be awesome! =)

[Nikita Vasilyev]

Created attachment 55551 [details]
Autocomplete CSS properties

Patch adds completion to CSS properties.

Next things I would like to do:
1) autocomplete CSS values (inside <span class="value">)
2) up/down — cycle through previous/next values

[Nikita Vasilyev]

(In reply to comment #20)
> > Firefox: I delete all text, then empty span still remains.
> > WebKit: I delete all text, then span got deleted.
> 
> I can't replicate this behavior right now.

This is what I meant: http://elv1s.ru/files/js/contentEditable.html

[Joseph Pecoraro]

Comment on attachment 55551 [details]
Autocomplete CSS properties

I just applied the patch locally and it wasn't working. I suspect
this may be an issue with the recent style panel changes. I
apologize for looking at this so late. I will comment on a few
things. Again, the patch looks great overall. Mostly just some
style issues need fixing, and to make this work again with ToT.

Also, the next patch that you put up, set the "review" flag
to be "?". That marks it for review and it won't get lost =)


> +++ b/WebCore/ChangeLog

Use `prepare-ChangeLog --bug #####`, in this case use the number
is 17374 (the bugzilla number) to generate the ChangeLog entry.
I see you're using git, and probably across multiple commits and
I know there are some options for that. However, your ChangeLog is
fine, it would be nice to have more detail though. Examples I
like are:
http://trac.webkit.org/changeset/59443 (a recently landed patch)
http://trac.webkit.org/changeset/59372 (shamelessly my own)


> +++ b/WebCore/inspector/front-end/CSS.js

I like the idea of a new file, but I think maybe we can make this
a little more generic. Maybe Completion.js so that in the future
we can prepare for HTML/DOM attribute completions as well (when editing
DOM attributes). I think much of the code could be shared between
these.


> @@ -0,0 +1,37 @@
> +WebInspector.CSS = {
> +    properties: (function getCSSProperties() {

`get`CSSProperties doesn't seem accurate. Maybe "build"
or "generate" might be better. We may be looking into
backend solutions for this, but I think your solution
right now is great =).

> +        // Add shorthands.

Since there is a clever trick in the code below this might
want to point that out. Something like:

  // Add shorthands to the end of the properties list.


> +      if (typeof document.documentElement.style[shorthand] !== "undefined" && properties.indexOf(shorthand) < 0) {
> +          properties.push(shorthand);
> +      }

Single statement if's shouldn't have braces.


> +WebInspector.CSS.properties.startsWith = function startsWith(str)
> +{
> +    return this.filter(function(property) {
> +        return property.indexOf(str) === 0;
> +    });
> +};

It might be worth looking into sorting the list, and more quickly
finding the appropriate matches. For a list of ~220 this isn't too
bad but I could see this getting out of hand. For starters, using
a for loop instead of a filter would let you break early. Sorting
would let you do a binary search. This could be improved in the
future.


> +    if (!str)
> +        return "";

Add Newline here.

> +    for (var i = 0; i < this.length; ++i) {
> +        if (this[i].indexOf(str) === 0) {
> +            return this[i];
> +        }
> +    }

if statement doesn't need braces. And Web Inspector code tends to
go with the for loop then not needing braces either.


> +        if (arrowKeyPressed || pageKeyPressed && matches && matches.length) {

Make this clearer so we don't have to remember our || and && ordering:

  if ((arrowKeyPressed || pageKeyPressed) && matches && matches.length) {



> +                        } else {
> +                            return;
> +                        }

No braces for the else. Also, this might read better
as an early bail. It would reduce the nesting and that
tends to be the Web Inspector's usual style.



> +                }

Add newline here.


> +        var name = element.querySelector(".name");
> +        var value = element.querySelector(".value");

I prefer nameElement and valueElement as they point out its
an element. Just name and value sound too much like strings.


> +                if (newProperty && newProperty !== property) {
> +                    name.textContent = newProperty;
> +                }

> +                if (n < 0) {
> +                    name.firstChild.select(n);
> +                }

Remove Braces.

>          } else {
>              // FIXME: this should cycle through known keywords for the current property name.
>              return;

It looks like this could probably be removed. Or the
FIXME could be updated to say we need to autocomplete
the values now.


> +    if (start < 0) {
> +        start = end + start;
> +    }

No Braces.

[Joseph Pecoraro]

> > I can't replicate this behavior right now.
> 
> This is what I meant: http://elv1s.ru/files/js/contentEditable.html

Oh! Interesting. I'll ask one of the contenteditable folks about this tomorrow.

[Nikita Vasilyev]

(In reply to comment #23)
> Single statement if's shouldn't have braces.
> ...

Would be nice to have a formating script like http://jsbeautifier.org/ , which does code styling properly where it possible.


> I like the idea of a new file, but I think maybe we can make this
> a little more generic. Maybe Completion.js so that in the future
> we can prepare for HTML/DOM attribute completions as well (when editing
> DOM attributes). I think much of the code could be shared between
> these.

I thought about
    Array.prototype.startsWith(str)
    Array.prototype.firstStartsWith(str)
, but I have a feeling you guys won't like it.

[Nikita Vasilyev]

Created attachment 56082 [details]
Autocomplete CSS properties

It should work now.

I hope all minor issues, if there is any of them, will be fixed in the *next* patch, not this one.

[Timothy Hatcher]

Comment on attachment 56082 [details]
Autocomplete CSS properties

WebCore/inspector/front-end/CSS.js:13
 +                  
Remove this empty line.

WebCore/inspector/front-end/CSS.js:22
 +      return this.filter(function(property) {
What is filter?

WebCore/inspector/front-end/StylesSidebarPane.js:1409
 +          } else if (/[A-Z-]/.test(event.character)) {
Shouldn't this be /[a-zA-Z-]/ or /[a-z-]/i?

WebCore/inspector/front-end/StylesSidebarPane.js:1410
 +              setTimeout(function() {
You should name this function and not inline it in the setTimeout call since it is so long.

There is another approch this could take, more like TextPrompt. TextPrompt calls autoCompleteSoon after some key events (it dosen't care if it was a character either), then autoCompleteSoon schedules a timeout that checks the text content.

It is similar to what you are doing here, but you schedule a new timeout after each keydown. TextPrompt coalesces them, so if you type fast it only autocompletes once.

Why can't this share code with TextPrompt or just use it?

r- because you should cancel previous timeouts at a minimum, for fast typers.

[Nikita Vasilyev]

(In reply to comment #27)
> WebCore/inspector/front-end/CSS.js:22
>  +      return this.filter(function(property) {
> What is filter?

filter is an Array method. 
https://developer.mozilla.org/En/Core_JavaScript_1.5_Reference/Objects/Array/filter
 

> WebCore/inspector/front-end/StylesSidebarPane.js:1409
>  +          } else if (/[A-Z-]/.test(event.character)) {
> Shouldn't this be /[a-zA-Z-]/ or /[a-z-]/i?

keydown event always returns uppercase characters. 
http://elv1s.ru/files/js/key-code-detector.html

[Nikita Vasilyev]

I've made an autocomplete, which doesn't use setTimeout at all, and, more importantly, doesn't flicker!
By flickering I mean this http://screenr.com/Wpp

http://elv1s.ru/files/web-inspector/fancy-autocomplete.html

I would like to rewrite TextPrompt to use this approach. Guys, what do you think about it?

[Joseph Pecoraro]

(In reply to comment #29)
> I've made an autocomplete, which doesn't use setTimeout at all, and, more importantly, doesn't flicker!
> By flickering I mean this http://screenr.com/Wpp
> 
> http://elv1s.ru/files/web-inspector/fancy-autocomplete.html
> 
> I would like to rewrite TextPrompt to use this approach. Guys, what do you think about it?

Neat. I think you should experiment with changing TextPrompt.
I wonder how usable this will be for the console autocompletion,
but we won't know unless we try! I think it feels great, but working
with a static list of values is a lot different then asynchronously
eval'ing a string and sending values across Page barriers (potentially
process barriers for Chrome).

[Timothy Hatcher]

Yep, the reason Joe mentions are why we are not synchronous. (We use to be.)

[Joseph Pecoraro]

(In reply to comment #29)
> I've made an autocomplete, which doesn't use setTimeout at all,
> and, more importantly, doesn't flicker!
> By flickering I mean this http://screenr.com/Wpp
> http://elv1s.ru/files/web-inspector/fancy-autocomplete.html

I just applied the most recent patch locally. I think its great, but
after seeing and thinking about your non-flickering, non-setTimeout
approach, I think that approach is worth taking. Further, I think it
would be fine to take that approach without needing to rewrite
TextPrompt.

The reason I feel this is because:
  - the console completion must be async (results come from the inspected page)
  - the CSS completion is a static list of known values

We can optimize TextPrompt in the future to try to perform
the non-flicker autocompletion. It can do this by doing some
parsing of the string (as is already done) and determining if
the last set of results can be reused (no-flicker), or if a new
list must be fetched (flicker).

Still, its great to do small patches, so I think step 1, non-flicker
CSS autocompletion would be awesome. Think you can create
a new patch for this?

[Nikita Vasilyev]

Created attachment 58907 [details]
Input fields: two vs one

What do you think about two input fields (one for CSS property name, another for value) instead of one? Firebug using two and I've found it more useful. 

To write "margin: auto" in Firebug I can:
1) type "m", "margin" will be suggested
2) press Tab
3) type "auto"

In Web Inspector Tab jumps on next CSS property.

To do the same In Web Inspector I should:
1) type "m", "margin" will be suggested (using my latest patch)
2) press Right Arrow
3) type ":"
4) type "auto"

I think pressing Tab is much easier than pressing Right Arrow and writing ":" after that.

[Joseph Pecoraro]

(In reply to comment #33)
> I think pressing Tab is much easier than pressing Right Arrow and writing ":" after that.

Some thoughts I had. You can append ": " to the end of each
autocompletion result. I'm trying to do something similar
elsewhere. Or manually append it when the user uses the right arrow.
But I agree. I tripped myself up using "tab" a few times with your
autocomplete patch. Splitting into two fields sounds like a
separate patch altogether, or are you suggesting doing that first?

[Nikita Vasilyev]

> (In reply to comment #33)
> Splitting into two fields sounds like a
> separate patch altogether, or are you suggesting doing that first?

No, I don't. I just looked at it, and it looks like more than a 2 hour task.

[Nikita Vasilyev]

Created attachment 59141 [details]
Autocomplete CSS properties and cycle through them using arrow keys

[Nikita Vasilyev]

Created attachment 59166 [details]
Autocomplete CSS properties and cycle through them using arrow keys

Remove keypress event then editing ended. Use binary search for WebInspector.CSS.properties.firstStartsWith.

[Joseph Pecoraro]

Comment on attachment 59166 [details]
Autocomplete CSS properties and cycle through them using arrow keys

> diff --git a/WebCore/inspector/front-end/CSS.js b/WebCore/inspector/front-end/CSS.js

I still don't like this file being called CSS.js. Its purpose
is more about autocompletion. I still like "Completion.js"
or something along those lines.

Also, when you add a file you should update a number of build files.
You can see the following for an example of most of them:
http://trac.webkit.org/changeset/60530

  - [easy] WebCore/inspector/front-end/WebKit.qrc
  - [easy] WebCore/WebCore.gypi
  - [optional, if you have Windows handy] WebCore/WebCore.vcproj/WebCore.vcproj
  - [optional, if you have XCode handy] WebCore/WebCore.xcodeproj/project.pbxproj


> +WebInspector.CSS = {
> +    properties: (function buildCSSProperties() {
> +        var properties = Array.convert(document.defaultView.getComputedStyle(document.documentElement, ""));
> +        var length = properties.length;
> +        // Add shorthands to the end of the properties list.
> +        for (var i = 0; i < length; ++i) {
> +            var propertyWords = properties[i].split("-");
> +            var j = propertyWords.length;
> +            while (--j) {
> +                var shorthand = propertyWords.slice(0, j).join("-");
> +                if (typeof document.documentElement.style[shorthand] !== "undefined" && properties.indexOf(shorthand) < 0)
> +                    properties.push(shorthand);
> +            }
> +        }
> +        return properties.sort();
> +    })()
> +}

The implementation of this function is okay But it can
be improved quite a bit. I played around and implemented
a version twice a fast. However, this already takes ~2ms
so its not really that important to improve. Some simple
things I think would be worth doing that keep the
readability while not going overboard are:

  - cache `document.documentElement.style` into a variable
    and remove the typeof. Resulting in:

    // Early on...
    var style = document.documentElement.style;

    // In the condition...
    style[shorthand] !== undefined

  - change the "slice().join()" call to a "pop(); join()"

    propertyWords.pop();
    var shorthand = propertyWords.join("-");

These few changes along should really speed things up
without affecting the readability. Other improvements
are mentioned here:
http://gist.github.com/444648

Ultimately I think the engine should provide this list,
but that is beyond the scope of this bug.

Also, after some further thought, I think this should
be an unnamed anonymous function. Giving it a name
makes it an unnecessary global.



> +WebInspector.CSS.properties.startsWith = function startsWith(prefix)
> +{
> +    // FIXME: Use binary search.
> +    return this.filter(function(property) {
> +        return property.indexOf(prefix) === 0;
> +    });
> +}

With a bit of refactoring, this could just use the binary search
you implemented in firstStartsWith. And, doing so you can achieve
a pretty quick ~20x speedup because of the nature of the data.
Again, not something particularly necessary to speedup, but since
you already have it!

Make a helper function, like firstStartsWith, that returns the
index of the first starting item (and -1 if not found). Then
its trivial to implement firstStartsWith and startsWith on top
of the binary search implementation.

To see what I mean:
http://gist.github.com/444674


> +        var middleIndex = (maxIndex + minIndex) >> 1; // Same as Math.floor((maxIndex + minIndex) / 2), but twice faster.

You can remove this comment, or just shorten
it to note that this handles the flooring.


> +    index = (index + this.length + shift) % this.length;
> +
> +    if (!prefix)
> +        return this[index];

The top statement can go inside the if block. Its not
necessary otherwise.


> \ No newline at end of file

We want that newline!


> +        // Restore <span class="webkit-css-property"> if it doesn't yet exist or was actidentally deleted.

Typo: actidentally => accidentally


> +Array.convert = function convert(list)
> +{
> +    // Cast array-like object to an array.
> +    return Array.prototype.slice.call(list);
> +}
> +

Again, I think this should be unnamed to prevent an
extra global reference. I know this helps JSC find the
function name in debugging, but I don't think its
that big of a deal in this case. Also, the engine
should be improved to infer the name. I think v8 does.



I hope this review didn't come across as bad. I really
liked the patch. I just think it could use a little more
polish! Cheers!

[Nikita Vasilyev]

(In reply to comment #38)
> (From update of attachment 59166 [details])
> > diff --git a/WebCore/inspector/front-end/CSS.js b/WebCore/inspector/front-end/CSS.js
> 
> I still don't like this file being called CSS.js. Its purpose
> is more about autocompletion. I still like "Completion.js"
> or something along those lines.

This file provides WebInspector.CSS, so I called it CSS.js. It's about CSS completions, not JS completions or something else.


> > +Array.convert = function convert(list)
> > +{
> > +    // Cast array-like object to an array.
> > +    return Array.prototype.slice.call(list);
> > +}
> > +
> 
> Again, I think this should be unnamed to prevent an
> extra global reference. I know this helps JSC find the
> function name in debugging, but I don't think its
> that big of a deal in this case. Also, the engine
> should be improved to infer the name. I think v8 does.

It isn't global for me and it shouldn't be global for you too. If it is, it's a bug.

[Timothy Hatcher]

I agree CSS.js is to generic. Call it CSSCompletions.js or something more specific.

[Nikita Vasilyev]

(In reply to comment #40)
> I agree CSS.js is to generic. Call it CSSCompletions.js or something more specific.

CSSCompletions.js sounds better than Completion.js to me. Should I also rename WebInspector.CSS object to WebInspector.CSSCompletions?

[Joseph Pecoraro]

(In reply to comment #41)
> (In reply to comment #40)
> > I agree CSS.js is to generic. Call it CSSCompletions.js or something more specific.
> 
> CSSCompletions.js sounds better than Completion.js to me.
> Should I also rename WebInspector.CSS object to
> WebInspector.CSSCompletions?

Yes. In fact you can change WebInspector.CSS.properties to
WebInspector.CSSCompletions to shorten things further.

[Nikita Vasilyev]

Created attachment 59191 [details]
Autocomplete CSS properties and cycle through them using arrow keys

[Joseph Pecoraro]

Comment on attachment 59191 [details]
Autocomplete CSS properties and cycle through them using arrow keys

I mentioned in IRC:

> WebCore/ChangeLog
> + 2010-06-19  Nikita Vasilyev  <[e-mail]>
> + 
> +         Reviewed by NOBODY (OOPS!).
> + 
> +         Web Inspector: added autocompletion for CSS properties. A suggestion for a property shows when you type.
> +         You can also cycle through known property names using arrow keys.
> +         https://bugs.webkit.org/show_bug.cgi?id=17374

The Bug Title & Link should be on their own. Your
description should be in a paragraph after. Like so:

            Inspector should support tab completion while editing CSS
            https://bugs.webkit.org/show_bug.cgi?id=17374
            
            Added autocompletion for CSS properties. A suggestion for a property
            shows when you type. You can also cycle through known property names
            using the Up and Down arrow keys.

[Nikita Vasilyev]

Created attachment 59206 [details]
Autocomplete CSS properties and cycle through them using arrow keys

Do not delete last character if selection is collapsed.

[Joseph Pecoraro]

Comment on attachment 59206 [details]
Autocomplete CSS properties and cycle through them using arrow keys

Great!

One thing we talked about in IRC was that this doesn't autocomplete
on backspace. This acts like the autocomplete in Safari's URL bar,
but not like the Inspector's JavaScript Console. This can be handled
in a follow up bug.

All inspector and http/tests/inspector* tests passed.

[WebKit Commit Bot]

Comment on attachment 59206 [details]
Autocomplete CSS properties and cycle through them using arrow keys

Clearing flags on attachment: 59206

Committed r61514: <http://trac.webkit.org/changeset/61514>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.