[webkit-reviews] review denied: [Bug 192947] Web Inspector: Keyboard shortcut for settings tab too greedy on non-US keyboards : [Attachment 362917] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has denied Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 192947: Web Inspector: Keyboard shortcut for settings tab too greedy on
non-US keyboards
https://bugs.webkit.org/show_bug.cgi?id=192947

Attachment 362917: Patch

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




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

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

> Source/WebInspectorUI/ChangeLog:16
> +	   (WI.Key.prototype.get keyCode): Removed.

There were other "use"s of something similar (e.g. `keyIdentifier`) in some
other files that should also be changed.
 - `isEnterKey` (Utilities.js)
 - `WI.SelectionController.prototype.handleKeyDown` (SelectionController.js)
 - `WI.BezierEditor.prototype._handleNumberInputKeydown` (BezierEditor.js)
 - `WI.BoxModelDetailsSectionRow.prototype._alteredFloatNumber` and
`WI.BoxModelDetailsSectionRow.prototype._handleKeyDown`
(BoxModelDetailsSectionRow.js)
 - `WI.DataGrid.prototype._keyDown` (DataGrid.js)
 - `WI.startEditing` (EditingSupport.js)
 - `WI.FindBanner.prototype._inputFieldKeyDown` and
`WI.FindBanner.prototype._inputFieldKeyUp` (FindBanner.js)
 - `WI.LogContentView.prototype._keyDown` (LogContentView.js)
 - `WI.NavigationBar.prototype._keyDown` (NavigationBar.js)
 - `WI.SpringEditor.prototype._handleNumberInputKeydown` (SpringEditor.js)
 - `WI.TreeOutline.prototype._treeKeyDown` (TreeOutline.js)

Please search around for uses of `code`, `charCode`, `keyCode`,
`keyIdentifier`, `which`, etc, and make sure that they're all changed to the
same uniform "style".

<https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent>

>> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:343
>> +	    if (event.key === WI.KeyboardShortcut.Key.Backspace.key ||
event.key === WI.KeyboardShortcut.Key.Delete.key)
> 
> I find `WI.KeyboardShortcut.Key.*.key` to be very verbose. I'd prefer to
simply use strings everywhere.
> 
>     if (event.key === "Backspace" || event.key === "Delete")
> 
> I find this to be more readable. What do you think? (Asking code reviewers.)

I think we should add a `matchesEvent` to `WI.Key` (similar to
`WI.KeyboardShortcut.prototype.matchesKey`).  That way, if we decide to change
the logic of how we detect specific keys (e.g. this patch), we only need to do
it in one place.

    if (WI.KeyboardShortcut.Key.Backspace.matchesEvent(event) ||
WI.KeyboardShortcut.Key.Delete.matchesEvent(event))

`WI.Key`:
    matchesEvent(event)
    {
	console.assert(event instanceof KeyboardEvent);
	return event.key === this._key;
    }

Perhaps we should also consider renaming `WI.KeyboardShortcut.Key` to
`WI.keyboardKeys` (or even just adding each item to `WI.Key` directly, like
`WI.Key.Space`) to make the code a bit less "verbose".

Regardless, we should be consistent and always use either a plain string or the
model object.

> Source/WebInspectorUI/UserInterface/Views/EditingSupport.js:205
> +	   else if (event.key === WI.KeyboardShortcut.Key.Escape.key ||
event.keyIdentifier === "U+001B")

Is the `event.keyIdentifier === "U+001B"` still necessary?

>
Source/WebInspectorUI/UserInterface/Views/GeneralStyleDetailsSidebarPanel.js:34
0
> +	   if (event.key !== WI.KeyboardShortcut.Key.Enter)

Missing `.key`.