Bug 38448 - Web Inspector: Should Autocomplete Style Properties
Summary: Web Inspector: Should Autocomplete Style Properties
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: Joseph Pecoraro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-02 21:20 PDT by Joseph Pecoraro
Modified: 2014-12-09 10:44 PST (History)
13 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (6.93 KB, patch)
2010-05-02 21:23 PDT, Joseph Pecoraro
pfeldman: review-
Details | Formatted Diff | Diff
[PATCH] Improved Fix (2.79 KB, patch)
2010-05-11 17:13 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2010-05-02 21:20:12 PDT
Steps to Reproduce:

  1. Open the Console
  2. Type: document.body.style.col

Results:
I expected to see "color" autocomplete. Actual results were no autocompletions.
Comment 1 Joseph Pecoraro 2010-05-02 21:23:45 PDT
Created attachment 54905 [details]
[PATCH] Proposed Fix

The generated list in this example came from converting the properties list from the CSS Tokenizer. However, the list can be automatically generated like so:


> // This is the list of properties
> var properties = []
> var keywords = window.getComputedStyle(document.documentElement);
> for (var i = 0, len = keywords.length; i < len; ++i) {
> 
>     // Strip leading hyphen from -vendor properties
>     var property = keywords[i];
>     if (property.charAt(0) === "-")
>         property = property.substring(1);
> 
>     // Turn hypens points into camel case points
>     properties[i] = property.replace(/\-./g, function(c) {
>         return c.charAt(1).toUpperCase();
>     });
> }
> 
> // `properties` contains the results
Comment 2 Joseph Pecoraro 2010-05-02 21:26:05 PDT
I specifically wanted this so I could play with "webkitTransform" and friends =).

I only tested use in the console. I'm not completely sure about other cases just yet. I just wanted some feedback on autogeneration Versus precompiled list before a final patch.
Comment 3 Pavel Feldman 2010-05-05 12:08:14 PDT
Comment on attachment 54905 [details]
[PATCH] Proposed Fix

WebCore/inspector/front-end/InjectedScript.js:133
 +          propertyNames = propertyNames.concat(InjectedScript._hiddenStyleProperties);
Be careful, _getPropertyNames above already has style properties due to your changes to InjectedScript._getPropertyNames.

In fact, if we are talking about autocomplete, you should only modify InjectedScript.getCompletions. Otherwise, style objects start containing weird properties in the object trees.
Comment 4 Joseph Pecoraro 2010-05-05 13:08:22 PDT
Good point, thanks for the review!
Comment 5 Joseph Pecoraro 2010-05-11 17:13:37 PDT
Created attachment 55779 [details]
[PATCH] Improved Fix

- Only happens for autocompletion.
- Automatically generate the list of properties (I can convert this back if desired, but it is fast).
Comment 6 Timothy Hatcher 2010-05-11 19:17:15 PDT
Comment on attachment 55779 [details]
[PATCH] Improved Fix

WebCore/inspector/front-end/InjectedScript.js:261
 +      var jsProperties = [];
properties is a simplier name, js prefix isn't needed.

