Bug 41414 - Web Inspector: autocompletion for CSS property values in Styles pane
: Web Inspector: autocompletion for CSS property values in Styles pane
Status: RESOLVED FIXED
: WebKit
Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
: 51332
:
  Show dependency treegraph
 
Reported: 2010-06-30 09:31 PST by
Modified: 2011-01-19 05:29 PST (History)


Attachments
Firebug autocomplete weakness (4.13 KB, image/png)
2010-06-30 18:06 PST, Nikita Vasilyev
no flags Details
Work-in-progress patch (26.06 KB, patch)
2010-07-13 08:22 PST, Nikita Vasilyev
no flags Review Patch | Details | Formatted Diff | Diff
[PATCH] Suggested solution (69.42 KB, patch)
2010-12-17 09:31 PST, Alexander Pavlov (apavlov)
no flags Review Patch | Details | Formatted Diff | Diff
[PATCH] Comments addressed (61.29 KB, patch)
2010-12-21 06:06 PST, Alexander Pavlov (apavlov)
no flags Review Patch | Details | Formatted Diff | Diff
[PATCH] Python style and Chromium non-debug_devtools compilability tentatively fixed (64.85 KB, patch)
2010-12-21 08:02 PST, Alexander Pavlov (apavlov)
no flags Review Patch | Details | Formatted Diff | Diff
[FILE] Generated CSSKeywordCompletions.js (22.32 KB, application/x-javascript)
2010-12-21 08:41 PST, Alexander Pavlov (apavlov)
no flags Details
[PATCH] Comments addressed, color nicknames fixed (66.58 KB, patch)
2010-12-22 05:02 PST, Alexander Pavlov (apavlov)
joepeck: review-
Review Patch | Details | Formatted Diff | Diff
[FILE] Generated CSSKeywordCompletions.js (with patch 77204) (22.09 KB, application/x-javascript)
2010-12-22 05:15 PST, Alexander Pavlov (apavlov)
no flags Details
[PATCH] Comments from joepeck addressed (68.42 KB, patch)
2010-12-24 06:21 PST, Alexander Pavlov (apavlov)
no flags Review Patch | Details | Formatted Diff | Diff
[FILE] Generated CSSKeywordCompletions.js (with patch 77411) (22.11 KB, application/x-javascript)
2010-12-24 06:25 PST, Alexander Pavlov (apavlov)
no flags Details
[PATCH] Fix Chromium compilability (68.61 KB, patch)
2010-12-24 08:53 PST, Alexander Pavlov (apavlov)
no flags Review Patch | Details | Formatted Diff | Diff
[PATCH] An alternative solution suggested by pfeldman. Non-generated CSSKeywordCompletions.js (which is to be updated manually) (45.22 KB, patch)
2010-12-29 08:30 PST, Alexander Pavlov (apavlov)
no flags Review Patch | Details | Formatted Diff | Diff
[PATCH] Alternative solution with rebaselined test (46.26 KB, patch)
2010-12-29 09:15 PST, Alexander Pavlov (apavlov)
pfeldman: review-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-06-30 09:31:03 PST
For instance, Inspector should autocomplete left/right/none for "float".

This issue is related to:
https://bugs.webkit.org/show_bug.cgi?id=17374
https://bugs.webkit.org/show_bug.cgi?id=38448
https://bugs.webkit.org/show_bug.cgi?id=40886
------- Comment #1 From 2010-06-30 09:34:59 PST -------
I can't assign it to myself, so I'm just saying I'm working on it.
------- Comment #2 From 2010-06-30 18:06:45 PST -------
Created an attachment (id=60177) [details]
Firebug autocomplete weakness

Let's talk about API and how to implement it. My suggestion:
WebInspector.CSSCompletions.getValuesByName(name, [prefix=""])


There are 3 examples:

WebInspector.CSSCompletions.getValueByName("float", "r")
-> "right"

WebInspector.CSSCompletions.getValueByName("background", "url(image.png) 40% no-re")
-> "url(image.png) 40% no-repeat"

WebInspector.CSSCompletions.getValueByName("-webkit-transition", "color 0.5s l")
-> "color 0.5s linear"


1. float.

WebInspector.CSSCompletions.values = {
    float: ["left", "right", "none"]
}

WebInspector.CSSCompletions.getValueByName = function(name, prefix)
{
    for (var i = 0; i < this.values[name].length; ++i)
        if (this.values[name][i].indexOf(prefix) === 0)
            return this.values[name][i];
    return prefix;
}

This one is works well.

Actual implementation may use binary search. It's not the point here.


2. background

WebInspector.CSSCompletions.values = {
    background: [
        "repeat-x", "repeat-y", "no-repeat", 
        "left", "right", "top", "bottom", "center",
        "white", "red", "yellow", ...all color names,
        "fixed", "scroll",
        ...
    ]
}

WebInspector.CSSCompletions.getValueByName = function(name, prefix)
{
    var splitted = prefix.split(" ");
    prefix = splitted.splice(splitted.length - 1, 1);
    for (var i = 0; i < this.values[name].length; ++i)
        if (this.values[name][i].indexOf(prefix) === 0) {
            splitted.push(this.values[name][i]);
            return splitted.join(" ");
        }
    return prefix;
}

This works for WebInspector.CSSCompletions.getValueByName("background", "url(image.png) 40% no-re"). I assume, this is what Firebug does. It has a downside, see attached firebug-autocomplete.png.

"background" shorthand actually has a pretty complicated syntax: http://www.w3.org/TR/css3-background/#the-background


3. -webkit-transition

WebInspector.CSSCompletions.getValueByName("-webkit-transition", "color 0.5s l")
-> "color 0.5s linear"

