Bug 52212 - Web Inspector: Employ TextPrompt for CSS property name/value autocompletion
Summary: Web Inspector: Employ TextPrompt for CSS property name/value autocompletion
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexander Pavlov (apavlov)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-11 07:58 PST by Alexander Pavlov (apavlov)
Modified: 2011-01-19 05:26 PST (History)
10 users (show)

See Also:


Attachments
[PATCH] Suggested solution (42.48 KB, patch)
2011-01-13 09:39 PST, Alexander Pavlov (apavlov)
yurys: review-
Details | Formatted Diff | Diff
[PATCH] Comments addressed (42.65 KB, patch)
2011-01-14 07:10 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[PATCH] Inherited TextPrompt to override key handlers (45.02 KB, patch)
2011-01-14 08:51 PST, Alexander Pavlov (apavlov)
pfeldman: review-
Details | Formatted Diff | Diff
[PATCH] Comments from pfeldman addressed (39.65 KB, patch)
2011-01-17 03:04 PST, Alexander Pavlov (apavlov)
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2011-01-11 07:58:52 PST
TextPrompt features text autocompletion, which is a great feature we could use for autocompleting CSS property names/values instead of writing ad-hoc autocompletions.
Comment 1 Alexander Pavlov (apavlov) 2011-01-13 09:39:55 PST
Created attachment 78819 [details]
[PATCH] Suggested solution
Comment 2 Yury Semikhatsky 2011-01-14 06:02:43 PST
Comment on attachment 78819 [details]
[PATCH] Suggested solution

View in context: https://bugs.webkit.org/attachment.cgi?id=78819&action=review

> Source/WebCore/inspector/front-end/CSSKeywordCompletions.js:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

2011

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1630
> +                    return { keyIdentifier: "___", handled: false };

___ looks weird, can you use null or undefined instead?

> Source/WebCore/inspector/front-end/TextPrompt.js:76
> +        if (this._customKeyDownHandler) {

Let's extract Up, Down and Tab handlers into their own methods to allow overriding them. r- for this.

> Source/WebCore/inspector/front-end/TextPrompt.js:400
> +    _tabKeyPressed: function(reverse)

This method can be inlined.
Comment 3 Alexander Pavlov (apavlov) 2011-01-14 07:10:26 PST
Created attachment 78939 [details]
[PATCH] Comments addressed

Introduced a customKeyHandlers parameter into the TextPrompt ctor to pass in custom Up, Down, and Tab handlers.
Comment 4 Alexander Pavlov (apavlov) 2011-01-14 08:51:58 PST
Created attachment 78946 [details]
[PATCH] Inherited TextPrompt to override key handlers
Comment 5 Pavel Feldman 2011-01-17 01:45:48 PST
Comment on attachment 78946 [details]
[PATCH] Inherited TextPrompt to override key handlers

View in context: https://bugs.webkit.org/attachment.cgi?id=78946&action=review

r- is for managing selection in the sidebar, rest seems fine.

> Source/WebCore/inspector/front-end/CSSKeywordCompletions.js:45
> +WebInspector.CSSKeywordCompletions._colors = [

Nit: please compress to ~120 chars per line.

> Source/WebCore/inspector/front-end/CSSKeywordCompletions.js:197
> +WebInspector.CSSKeywordCompletions._colorAwareProperties = [

ditto

> Source/WebCore/inspector/front-end/CSSKeywordCompletions.js:224
> +WebInspector.CSSKeywordCompletions._propertyKeywordMap = {

ditto

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1690
> +            finalSelectionRange.setStart(replacementTextNode, 0);

I would expect all the selection management code to move to the prompt. Why is it still here?

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1902
> +

Nit: blank line.
Comment 6 Alexander Pavlov (apavlov) 2011-01-17 03:03:57 PST
(In reply to comment #5)
> (From update of attachment 78946 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78946&action=review
> 
> r- is for managing selection in the sidebar, rest seems fine.
> 
> > Source/WebCore/inspector/front-end/CSSKeywordCompletions.js:45
> > +WebInspector.CSSKeywordCompletions._colors = [
> 
> Nit: please compress to ~120 chars per line.

Done.

> > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1690
> > +            finalSelectionRange.setStart(replacementTextNode, 0);
> 
> I would expect all the selection management code to move to the prompt. Why is it still here?

I don't understand why you would expect this. I'd rather understand you expect all the _prompt_ management code move there. But scrolling numeric values is a whole different thing, isn't it? It does not match the TextPrompt responsibilities altogether.

> > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1902
> > +
> 
> Nit: blank line.

Done.
Comment 7 Alexander Pavlov (apavlov) 2011-01-17 03:04:32 PST
Created attachment 79146 [details]
[PATCH] Comments from pfeldman addressed
Comment 8 Alexander Pavlov (apavlov) 2011-01-19 05:26:22 PST
Landed with minor fixes: http://trac.webkit.org/changeset/76116