[webkit-reviews] review denied: [Bug 177313] Web Inspector: Styles Redesign: hook up autocompletion to property names and values : [Attachment 323487] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Oct 12 11:27:36 PDT 2017


Joseph Pecoraro <joepeck at webkit.org> has denied Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 177313: Web Inspector: Styles Redesign: hook up autocompletion to property
names and values
https://bugs.webkit.org/show_bug.cgi?id=177313

Attachment 323487: Patch

https://bugs.webkit.org/attachment.cgi?id=323487&action=review




--- Comment #13 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 323487
  --> https://bugs.webkit.org/attachment.cgi?id=323487
Patch

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

r- for the few smaller issues identified in screenshots and comments. Address
them and this should be good to land and addressing other issues as follow-ups.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
css:49
> +    margin-right: 3px;

Based on the issue screenshot I attached maybe this should be 4px on the right?
It was unbalanced with the left.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:154
> +    set _suggestionHint(value)

I really don't like getters/setters with leading underscores. I think its
better to just make it a public.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:228
> +    _handleKeyDownForSuggestionView(event)

This appears to break ⇧⌥➞ or selecting a word to the right since ⇧⌥← still
works.

Probably early on in _handleKeyDownForSuggestionView, or per-case, you may want
to bail if either event.shiftKey or event.altKey are active.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:359
> +	   let match = prefix.match(/[a-z()-]+$/i);

0-9 are allowed in property names:

    <style>
    :root { --color1--something: blue; }
    body { background: var(--color1--something); }
    </style>
    <body></body>

Numbers just can't be the first character but we don't have to be precise here.
It seems there are visible issues right now with numbers in property names,
should be easy to fix by allowing numbers here.

Technically a lot more is allowed (for unicode variable names):

    nonascii		[\240-\377]
    unicode		\\{h}{1,6}(\r\n|[ \t\r\n\f])?
    escape		{unicode}|\\[^\r\n\f0-9a-f]
    nmstart		[_a-z]|{nonascii}|{escape}
    nmchar		[_a-z0-9-]|{nonascii}|{escape}
    ident		-?{nmstart}{nmchar}*


More information about the webkit-reviews mailing list