Bug 177711 – Web Inspector: Styles Redesign: Add support for keyboard navigation (Tab, Shift-Tab, Enter, Esc)

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Enter, Tab, Shift-Tab should commit changes.
Escape should discard changes.

Tab, Enter, and Escape should navigate forward (focus on the next field).
Shift-Tab should navigate backward (focus on the previous field).


When navigating forward from:

- Selector: Focus on the first property name. If it doesn’t exist, create a blank property.

- Property name:
 -- If property name is blank, discard the property and focus on the next editable field (property name or selector of the next rule).
 -- If property is not blank, focus on the value.

- Property value: 
 -- If the last value in the rule, create a blank property and focus on its name.
 -- If not the last value in the rule, focus on the next editable field (property name or selector of the next rule).


When navigating backward from:

- Selector: create a blank property on the previous editable rule and focus on its name.

- Property name:
 -- Focus on the rule's selector.

- Property value:
 -- Focus on the property name.

<rdar://problem/33526348>

[Nikita Vasilyev]

Created attachment 322308 [details]
[Animated GIF] WIP

[Nikita Vasilyev]

Created attachment 322310 [details]
WIP

Don't look at the code. It's full of logging, commented out blocks, and it's gross overall.

Feel free to apply the patch and let me know if something is seriously broken.

[Nikita Vasilyev]

Created attachment 322769 [details]
Patch

[Nikita Vasilyev]

(In reply to Nikita Vasilyev from comment #0)
> Enter, Tab, Shift-Tab should commit changes.
> Escape should discard changes.
> 
> Tab, Enter, and Escape should navigate forward (focus on the next field).

Correction: Escape should just discard changes. It shouldn't navigate forward or backward.

[Joseph Pecoraro]

Comment on attachment 322769 [details]
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()!

[Joseph Pecoraro]

I saw a bunch of asserts:

Models/CSSProperty.js:341:23: CONSOLE ERROR Style text did not change 
Models/CSSProperty.js:341:23: CONSOLE ERROR Style text did not change 

They are harmless but it might be good to cut down on the noise if you can figure out what causes it.

[Nikita Vasilyev]

Comment on attachment 322769 [details]
Patch

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

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

Yes, correct.

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

It mirrors DOM Selection.propotype.collapseToEnd().

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

I kept the names from WI.CSSStyleDeclarationTextEditor. I can change them.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:56
>>      }
> 
> This appears to no longer be used since you inlined it below. You can probably remove this.

I incorrectly rebaselined a patch 🤦‍♂️

[Nikita Vasilyev]

Created attachment 322919 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 322919 [details]
Patch

cq-, investigation an exception:

[Error] TypeError: undefined is not an object (evaluating 'this._propertyViews[index].valueTextField')
	spreadsheetCSSStyleDeclarationEditorFocusMoved (SpreadsheetCSSStyleDeclarationEditor.js:126)
	spreadsheetTextFieldDidCommit (SpreadsheetStyleProperty.js:194)
	_handleKeyDown (SpreadsheetTextField.js:139)
	_handleKeyDown

[Nikita Vasilyev]

Created attachment 322932 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 322932 [details]
Patch

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

r=me.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetSelectorField.js:85
> +    _handleClick(event)

What happens if you Right Click? (event.button !== 1)

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetSelectorField.js:100
> +            this._delegate.spreadsheetSelectorFieldDidChange();

Might be worth passing `null` here as a sign that you know you are sending null as the direction.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:1
> +/*

Much nicer having these classes in their own file!

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:26
> +WI.SpreadsheetTextField = class SpreadsheetTextField

I think the SpreadsheetSelectorField and SpreadsheetTextField should eventually be merged. They are mostly identical so it is just confusing to have two classes that do nearly the same thing.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:51
> +    get element() { return this._element; }

Style: I'd recommend putting element at the top of the list of getters. Its normally the one people look for things that have an element.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:55
> +        if (this._editing) return;

Style: put the return on a newline.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:111
> +    _handleClick(event)
> +    {
> +        if (!this._editing)
> +            return;
> +
> +        this.startEditing();
> +    }

This doesn't appear to do anything.

    • if you are not editing it returns
    • if you are editing it returns immediately in startEditing

One possible solution is to remove this and the event registration.
Another is to actually make this startEditing if we aren't editing (like the SelectorField does).

Which should this do?

What happens if you Right Click? (event.button !== 1)

[Nikita Vasilyev]

Comment on attachment 322932 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:26
>> +WI.SpreadsheetTextField = class SpreadsheetTextField
> 
> I think the SpreadsheetSelectorField and SpreadsheetTextField should eventually be merged. They are mostly identical so it is just confusing to have two classes that do nearly the same thing.

Yes, that's my plan!

The main difference is that selectors update on blur, but names and values update immediately.

I may convert delegates to events.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:111
>> +    }
> 
> This doesn't appear to do anything.
> 
>     • if you are not editing it returns
>     • if you are editing it returns immediately in startEditing
> 
> One possible solution is to remove this and the event registration.
> Another is to actually make this startEditing if we aren't editing (like the SelectorField does).
> 
> Which should this do?
> 
> What happens if you Right Click? (event.button !== 1)

Good catch! I'll remove this.

"click" event doesn't fire on Right Click (mousedown does).

[Nikita Vasilyev]

Created attachment 322988 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 322988 [details]
Patch

Clearing flags on attachment: 322988

Committed r222959: <http://trac.webkit.org/changeset/222959>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.