Bug 38448 – Web Inspector: Should Autocomplete Style Properties

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

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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

[Joseph Pecoraro]

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.

[Pavel Feldman]

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.

[Joseph Pecoraro]

Good point, thanks for the review!

[Joseph Pecoraro]

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).

[Timothy Hatcher]

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.

[Timothy Hatcher]

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.

[Joseph Pecoraro]

(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?

[Timothy Hatcher]

It would be nice to have a way to get all the properties (shorthands and all) from the inspector host.

[Timothy Hatcher]

We could use that for the syntax highlighter too.

[Joseph Pecoraro]

(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)

[Pavel Feldman]

> 
>   - 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.

[Joseph Pecoraro]

> 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!)

[Joseph Pecoraro]

> 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

[WebKit Review Bot]

http://trac.webkit.org/changeset/61506 might have broken GTK Linux 32-bit Release, GTK Linux 64-bit Release, and Qt Linux Release

[Nikita Vasilyev]

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

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Rolled out in r61507
r61507 = dc59d8744308cdc41b57e3aaf96b26f58e4fe4ef (refs/remotes/trunk)
http://trac.webkit.org/changeset/61507

I'm going to take Nikita's suggestion!

[Joseph Pecoraro]

I am not working on this for the time being. Someone else can jump on this.

[Rob Colburn]

Old bug, appears complete on Chrome.