[webkit-reviews] review denied: [Bug 177012] Web Inspector: Styles Redesign: support editing of rule selectors : [Attachment 321902] Patch

[bugzilla-daemon at webkit.org]

Matt Baker <mattbaker at apple.com> has denied Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 177012: Web Inspector: Styles Redesign: support editing of rule selectors
https://bugs.webkit.org/show_bug.cgi?id=177012

Attachment 321902: Patch

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




--- Comment #6 from Matt Baker <mattbaker at apple.com> ---
Comment on attachment 321902
  --> https://bugs.webkit.org/attachment.cgi?id=321902
Patch

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

This looks really good. Applied patch locally and it works really well, even
with selectors that wrap to multiple lines.

Other than a couple comments, I think two things need to be address before
landing, so r- for now:
- After editing stops, there shouldn’t be a visible focus ring left around the
selector element.
- A rule selector that doesn’t match anything after being edited will
disappear. This is unexpected, and there is no way to find the rule again.
Instead, the selector should become grayed out.

> Source/WebInspectorUI/ChangeLog:7
> +	   field editable.

I noticed clicking a property name/value does not select the text, it just
begins editing. We should probably change that too (but can be another patch
unless it's a small change).

> Source/WebInspectorUI/ChangeLog:12
> +	   Shift-Tab should commit changes and navigate to the last rule's
property value, if there's one.

I think these four lines would be better as a list:

Keyboard behavior while editing:
- Enter should commit changes.
- Escape should...

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:397
> +	       this._selectorElement.selectText();

When is this called? In testing I never saw this code run.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection
.js:337
> +	   element.selectText();

If SpreadsheetSelectorField was responsible for selecting the element's text,
you could remove the `selectText` utility function and move that code here.