[webkit-reviews] review granted: [Bug 192919] Web Inspector: Styles: Pressing Esc when editing name/value should select entire property : [Attachment 357866] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Dec 20 16:25:30 PST 2018

Devin Rousso <drousso at apple.com> has granted Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 192919: Web Inspector: Styles: Pressing Esc when editing name/value should
select entire property

Attachment 357866: Patch


--- Comment #4 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 357866
  --> https://bugs.webkit.org/attachment.cgi?id=357866

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

rs=me, with a few Style/NITs

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

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

More information about the webkit-reviews mailing list