Consider landing manually and not using CQ this time.
Comment 7 Timothy Hatcher 2010-05-11 19:18:42 PDT
A small note, computed style does not have shorthands and is sometimes missing new properties, since they need to be added to the computed style list to show up.
Comment 8 Joseph Pecoraro 2010-05-11 19:27:17 PDT
(In reply to comment #7)
> A small note, computed style does not have shorthands and is sometimes missing new properties, since they need to be added to the computed style list to show up.

So would you prefer a pre-computed list, to be updated occasionally? Or maybe adding some kind of function that really does return a list of all style properties?
Comment 9 Timothy Hatcher 2010-05-11 19:33:05 PDT
It would be nice to have a way to get all the properties (shorthands and all) from the inspector host.
Comment 10 Timothy Hatcher 2010-05-11 19:33:32 PDT
We could use that for the syntax highlighter too.
Comment 11 Joseph Pecoraro 2010-05-11 19:35:30 PDT
(In reply to comment #9)
> It would be nice to have a way to get all the properties
> (shorthands and all) from the inspector host.

(In reply to comment #10)
> We could use that for the syntax highlighter too.

Yes, that is what I meant. =)

I will investigate this. This would really help keep in sync
the following places:

  - console autocompletion (injected side)
  - styles autocompletion (sidebar)
  - css tokenizer (highlighting)
Comment 12 Pavel Feldman 2010-05-11 21:58:35 PDT
> 
>   - console autocompletion (injected side)
>   - styles autocompletion (sidebar)
>   - css tokenizer (highlighting)

Computed style is missing not only shorthands, but some longhand properties as well. I don't see border radius properties there for example. There is no single place with the CSS property names in the parser we could get them from, so whatever solution is, it would need to refactor some native code.

As a short term solution, we could generate a single white list of properties and use it from within three subsystems you mentioned above. We could push the list from the front-end to the injected script lazily if necessary.
Comment 13 Joseph Pecoraro 2010-05-11 22:46:22 PDT
> As a short term solution, we could generate a single white list of
> properties and use it from within three subsystems you mentioned
> above. We could push the list from the front-end to the injected
> script lazily if necessary.

Should we include support for properties WebKit doesn't actually
understand? Like other browser vendor properties. I don't think we
should, that way we make it clear what is relevant in the browser
running WebKit.

I'm wondering if a backend solution could twist that into a good
thing. For instance if the browser running the Web Inspector has
doesn't support some style properties (say they are hidden behind
an ENABLE flag) than that Web Inspector wouldn't highlight or
autocomplete those properties.

I'm just thinking out loud though. A pre-compiled / generated
list is an easy solution, but I keep thinking its not easy to
maintain. This is really something that I think should be
investigated for a long term solution. 

(TAKE THAT Bugzilla! I can totally enforce my own 80 char wrap!)
Comment 14 Joseph Pecoraro 2010-06-20 10:11:18 PDT
> WebCore/inspector/front-end/InjectedScript.js:261
>  +      var jsProperties = [];
> properties is a simplier name, js prefix isn't needed.

Done.

> I will investigate this. This would really help keep in sync
> the following places:
> 
>   - console autocompletion (injected side)
>   - styles autocompletion (sidebar)
>   - css tokenizer (highlighting)

Filed Bug for this:
https://bugs.webkit.org/show_bug.cgi?id=40886

This would address Pavel's comments as well. For
instance this patch didn't autocomplete "background"
or "borderRadius" but it did longer versions.


Committed r61506
	M	WebCore/ChangeLog
	M	WebCore/inspector/front-end/InjectedScript.js
r61506 = 72b23490dfc195079550fc7c3f17fdee30e8dd3a (refs/remotes/trunk)
http://trac.webkit.org/changeset/61506
Comment 15 WebKit Review Bot 2010-06-20 10:20:35 PDT
http://trac.webkit.org/changeset/61506 might have broken GTK Linux 32-bit Release, GTK Linux 64-bit Release, and Qt Linux Release
Comment 16 Nikita Vasilyev 2010-06-20 11:33:48 PDT
I think WebKit should just enumerate these properties, like all other browsers do. It will make this patch unnecessary.
https://bugs.webkit.org/show_bug.cgi?id=23946
Comment 17 Joseph Pecoraro 2010-06-20 11:43:34 PDT
The following line causes the crash:

  var keywords = window.getComputedStyle(document.documentElement);

Changing window to inspectedWindow, there is still the possibility
that inspectedWindow.document is no longer defined (on navigation)
and this causes the crash.

Nikita bring up a good point. So, I am going to revert this, and
try and tackle the problem from that angle.
Comment 18 Joseph Pecoraro 2010-06-20 11:57:52 PDT
Rolled out in r61507
r61507 = dc59d8744308cdc41b57e3aaf96b26f58e4fe4ef (refs/remotes/trunk)
http://trac.webkit.org/changeset/61507

I'm going to take Nikita's suggestion!
Comment 19 Joseph Pecoraro 2010-07-01 12:46:38 PDT
I am not working on this for the time being. Someone else can jump on this.
Comment 20 Rob Colburn 2012-05-09 09:37:17 PDT
Old bug, appears complete on Chrome.