[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 bugzilla-daemon at webkit.org
Mon Feb 25 13:37:37 PST 2019

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

Attachment 362917: Patch


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

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


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

	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?

> +	   if (event.key !== WI.KeyboardShortcut.Key.Enter)

Missing `.key`.

More information about the webkit-reviews mailing list