[webkit-reviews] review denied: [Bug 177711] Web Inspector: Styles Redesign: Add support for keyboard navigation (Tab, Shift-Tab, Enter, Esc) : [Attachment 322769] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 177711: Web Inspector: Styles Redesign: Add support for keyboard navigation
(Tab, Shift-Tab, Enter, Esc)
https://bugs.webkit.org/show_bug.cgi?id=177711

Attachment 322769: Patch

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




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

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

Overall this feels real nice.

r-, but fix up the blur of the SpreadSheetTextField and this should be good
enough to land. Ideally there would be some tests on the foundational changes
but there was just one I think (remove).

I have mostly a bunch of style and naming nits. The naming here is all over the
place. I'd like to see longer descriptive names instead of shorthands like
"EditorSwitchRule" / "EditorNextRule" because they lack the action behind the
method. For example these could be"EditorStartEditingAdjacentRule",
"EditorStartEditingNextRule". With those names you could search on
"StartEditing" and find how most of this complex logic moves around. Right now
there are many different names for similar actions.

> Source/WebInspectorUI/ChangeLog:71
> +	   (WI.SpreadsheetStyleProperty):

There should be some comments, at least at the file level, for the changes you
are making. This gets a delegate for example, so there should be a description
that says why? For example "Give StyleDeclarationEditor a delegate so that ..."

Also, you should move SpreadSheetStyleProperty and SpreadSheetTextField into
their own file. We try to avoid multiple classes per-file and it makes it much
easier to jump around our code if its 1 class per file.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:121
> +    remove()

There should really be a test for this. Its a new API on a foundational class
like CSSProperty.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:123
> +	   // Setting name or value to en empty string removes the entire
CSSProperty.

Typo: "en" => "an"

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:336
>	   this._updateOwnerStyleText(this._text, text);
> +	   this._text = text;

I think we probably want to reverse the order of these. We are telling
OwnerStyleText that we have next text, so I'd expect _text to be the new value.

    let oldText = this._text;
    this._text = text;
    this._updateOwnerStyleSheet(oldText, this._text);

If setting this._text after calling this._updateOwnerStyleSheet is required,
than it would warrant a comment, since it would be a non-obvious dependency.

> Source/WebInspectorUI/UserInterface/Models/CSSRule.js:62
> +    get selectorEditable()
> +    {
> +	   return this._ownerRule && this.editable;
> +    }

CSSRule does not have an ownerRule, so this would always be false. In any case
you don't seem to use this so it looks like this can be removed.

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:384
> +	       if (property._styleSheetTextRange) {

What does it mean for there to be a CSSProperty without a StyleSheetTextRange?
Is that like a longhand property of a shorthand?

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:412
> +    _insertionRange(index)

This name is vague. Maybe _rangeAfterPropertyAtIndex(index)?

> Source/WebInspectorUI/UserInterface/Models/TextRange.js:131
> +    collapseToEnd()

This name was confusing to me, but it seems like it might be akin to
Range.prototype.collapse()?

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:81
> +	   if (this._propertyViews.length > 0)

Style: No need for the `> 0`.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:89
> +    selectLastProperty()

A better name for these would probably be: startEditingFirstProperty[Name] and
startEditingLastProperty[Value]. The "select" sounds misleading.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:102
> +	   let {direction, movedFromIndex, didRemoveProperty} = options;

Style: I'd suggest just putting this right in the signature:

    spreadsheetCSSStyleDeclarationEditorFocusMoved({direction, movedFromIndex,
didRemoveProperty})

That way if someone just searches the function name "EditorFocusMoved" they
will see the options without having to look inside.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:308
> +	   if (textField === this._valueTextField) {

Style: Remove the braces

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:333
> +	   let {direction} = options;

Style: I suggest putting this in the function signature.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:430
> +	   requestAnimationFrame(() => {

Why the rAF? This needs an explanation.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:466
> +    _handleBlur(event)
> +    {
> +	   this.stopEditing();
> +    }

If this is a Blank New Property then it should end up getting removed. This, or
stopEditing, probably needs to inform some delegate to do the cleanup. Note
that Shift+Tab from a blank does get cleaned up (the
spreadsheetTextFieldDidCommit delegate does the clean). It seems like blur
should behave like a Commit without moving in any direction.

Steps to Reproduce:
1. Tab through a rule to get a new property
2. Click outside the style sidebar
  => A blank property remains and is not editable

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:470
> +	   if (event.key === "Enter" && !this._editing) {

Hmm, this sounds good but how can happen? This would mean the property is
focused but not editing?

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection
.js:56
>      get selectorEditable()
>      {
> -	   return this._style.editable && this._style.ownerRule;
> +	   return this._style.selectorEditable;
>      }

This appears to no longer be used since you inlined it below. You can probably
remove this.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection
.js:419
> +	       event.stopImmediatePropagation();
> +	       event.preventDefault();

You just added event.stop()!