Using previous approach, it will give you "color 0.5 left" instead, because "left" is a CSS property like "color"! Not good.

So, we need some grammar logic! We can do better than Firebug, right? Any suggestions how that logic will look like?
------- Comment #3 From 2010-07-01 00:31:09 PST -------
(In reply to comment #2)
> Created an attachment (id=60177) [details] [details]
> Firebug autocomplete weakness
> 
> Let's talk about API and how to implement it. My suggestion:
> WebInspector.CSSCompletions.getValuesByName(name, [prefix=""])
> 
> 
> There are 3 examples:
> 
> WebInspector.CSSCompletions.getValueByName("float", "r")
> -> "right"
> 

I agree that it makes sense to try implementing synchronous api. There already is a WebInspector.CSSStyleModel class that should get the new method. The method itself should probably have "getCompletions(name, prefix)" signature. So same arguments, different name.

> So, we need some grammar logic! We can do better than Firebug, right? Any suggestions how that logic will look like?

I'd start with hinting the names. Joe, do you plan to finish https://bugs.webkit.org/show_bug.cgi?id=40886? Starting on something and then dropping it makes others stay away from the problem thinking that it is being taken care of. Let us know.

Talking about the values for the properties, as you suggest, i'd use groups of values per property name (potentially referencing sub-groups to re-use stuff). It is already very useful.

As we go on, we can add mutually exclusive groups for property names so that "color" and "left" fell into one and fixed the issue you are talking about. I don't think we should deal with parsing for now - it is an overkill. I'd be interested in your thoughts on it though.
------- Comment #4 From 2010-07-01 04:58:39 PST -------
(In reply to comment #3)
> As we go on, we can add mutually exclusive groups for property names so that "color" and "left" fell into one and fixed the issue you are talking about. I don't think we should deal with parsing for now - it is an overkill. I'd be interested in your thoughts on it though.

It isn't clear to me how to implement this "mutually exclusive groups". So, I agree, let's start with groups of values per property name.
------- Comment #5 From 2010-07-01 09:58:54 PST -------
> Joe, do you plan to finish https://bugs.webkit.org/show_bug.cgi?id=40886? Starting on something and then dropping it makes others stay away from the problem thinking that it is being taken care of. Let us know.

Yep, I'll be making time to put up versions of my patches for the above
and another one I think I was working on. Sorry about the inactivity,
I have just been super busy! I'll focus on Bugzilla today.

I think I only stopped working on one (for the time being) because I
know I won't get around to it. I'll comment on that bug.
------- Comment #6 From 2010-07-02 09:02:16 PST -------
(In reply to comment #3)
> I agree that it makes sense to try implementing synchronous api. There already is a WebInspector.CSSStyleModel class that should get the new method. The method itself should probably have "getCompletions(name, prefix)" signature. So same arguments, different name.

Is getCompletions(name, prefix) gonna be a back-end solution? If so, I won't bother myself with it and just wait until it's done :)
------- Comment #7 From 2010-07-02 09:10:12 PST -------
> Is getCompletions(name, prefix) gonna be a back-end solution? If so, I won't bother myself with it and just wait until it's done :)

I'd implement it on the front-end side!
------- Comment #8 From 2010-07-13 08:22:06 PST -------
Created an attachment (id=61380) [details]
Work-in-progress patch

http://screenr.com/mtR (there is a debug colors)

I took WebInspector.CSSStyleModel.properties and WebInspector.CSSStyleModel.keywords from FireBug (just removed -moz- properties). There are no -webkit- properties. It's all hardcoded.

I would like to hear about UI bugs and the code in general. I don't need code style review yet.
------- Comment #9 From 2010-07-13 12:35:36 PST -------
I have a hash of all CSS values: WebInspector.CSSStyleModel.keywordMap. It looks like:

{
  background: Array (185)
    0: "repeat"
    1: "repeat-x"
    2: "repeat-y"
    ...
    152: "Yellow"
    ...
    184: "none"
  ...
  float: Array (3)
    0: "left"
    1: "right"
    2: "none"
  ...
}

Now, I lazily build it within WebInspector.CSSStyleModel.getCSSKeywordsByProperty method. It builds from WebInspector.CSSStyleModel.properties and WebInspector.CSSStyleModel.keywords.

We need to keep it up to date! How we going to do that?
------- Comment #10 From 2010-07-14 05:29:35 PST -------
(From update of attachment 61380 [details])
WebCore/inspector/front-end/utilities.js:378
 +  Element.prototype.select = function select(start, end)
Please see Node.prototype.rangeBoundaryForOffset. Does it do pretty much the same thing?
------- Comment #11 From 2010-07-14 06:37:01 PST -------
(In reply to comment #10)
> (From update of attachment 61380 [details] [details])
> WebCore/inspector/front-end/utilities.js:378
>  +  Element.prototype.select = function select(start, end)
> Please see Node.prototype.rangeBoundaryForOffset. Does it do pretty much the same thing?

It works, but instead of one line:

    element.select(startOffset, endOffset);

I have to use ten:

    var start = element.rangeBoundaryForOffset(startOffset)
    var end = element.rangeBoundaryForOffset(endOffset)
    var selection = getSelection()
    if (selection.rangeCount) {
        var range = selection.getRangeAt(0);
        range.setStart(start.container, start.offset)
        range.setEnd(end.container, end.offset)
        selection.removeAllRanges()
        selection.addRange(range)
    }

I prefer one line :)
------- Comment #12 From 2010-07-14 07:21:29 PST -------
> It works, but instead of one line:
> 
>     element.select(startOffset, endOffset);
> 
> I have to use ten:
> 
>     var start = element.rangeBoundaryForOffset(startOffset)
>     var end = element.rangeBoundaryForOffset(endOffset)
>     var selection = getSelection()
>     if (selection.rangeCount) {
>         var range = selection.getRangeAt(0);
>         range.setStart(start.container, start.offset)
>         range.setEnd(end.container, end.offset)
>         selection.removeAllRanges()
>         selection.addRange(range)
>     }
> 

All I ask is that you implement it on top of rangeBoundaryForOffset. That way you add 10 lines, not 60 to the utilities.
------- Comment #13 From 2010-07-14 10:49:18 PST -------
(In reply to comment #12)

Okay, I will use rangeBoundaryForOffset.

By the way, I have QUnit tests[1] for Element.prototype.select and Text.prototype.select. I would like to know how to port it to Inspector's tests.

[1]: http://nv.github.com/select-text.js/test/
------- Comment #14 From 2010-07-19 04:43:06 PST -------
(In reply to comment #9)
> We need to keep it up to date! How we going to do that?

I've found WebCore/css/CSSValueKeywords.in. I wonder if WebInspector.CSSStyleModel.keywordMap could be generated from that on back-end. Can it?
------- Comment #15 From 2010-07-19 09:45:54 PST -------
(In reply to comment #14)
> I've found WebCore/css/CSSValueKeywords.in. I wonder if
> WebInspector.CSSStyleModel.keywordMap could be generated
> from that on back-end. Can it?

Hmm, it looks like the CSSValueKeywords.in file does have the idea
of relationships between properties and values. You could find the
script that parsers it, to see if you could generate something from it.
I think its doable.
------- Comment #16 From 2010-07-30 05:29:23 PST -------
I wonder how WebKit does relationships between CSS values and keywords? There is a lot of inconsistencies in CSSValueKeywords.in.

`-webkit-box-pack: start` is a valid property, but there is no "CSS_PROP__WEBKIT_BOX_PACK" in CSSValueKeywords.in. There is a "CSS_PROP_BOX_PACK", but it's commented out:

    # CSS_PROP_BOX_PACK
    # start
    # end
    # center
    # justify

Most groups start with "CSS_PROP_", but some don't:

    #
    # background-size
    #
    contain
    cover

Also, there is one strange group:

    # Unordered rest
    #
    a3
    a4
    a5
    above
    absolute
    always
    ...
------- Comment #17 From 2010-07-30 09:42:51 PST -------
Hmm, then maybe that file isn't completely accurate or up to date in that way.
It looks likes its only use is to generate CSSValueKeywords.{c,h} in DerivedSources.
The script that processes it is WebCore/css/makevalues.pl, and it just ignores
all comments.
------- Comment #18 From 2010-12-17 09:31:13 PST -------
Created an attachment (id=76887) [details]
[PATCH] Suggested solution

As there has been no progress on this bug for quite a time, I take liberty to implement the feature with auto-generated lists of keywords for every supported property
------- Comment #19 From 2010-12-17 09:35:31 PST -------
Attachment 76887 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7255016
------- Comment #20 From 2010-12-17 10:18:46 PST -------
(From update of attachment 76887 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=76887&action=review

> As there has been no progress on this bug for quite a time, I take liberty to
> implement the feature with auto-generated lists of keywords for every
> supported property

Nicely done. I like the approach. Some high level comments after looking at the patch:

  • Whenever you attach a patch with a new generated file, please also attach a copy
     of the generated file, so we can see what it looks like!

  • You generate a .js file for the property -> values map. This is an excellent approach.
     I think you should also add the complete property list to this generated file and
     completely remove the backend fetch approach:
     InspectorBackend.getSupportedCSSProperties

  • If you could break this up into smaller patches it might be easier to review. Ex.
     - Refactoring CSSCompletions from a singleton to a "class", so we can then see
     your changes to it, if any, after the refactoring.

I'll have some "quick" review comments.


> WebCore/css/makevalues.pl:49
> +open KEYWORDMAP, ">CSSKeywordCompletions.js" || die "Could not open CSSKeywordMap.js for writing";

Nit. I think "CSSKeywordMap.js" was probably an older name choice for the file? Update to
match CSSKeywordCompletions.js.


> WebCore/inspector/front-end/StylesSidebarPane.js:1627
> +                // WebCore crash workaround: we cannot remove the range right away, since
> +                // if there is no suggest, WebCore/editing will try to remove the same range
> +                // once again, as event.preventDefault() will not have been called.

Is there a bug filed for this?


> WebCore/inspector/front-end/inspector.js:1763
> +    config = config || {};
>      element.__editing = true;
>      WebInspector.__editing = true;
> +    var committedCallback = config.commitHandler;
> +    var cancelledCallback = config.cancelHandler;
>  
>      config = config || {};
>      var committedCallback = config.commitHandler;

Below it looks like some (maybe all) of these lines already exist. Maybe a merge conflict?
But it looks like these changes won't be needed.
------- Comment #21 From 2010-12-17 11:21:09 PST -------
Attachment 76887 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7224019
------- Comment #22 From 2010-12-21 05:21:32 PST -------
(In reply to comment #20)
> (From update of attachment 76887 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76887&action=review
> 
> > As there has been no progress on this bug for quite a time, I take liberty to
> > implement the feature with auto-generated lists of keywords for every
> > supported property
> 
> Nicely done. I like the approach. Some high level comments after looking at the patch:
> 
>   • Whenever you attach a patch with a new generated file, please also attach a copy
>      of the generated file, so we can see what it looks like!

Will do

>   • You generate a .js file for the property -> values map. This is an excellent approach.
>      I think you should also add the complete property list to this generated file and
>      completely remove the backend fetch approach:
>      InspectorBackend.getSupportedCSSProperties

That's what I'm eventually going to do, but I since property names and keywords are processed by different Perl scripts, I have no clue how to join the two script outputs into a single file. My current idea is to generate a separate JS file beside (I guess it warrants a separate change).

>   • If you could break this up into smaller patches it might be easier to review. Ex.
>      - Refactoring CSSCompletions from a singleton to a "class", so we can then see
>      your changes to it, if any, after the refactoring.

Done

> I'll have some "quick" review comments.
> 
> 
> > WebCore/css/makevalues.pl:49
> > +open KEYWORDMAP, ">CSSKeywordCompletions.js" || die "Could not open CSSKeywordMap.js for writing";
> 
> Nit. I think "CSSKeywordMap.js" was probably an older name choice for the file? Update to
> match CSSKeywordCompletions.js.

Correct, fixed.

> > WebCore/inspector/front-end/StylesSidebarPane.js:1627
> > +                // WebCore crash workaround: we cannot remove the range right away, since
> > +                // if there is no suggest, WebCore/editing will try to remove the same range
> > +                // once again, as event.preventDefault() will not have been called.
> 
> Is there a bug filed for this?

Bug 51389 filed.

> > WebCore/inspector/front-end/inspector.js:1763
> > +    config = config || {};
> >      element.__editing = true;
> >      WebInspector.__editing = true;
> > +    var committedCallback = config.commitHandler;
> > +    var cancelledCallback = config.cancelHandler;
> >  
> >      config = config || {};
> >      var committedCallback = config.commitHandler;
> 
> Below it looks like some (maybe all) of these lines already exist. Maybe a merge conflict?
> But it looks like these changes won't be needed.

Right, it's a bad merge. File reverted.
------- Comment #23 From 2010-12-21 06:06:04 PST -------
Created an attachment (id=77107) [details]
[PATCH] Comments addressed
------- Comment #24 From 2010-12-21 06:11:26 PST -------
Attachment 77107 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7317066
------- Comment #25 From 2010-12-21 06:56:13 PST -------
Attachment 77107 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7311073
------- Comment #26 From 2010-12-21 08:02:57 PST -------
Created an attachment (id=77115) [details]
[PATCH] Python style and Chromium non-debug_devtools compilability tentatively fixed
------- Comment #27 From 2010-12-21 08:41:34 PST -------
Created an attachment (id=77120) [details]
[FILE] Generated CSSKeywordCompletions.js
------- Comment #28 From 2010-12-21 08:45:33 PST -------
(In reply to comment #20)
>   • Whenever you attach a patch with a new generated file, please also attach a copy
>      of the generated file, so we can see what it looks like!

Done.

>   • If you could break this up into smaller patches it might be easier to review. Ex.
>      - Refactoring CSSCompletions from a singleton to a "class", so we can then see

As of now, I cannot see any changes to extract. Currently, there are two logical parts in this change: (1) generate CSSKeywordCompletions.js on all platforms; (2) Make use of the file in StylesSidebarPane.js. However we don't seem to land unused code, so landing a separate patch for (1) does not look like a good solution. What do you think about it?
------- Comment #29 From 2010-12-21 10:21:29 PST -------
Thanks for addressing my comments!

> >   • If you could break this up into smaller patches it might be easier to review. Ex.
> >      - Refactoring CSSCompletions from a singleton to a "class", so we can then see
> 
> As of now, I cannot see any changes to extract. Currently, there are two logical parts in
> this change: (1) generate CSSKeywordCompletions.js on all platforms; (2) Make use of
> the file in StylesSidebarPane.js. However we don't seem to land unused code, so landing
> a separate patch for (1) does not look like a good solution. What do you think about it?

Its really a personal preference. Your choice to leave them in one patch is fine. Already
removing the CSSCompletions refactoring makes this patch easier to follow now that
I know there were no changes made to its internals!

A couple things I noticed from the generated file:

> var acceptedKeywords = [ "initial" ];

Nit: I don't think we want spaces inside the braces.

>    "box-lines": [
>        "single",
>        "multiple"
>    ],

Would it be useful to sort these value lists at generation time? It would be useful for
me reading the file to see if a property is missing. Currently WebInspector.CSSCompletions
sorts the provided properties, and that is called every time the forProperty method is called.
This is a lot of wasted cycles if it could just be sorted once. Removing the sort in
CSSCompletions would require fixing its other callers, but there is just one in
inspector.js and that list (which will eventually be replaced with a new generated file)
can be sorted before calling the constructor.


>   "_color_": [ ... ]

This is an improved list of colors, since it is auto-generated, over
WebInspector.Color.Nicknames in front-end/Color.js, which I think I just took colors
mentioned in the CSS 2.1 spec. Merging these lists, to eliminate duplicated code,
would be another thing good for future consideration. I think your list had ~40
extra values, nice! Without those colors in the WebInspector.Color list, those colors
don't have a swatch and cannot be cycled since we don't know their corresponding
hex. I don't know how that would work for a color like "-webkit-link", which I
am not sure is always a static color, but we can come up with something eventually.
------- Comment #30 From 2010-12-22 03:26:45 PST -------
(In reply to comment #29)
> Thanks for addressing my comments!
> 
> A couple things I noticed from the generated file:
> 
> > var acceptedKeywords = [ "initial" ];
> 
> Nit: I don't think we want spaces inside the braces.

Fixed.

> >    "box-lines": [
> >        "single",
> >        "multiple"
> >    ],
> 
> Would it be useful to sort these value lists at generation time? It would be useful for
> me reading the file to see if a property is missing. Currently WebInspector.CSSCompletions
> sorts the provided properties, and that is called every time the forProperty method is called.
> This is a lot of wasted cycles if it could just be sorted once. Removing the sort in
> CSSCompletions would require fixing its other callers, but there is just one in
> inspector.js and that list (which will eventually be replaced with a new generated file)
> can be sorted before calling the constructor.

As discussed on IRC, several keyword lists are merged at runtime, so we still need to sort the resulting list.

> >   "_color_": [ ... ]
> 
> This is an improved list of colors, since it is auto-generated, over
> WebInspector.Color.Nicknames in front-end/Color.js, which I think I just took colors
> mentioned in the CSS 2.1 spec. Merging these lists, to eliminate duplicated code,
> would be another thing good for future consideration. I think your list had ~40
> extra values, nice! Without those colors in the WebInspector.Color list, those colors
> don't have a swatch and cannot be cycled since we don't know their corresponding
> hex. I don't know how that would work for a color like "-webkit-link", which I
> am not sure is always a static color, but we can come up with something eventually.

Most of those new keywords are CSS2 system colors which were deprecated in CSS3 (and also CSS2.1). As per http://www.w3.org/TR/css3-color/#css2-system,
"The CSS2 System Color values have been deprecated in favor of the CSS3 UI ‘appearance’ property. If you want to emulate the look of a user interface related element or control, please use the ‘appearance’ property instead of attempting to mimic a user interface element through a combination of system colors."

Thus, I will unmark these system color keywords so that they never appear in the "_color_" category. I have also added the "*grey" color nicknames to your list, since WebCore seems to support both kinds of variations. Do we have any other issues to solve yet?
------- Comment #31 From 2010-12-22 05:02:25 PST -------
Created an attachment (id=77204) [details]
[PATCH] Comments addressed, color nicknames fixed

- Added "*grey" color nicknames to Colors to accompany the "*gray" ones
- Removed the system color keywords, as they have been deprecated in CSS2.1 and CSS3
- Renamed "lightGrey" to "lightGray" in HexTable to keep the "...Gray" spelling consistent across the table
------- Comment #32 From 2010-12-22 05:15:17 PST -------
Created an attachment (id=77205) [details]
[FILE] Generated CSSKeywordCompletions.js (with patch 77204)
------- Comment #33 From 2010-12-23 11:54:41 PST -------
(From update of attachment 77204 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=77204&action=review

Having tried this patch and testing some weird cases, I'm happy with this patch.
I want to get a CSS reviewer a chance to see if they might have any concerns with
the CSSValueKeywords.in changes. Otherwise it looks good to me! I'll grab
a CSS expert.

> LayoutTests/inspector/styles-autocomplete-values-expected.txt:1
> +Tests that Up/Down keys switch suggestions in CSS property values.

Is there an Up/Down test for #s? Could you add a test, or section to this test, to verify #s change
(Up/Down and maybe PageUp/PageDown). I ask because I see at the bottom this touches that code
(maybe it just indents it though).

That is not a requirement for this patch, and could be filed separately.

> WebCore/ChangeLog:1
> +2010-12-22  Alexander Pavlov  <apavlov@chromium.org>

This is pretty sparse for such an interesting change. Please add comments to ChangeLogs!
I know I stopped harping about this in the past... but it really is helpful, especially for reviewers. =)

> WebCore/WebCore.gyp/scripts/action_cssvaluekeywords.py:135
> +            parts = line.partition(',')
> +            if parts[1] != '':
> +                value = parts[0]

Is the parts[1] != '' check really necessary here? Previously it just added all lines. What lines are new such that parts[1] is "" and invalid?
This is not r- material, since I see that it will work, but it seems unnecessary at the moment.

> WebCore/WebCore.pri:603
> +cssKeywordCompletions.wkAddOutputToSources = false
> +cssKeywordCompletions.output = generated/CSSKeywordCompletions.qrc
> +cssKeywordCompletions.input = CSS_KEYWORD_COMPLETIONS_QRC
> +cssKeywordCompletions.tempNames = $$PWD/$$CSS_KEYWORD_COMPLETIONS_QRC $${WC_GENERATED_SOURCES_DIR}/CSSKeywordCompletions.qrc
> +cssKeywordCompletions.commands = $$QMAKE_COPY $$replace(cssKeywordCompletions.tempNames, "/", $$QMAKE_DIR_SEP)
> +addExtraCompiler(cssKeywordCompletions)

1. Other generated sections have a comment, like "# GENERATOR 6-B:". I suggest
adding some kind of comment here.

2. "generated/CSSKeywordCompletions.qrc" should probably use the $${WC_GENERATED_SOURCES_DIR}
variable instead of a hardcoded generated directory. Like you do for tempNames in (line 600).

For the same reasons maybe you can drive by improve the generator for InspectorBackendStub.
Add a comment and use the variable:

    646    inspectorBackendStub.output = generated/InspectorBackendStub.qrc

> WebCore/css/makevalues.pl:38
> +  @parts = split(/,/);

I don't think the parenthesis are needed here. Other perl calls in this file don't use them,
so you can probably drop them.

> WebCore/css/makevalues.pl:80
> + * THIS FILE IS GENERATED BY makevalues.pl. DO NOT MODIFY!

I would suggest giving the full path if "makevalues.pl": WebCore/css/makevalues.pl

However, this runs the risk of becoming stale if the file moves. But it still would make
it easier for people to find the generator.

> WebCore/inspector/front-end/StylesSidebarPane.js:1373
> +        nameElement.normalize();
> +        valueElement.normalize();

Whoa. I just learned about this by reviewing this patch! That really could have saved me
some time in the past. Good solution!
https://developer.mozilla.org/En/DOM/Node.normalize

I'm not sure if normalizing here is the best place, but I don't think its a performance
issue at this point.

> WebCore/inspector/front-end/StylesSidebarPane.js:1596
> +        const styleValueDelimiters = " \u00A0\t\n\"':;,/()";

I don't like that this constant is in two places. Maybe it could be made a constant on the class
or the code for its two uses shared.

> WebCore/inspector/front-end/StylesSidebarPane.js:1627
> +                // WebCore crash workaround: we cannot remove the range right away, since
> +                // if there is no suggest, WebCore/editing will try to remove the same range
> +                // once again, as event.preventDefault() will not have been called.

Please link to the Bugzilla bug you made.
<http://webkit.org/b/51389> Selection becomes stale when CharacterData is manipulated directly

Note there has been a lot of progress and that, and it might be fixed today.

> WebCore/inspector/front-end/StylesSidebarPane.js:1711
> +        if (!!this.cssValueCompletions.firstStartsWith(wordStringLower)) {

Nit: The !! is unnecessary here. The value returned is always a valid string or "".
And "" is a falsely value. Does it make things clearer for you to leave in the !!?

  js> "" && alert(1); // No alert.

> WebCore/inspector/front-end/StylesSidebarPane.js:1776
> +        this.valueElement.firstChild.select(selectionStart, selectionStart + replacementString.length)

Nit: Should have a semicolon.

> WebKit/chromium/ChangeLog:8
> +        * WebKit.gyp:

Could use a comment.

> WebKit/chromium/WebKit.gyp:755
> +                '../../WebCore/WebCore.gyp/WebCore.gyp:webcore_bindings_sources'

Nit: Other dependencies above always add trailing commas. Might be nice to reduce future diffs.

> WebKit/chromium/WebKit.gyp:768
> +                    '<(SHARED_INTERMEDIATE_DIR)/webkit/CSSKeywordCompletions.js'

Nit: Other dependencies above always add trailing commas. Might be nice to reduce future diffs.
------- Comment #34 From 2010-12-23 12:05:03 PST -------
(From update of attachment 77204 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=77204&action=review

> WebCore/ChangeLog:18
> +        * css/CSSValueKeywords.in:

The changes to this file seem to make it hard to read and maintain, while not even helping the css parser.
------- Comment #35 From 2010-12-23 12:21:38 PST -------
(In reply to comment #34)
> (From update of attachment 77204 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77204&action=review
> 
> > WebCore/ChangeLog:18
> > +        * css/CSSValueKeywords.in:
> 
> The changes to this file seem to make it hard to read and maintain, while not
> even helping the css parser.

I agree. It seems like it should be possible to generate the mapping between
values and the properties they apply to by reading in the entire file once, and
generating a map. Then writing out that map after parsing. This way we would
only need to make minimal changes the input file. Challenges:

  • Special case globals (inherit, initial)?
  • Special case colors?
  • Mapping comments like CSS_PROP_OUTLINE_STYLE to their CSS string
  • Detecting when to apply a value to multiple properties or just one. Ex.

    Multiple properties where the values apply. Note that the value list
    does apply to all the properties, not just the last one.

        # CSS_PROP_OUTLINE_STYLE
        # CSS_PROP_BORDER_TOP_STYLE
        # CSS_PROP_BORDER_BOTTOM_STYLE
        # CSS_PROP_BORDER_LEFT_STYLE
        none
        hidden
        inset
        groove
        ridge
        outset
        dotted
        dashed
        solid
        double

    A case like this we would need to reset.

        #
        # CSS_PROP_FONT:
        #
        caption
        icon
        menu
        message-box
        small-caption
        -webkit-mini-control
        -webkit-small-control
        -webkit-control
        status-bar

I will r- based on this, and give you a chance to address these comments.
------- Comment #36 From 2010-12-23 12:22:58 PST -------
@Reviewers. I will be on vacation for a few weeks (returning ~ January 10th) so feel
free to pick up where this was left off. The front-end changes looked good, and
based on the review comments so far they wouldn't change much.
------- Comment #37 From 2010-12-24 05:45:17 PST -------
(In reply to comment #33)
> (From update of attachment 77204 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77204&action=review
> 
> Having tried this patch and testing some weird cases, I'm happy with this patch.
> I want to get a CSS reviewer a chance to see if they might have any concerns with
> the CSSValueKeywords.in changes. Otherwise it looks good to me! I'll grab
> a CSS expert.
> 
> > LayoutTests/inspector/styles-autocomplete-values-expected.txt:1
> > +Tests that Up/Down keys switch suggestions in CSS property values.
> 
> Is there an Up/Down test for #s? Could you add a test, or section to this test, to verify #s change
> (Up/Down and maybe PageUp/PageDown). I ask because I see at the bottom this touches that code
> (maybe it just indents it though).

It's just due to the diff algorithm. In fact, some code has been inserted before this chunk, and for some reason the chunk was deemed modified in full. I did not change anything number-scroll-related.

> > WebCore/ChangeLog:1
> > +2010-12-22  Alexander Pavlov  <apavlov@chromium.org (:apavlov) (c)>
> 
> This is pretty sparse for such an interesting change. Please add comments to ChangeLogs!
> I know I stopped harping about this in the past... but it really is helpful, especially for reviewers. =)

Good, fixed!

> > WebCore/WebCore.gyp/scripts/action_cssvaluekeywords.py:135
> > +            parts = line.partition(',')
> > +            if parts[1] != '':
> > +                value = parts[0]
> 
> Is the parts[1] != '' check really necessary here? Previously it just added all lines. What lines are new such that parts[1] is "" and invalid?
> This is not r- material, since I see that it will work, but it seems unnecessary at the moment.

This was added to account for keywords without related properties (i.e., just "red" instead of "red,_color_"). However I considered such lines invalid for some reason. Fixed.

> > WebCore/WebCore.pri:603
> > +cssKeywordCompletions.wkAddOutputToSources = false
> > +cssKeywordCompletions.output = generated/CSSKeywordCompletions.qrc
> > +cssKeywordCompletions.input = CSS_KEYWORD_COMPLETIONS_QRC
> > +cssKeywordCompletions.tempNames = $$PWD/$$CSS_KEYWORD_COMPLETIONS_QRC $${WC_GENERATED_SOURCES_DIR}/CSSKeywordCompletions.qrc
> > +cssKeywordCompletions.commands = $$QMAKE_COPY $$replace(cssKeywordCompletions.tempNames, "/", $$QMAKE_DIR_SEP)
> > +addExtraCompiler(cssKeywordCompletions)
> 
> 1. Other generated sections have a comment, like "# GENERATOR 6-B:". I suggest
> adding some kind of comment here.

Done.

> 2. "generated/CSSKeywordCompletions.qrc" should probably use the $${WC_GENERATED_SOURCES_DIR}
> variable instead of a hardcoded generated directory. Like you do for tempNames in (line 600).

Done.

> For the same reasons maybe you can drive by improve the generator for InspectorBackendStub.
> Add a comment and use the variable:
> 
>     646    inspectorBackendStub.output = generated/InspectorBackendStub.qrc

Done.

> > WebCore/css/makevalues.pl:38
> > +  @parts = split(/,/);
> 
> I don't think the parenthesis are needed here. Other perl calls in this file don't use them,
> so you can probably drop them.

Done.

> > WebCore/css/makevalues.pl:80
> > + * THIS FILE IS GENERATED BY makevalues.pl. DO NOT MODIFY!
> 
> I would suggest giving the full path if "makevalues.pl": WebCore/css/makevalues.pl
> 
> However, this runs the risk of becoming stale if the file moves. But it still would make
> it easier for people to find the generator.

Done.

> > WebCore/inspector/front-end/StylesSidebarPane.js:1596
> > +        const styleValueDelimiters = " \u00A0\t\n\"':;,/()";
> 
> I don't like that this constant is in two places. Maybe it could be made a constant on the class
> or the code for its two uses shared.

Done.

> > WebCore/inspector/front-end/StylesSidebarPane.js:1627
> > +                // WebCore crash workaround: we cannot remove the range right away, since
> > +                // if there is no suggest, WebCore/editing will try to remove the same range
> > +                // once again, as event.preventDefault() will not have been called.
> 
> Please link to the Bugzilla bug you made.
> <http://webkit.org/b/51389> Selection becomes stale when CharacterData is manipulated directly
> Note there has been a lot of progress and that, and it might be fixed today.

Alas, the patch has landed but the assertion still occurs. Linking to the bug.

> > WebCore/inspector/front-end/StylesSidebarPane.js:1711
> > +        if (!!this.cssValueCompletions.firstStartsWith(wordStringLower)) {
> 
> Nit: The !! is unnecessary here. The value returned is always a valid string or "".
> And "" is a falsely value. Does it make things clearer for you to leave in the !!?
> 
>   js> "" && alert(1); // No alert.

Yes, I find it cleaner to explicitly cast to a boolean. On a side note, this makes perfect sense when assigning the result to a variable. Here it is not necessary, so removing !!.

> > WebCore/inspector/front-end/StylesSidebarPane.js:1776
> > +        this.valueElement.firstChild.select(selectionStart, selectionStart + replacementString.length)
> 
> Nit: Should have a semicolon.

Done.

> > WebKit/chromium/ChangeLog:8
> > +        * WebKit.gyp:
> 
> Could use a comment.

Done.

> > WebKit/chromium/WebKit.gyp:755
> > +                '../../WebCore/WebCore.gyp/WebCore.gyp:webcore_bindings_sources'
> 
> Nit: Other dependencies above always add trailing commas. Might be nice to reduce future diffs.

Done.

> > WebKit/chromium/WebKit.gyp:768
> > +                    '<(SHARED_INTERMEDIATE_DIR)/webkit/CSSKeywordCompletions.js'
> 
> Nit: Other dependencies above always add trailing commas. Might be nice to reduce future diffs.

Done.
------- Comment #38 From 2010-12-24 06:09:19 PST -------
(In reply to comment #35)
> (In reply to comment #34)
> > (From update of attachment 77204 [details] [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=77204&action=review
> > 
> > > WebCore/ChangeLog:18
> > > +        * css/CSSValueKeywords.in:
> > 
> > The changes to this file seem to make it hard to read and maintain, while not
> > even helping the css parser.
> 
> I agree. It seems like it should be possible to generate the mapping between
> values and the properties they apply to by reading in the entire file once, and
> generating a map. Then writing out that map after parsing. This way we would
> only need to make minimal changes the input file. Challenges:
> 
>   • Special case globals (inherit, initial)?
>   • Special case colors?
>   • Mapping comments like CSS_PROP_OUTLINE_STYLE to their CSS string
>   • Detecting when to apply a value to multiple properties or just one. Ex.
>     Multiple properties where the values apply. Note that the value list
>     does apply to all the properties, not just the last one.
> 
>         # CSS_PROP_OUTLINE_STYLE
>         # CSS_PROP_BORDER_TOP_STYLE
>         # CSS_PROP_BORDER_BOTTOM_STYLE
>         # CSS_PROP_BORDER_LEFT_STYLE
>         none
>         hidden
>         inset
>         groove
>         ridge
>         outset
>         dotted
>         dashed
>         solid
>         double
> 
>     A case like this we would need to reset.
> 
>         #
>         # CSS_PROP_FONT:
>         #
>         caption
>         icon
>         menu
>         message-box
>         small-caption
>         -webkit-mini-control
>         -webkit-small-control
>         -webkit-control
>         status-bar
> 
> I will r- based on this, and give you a chance to address these comments.

Unfortunately, there are a few more grave challenges with the suggested approach:
* The file does not have consistent CSS_PROP_* comments for all the values: there are a bunch of unlabelled keywords; there are comments that contain plain property names (text-rendering, -webkit-color-correction, etc.); the comment format is inconsistent (e.g. CSS_PROP_WIDTH/MIN_WIDTH/MAX_WIDTH); comments do not correspond to the actual properties (CSS_PROP_SPEECH -> speak, CSS_PROP_GENERIC_FONT_FAMILY -> font-family)
* The CSS_PROP_* keywords do not take shorthands into account.
* Keywords can occur in the file only once, so it is a real challenge to organize the file structure so as to map 1 keyword -> N properties.
* Some of the CSS_PROP_* comments are misleading/unparsable: CSS_PROP_*_COLOR; CSS_PROP_MASK and CSS_PROP_OPACITY (in SVGCSSValueKeywords.in) correspond to the "accumulate" and "new" keywords that are not valid for the "mask" and "opacity" SVG properties. 

I'm pretty sure this list is not comprehensive, so I decided to hand-write the mappings to avoid [subtle] confusions. A much simpler approach could be to generate the CSSKeywordCompletions.js file once to commit it and rely on manual updates of the mappings. Does this solution sound acceptable? If not, are there any suggestions on how the current format (or the entire approach) can be improved, beyond bare criticism? :)
------- Comment #39 From 2010-12-24 06:21:44 PST -------
Created an attachment (id=77411) [details]
[PATCH] Comments from joepeck addressed

Waiting for suggestions on the CSSKeywordSuggestions.js generation!
------- Comment #40 From 2010-12-24 06:25:57 PST -------
Created an attachment (id=77412) [details]
[FILE] Generated CSSKeywordCompletions.js (with patch 77411)

In the previous comment, I meant to say "CSSKeywordCompletions.js", of course.
------- Comment #41 From 2010-12-24 06:26:42 PST -------
Attachment 77411 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7326168
------- Comment #42 From 2010-12-24 07:10:30 PST -------
Attachment 77411 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7213164
------- Comment #43 From 2010-12-24 08:53:01 PST -------
Created an attachment (id=77416) [details]
[PATCH] Fix Chromium compilability
------- Comment #44 From 2010-12-29 08:30:05 PST -------
Created an attachment (id=77620) [details]
[PATCH] An alternative solution suggested by pfeldman. Non-generated CSSKeywordCompletions.js (which is to be updated manually)
------- Comment #45 From 2010-12-29 09:15:35 PST -------
Created an attachment (id=77622) [details]
[PATCH] Alternative solution with rebaselined test
------- Comment #46 From 2010-12-30 05:30:25 PST -------
(From update of attachment 77622 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=77622&action=review

> WebCore/inspector/front-end/CSSKeywordCompletions.js:1107
> +    "-webkit-text-emphasis-style": [

I'd suggest that we:
- remove \n between values
- group -webkit items

> WebCore/inspector/front-end/StylesSidebarPane.js:1635
> +        var character = event.data.toLowerCase();

You should use TextPrompt instead. It has configurable completions and stopCharacters. You will need to make it a bit more flexible.
------- Comment #47 From 2010-12-30 05:30:47 PST -------
(From update of attachment 77416 [details])
Clearing r? from obsolete patch.
------- Comment #48 From 2011-01-10 22:55:18 PST -------
> Unfortunately, there are a few more grave challenges with the suggested approach:

Arg, you bring up some very good points.


> I'm pretty sure this list is not comprehensive, so I decided to hand-write the mappings
> to avoid [subtle] confusions. A much simpler approach could be to generate the
> CSSKeywordCompletions.js file once to commit it and rely on manual updates of the
> mappings. Does this solution sound acceptable? If not, are there any suggestions on how
> the current format (or the entire approach) can be improved, beyond bare criticism? :)

I don't have any good suggestions. I believe there are other places in the Inspector code
where things can get out of sync between reality (WebCore) and what the Inspector knows
about (not just the tokenizers). So ultimately autogeneration would be best. But, I see your
point, and going back and forth isn't helping get this useful feature into the nightlies.

I'm okay with a patch containing a generated completions file with the intent that it be
updating manually in the future. My immediate suggestions for that type of patch would
be to:

  1. Add a comment into the relevant *.in files along the lines of:

      "When adding new or changing CSS properties / values, please check if the Inspector
       mapping needs an update as well: WebCore/inspector/front-end/CSSKeywordCompletions.js"

  2. Open a new bug specifically about better autogeneration of CSS Properties and
       Values for WebCore and the Inspector to share. Ultimately, autogeneration is
       not necessary for this patch, it is just desired and useful.

       For instance, the generated file you have right now could quite quickly be turned
       into a JSON input file that could potentially replace the existing *.in files. However
       there are probably plenty of concerns that the CSS Experts might have moving to a
       new format or possibly better formats. I'm still not completely familiar with how
       these *.in files are used and maintained.
------- Comment #49 From 2011-01-19 05:29:58 PST -------
The feature was implemented in bug 52212.