Bug 192919 – Web Inspector: Styles: Pressing Esc when editing name/value should select entire property

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Currently, when editing a property, there's no keyboard shortcut to go from editing mode to selection mode.

<rdar://problem/35068265>

[Nikita Vasilyev]

Created attachment 357866 [details]
Patch

[Nikita Vasilyev]

Created attachment 357867 [details]
[Video] With patch applied

[Nikita Vasilyev]

(In reply to Nikita Vasilyev from comment #2)
> Created attachment 357867 [details]
> [Video] With patch applied

- Esc still discards the completion popover.
- When there's no completion popover, it selects outer scope, i.e. it goes from editing name/value to selecting the entire property.
- Esc still discards changes.
- Pressing Esc for newly added properties discards those properties.

[Devin Rousso]

Comment on attachment 357866 [details]
Patch

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

rs=me, with a few Style/NITs

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:483
> +        this.deselectProperties();

Rather than deselecting everything, shouldn't we just deselect that property (assuming it even is selected)?  If not, should we at least limit this so that it only deselects if the `propertyView` was already selected?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:496
> +            this.deselectProperties();

It doesn't seem possible to hit this code.  If the `propertyView` has an `index === -1`, that would mean that it's not being shown, which would mean it's just been deleted.  The only way this function gets called in the first place is if the property is not being deleted (e.g. not new), therefore meaning that it has to have been visible.  I think we should replace this with `console.assert(index !== -1)`.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:394
> +    spreadsheetTextFieldDidPressEsc(textField, valueBeforeEditing)

NIT: rather than use `valueBeforeEditing` (which makes me think of the property's "value"), perhaps we can use `textBeforeEditing` or `contentsBeforeEditing` so it's less confusing.  I get that this is a delegate call from `WI.SpreadsheetTextField`, which has nothing to do with a `WI.CSSProperty`, but I think that that may be confusing in this context.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:396
> +        const isNewProperty = !valueBeforeEditing;

Style: use `let`.

[Nikita Vasilyev]

Created attachment 357901 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:496
>> +            this.deselectProperties();
> 
> It doesn't seem possible to hit this code.  If the `propertyView` has an `index === -1`, that would mean that it's not being shown, which would mean it's just been deleted.  The only way this function gets called in the first place is if the property is not being deleted (e.g. not new), therefore meaning that it has to have been visible.  I think we should replace this with `console.assert(index !== -1)`.

I think you're right, I can't hit this anymore.

[WebKit Commit Bot]

Comment on attachment 357901 [details]
Patch

Clearing flags on attachment: 357901

Committed r239481: <https://trac.webkit.org/changeset/239481>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.