Bug 192947 – Web Inspector: Keyboard shortcut for settings tab too greedy on non-US keyboards

• Component: Web Inspector
• Status: RESOLVED FIXED

The Inspector, in WebInspectorUI/UserInterface/Models/KeyboardShortcut.js, handles keyboard shortcuts through the deprecated "keyCode" attribute from the onkeydown event.
On non-US keyboards, this attribute is unreliable (see https://hacks.mozilla.org/2017/03/internationalize-your-keyboard-controls/). 

For instance:
On a French MacBook Pro keyboard, the "<" key (between left shift and W, no equivalent key on an QWERTY keyboard) has the same keyCode (188) as the Comma key. (On a Turkish keyboard, the ö key also has the 188 keyCode.) On some systems, Cmd-< is used to cycle between windows (the equivalent to Cmd-` on US layouts). French users hitting the cycle shortcut out of muscle memory end up on the Inspector's setting page.

Possible solutions:
The "key" attribute on the onkeydown event returns the proper key values for every keyboard type. KeyboardShortcut.js could be modified as such.

[Radar WebKit Bug Importer]

<rdar://problem/46886779>

[Nikita Vasilyev]

I was able to reproduce the problem on MacOS with the French virtual keyboard.

Thank you for the detailed bug report! I also agree with the proposed solution.

[Nikita Vasilyev]

Created attachment 362917 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 362917 [details]
Patch

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

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

[Devin Rousso]

Comment on attachment 362917 [details]
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:340
> +        if (event.key !== WI.KeyboardShortcut.Key.Enter)

Missing `.key`.

[Nikita Vasilyev]

Comment on attachment 362917 [details]
Patch

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

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

I like `WI.Key.Space`.

Regarding `matchesEvent`. What if we do this:

    event.matchesKey(WI.Key.Backspace, WI.Key.Delete)

instead of

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

?

[Devin Rousso]

Comment on attachment 362917 [details]
Patch

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

>>>> 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.
> 
> I like `WI.Key.Space`.
> 
> Regarding `matchesEvent`. What if we do this:
> 
>     event.matchesKey(WI.Key.Backspace, WI.Key.Delete)
> 
> instead of
> 
>     WI.Key.Backspace.matchesEvent(event) || WI.Key.Delete.matchesEvent(event)
> 
> ?

`event.matchesKey(WI.Key.Backspace, WI.Key.Delete)` makes me think we're trying to match a case where the user is pressing Backspace and Delete at the same time (imagine if you used `WI.Key.Plus` and `WI.Key.Minus` instead).

I prefer the more "verbose" approach of having each key use `matchesEvent` as it gives us (the Web Inspector developers) more flexibility.

[Nikita Vasilyev]

Comment on attachment 362917 [details]
Patch

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

>>>>> 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.
>> 
>> I like `WI.Key.Space`.
>> 
>> Regarding `matchesEvent`. What if we do this:
>> 
>>     event.matchesKey(WI.Key.Backspace, WI.Key.Delete)
>> 
>> instead of
>> 
>>     WI.Key.Backspace.matchesEvent(event) || WI.Key.Delete.matchesEvent(event)
>> 
>> ?
> 
> `event.matchesKey(WI.Key.Backspace, WI.Key.Delete)` makes me think we're trying to match a case where the user is pressing Backspace and Delete at the same time (imagine if you used `WI.Key.Plus` and `WI.Key.Minus` instead).
> 
> I prefer the more "verbose" approach of having each key use `matchesEvent` as it gives us (the Web Inspector developers) more flexibility.

I can't find a single keyboard shortcut that requires pressing two non-modifier keys at the same time.
I found over a dozen of cases when we check for delete or backspace, up or down, right or left, and pageUp or pageDown.

[Nikita Vasilyev]

Comment on attachment 362917 [details]
Patch

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

>>>>>> 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.
>>> 
>>> I like `WI.Key.Space`.
>>> 
>>> Regarding `matchesEvent`. What if we do this:
>>> 
>>>     event.matchesKey(WI.Key.Backspace, WI.Key.Delete)
>>> 
>>> instead of
>>> 
>>>     WI.Key.Backspace.matchesEvent(event) || WI.Key.Delete.matchesEvent(event)
>>> 
>>> ?
>> 
>> `event.matchesKey(WI.Key.Backspace, WI.Key.Delete)` makes me think we're trying to match a case where the user is pressing Backspace and Delete at the same time (imagine if you used `WI.Key.Plus` and `WI.Key.Minus` instead).
>> 
>> I prefer the more "verbose" approach of having each key use `matchesEvent` as it gives us (the Web Inspector developers) more flexibility.
> 
> I can't find a single keyboard shortcut that requires pressing two non-modifier keys at the same time.
> I found over a dozen of cases when we check for delete or backspace, up or down, right or left, and pageUp or pageDown.

One KeyboardEvent can't even contain two different keys at the same time (like minus and plus simultaneously).

[Devin Rousso]

Comment on attachment 362917 [details]
Patch

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

>>>>>>> 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.
>>>> 
>>>> I like `WI.Key.Space`.
>>>> 
>>>> Regarding `matchesEvent`. What if we do this:
>>>> 
>>>>     event.matchesKey(WI.Key.Backspace, WI.Key.Delete)
>>>> 
>>>> instead of
>>>> 
>>>>     WI.Key.Backspace.matchesEvent(event) || WI.Key.Delete.matchesEvent(event)
>>>> 
>>>> ?
>>> 
>>> `event.matchesKey(WI.Key.Backspace, WI.Key.Delete)` makes me think we're trying to match a case where the user is pressing Backspace and Delete at the same time (imagine if you used `WI.Key.Plus` and `WI.Key.Minus` instead).
>>> 
>>> I prefer the more "verbose" approach of having each key use `matchesEvent` as it gives us (the Web Inspector developers) more flexibility.
>> 
>> I can't find a single keyboard shortcut that requires pressing two non-modifier keys at the same time.
>> I found over a dozen of cases when we check for delete or backspace, up or down, right or left, and pageUp or pageDown.
> 
> One KeyboardEvent can't even contain two different keys at the same time (like minus and plus simultaneously).

I understand that.  I'm talking about how the code reads, not what it does.  That reads like "check that the key is <a> _and_ <b>", not "check that the key is <a> _or_ <b>".

[Nikita Vasilyev]

Converting everything to `key` broke keyboard shortcuts that use Option key.

For instance, Command-Option-2 supposed to select second WI tab, when undocked.

Option-2 produces "™" (tm) character instead of "2".

[Nikita Vasilyev]

(In reply to Nikita Vasilyev from comment #11)
> Converting everything to `key` broke keyboard shortcuts that use Option key.
> 
> For instance, Command-Option-2 supposed to select second WI tab, when
> undocked.
> 
> Option-2 produces "™" (tm) character instead of "2".

`code` is based on the physical position of the key. Option-2 produces `Digit2`.

Why can't use `code` everywhere - it would break non-Querty layouts, such as Dvorak and Colemak.

The best solution I can think of is to use `key` for most cases and
use `code` for cases when `key` doesn't work. We'd have to make WI.KeyboardShortcut support both.

[Nikita Vasilyev]

We have keyboard shortcuts like Command-Option-R.

Option-R on Querty produces:
key: ® (R)
code: KeyR

Option-R on Colemak produces:
key: ® (R)
code: KeyS (different key!)

So we can use neither `code` nor `key`.

---

keyIdentifier seems to work fine for this case.

Option-R
keyIdentifier: U+0052 (Latin capital letter R)

`keyIdentifier` is unreadable and non-standard :(

[Nikita Vasilyev]

Created attachment 362945 [details]
Patch

I feel pretty strongly now that fixing this bug should be separate from refactoring all uses of code/key/charCode/keyCode/keyIdentifier and potentially introducing several regressions.

[Devin Rousso]

(In reply to Nikita Vasilyev from comment #13)
> keyIdentifier seems to work fine for this case.
> 
> Option-R
> keyIdentifier: U+0052 (Latin capital letter R)
> 
> `keyIdentifier` is unreadable and non-standard :(
Why is this "unreadable"?  It matches exactly with the Unicode table.

If `keyIdentifier` is consistent across keyboard layouts, I think we should use that going forward, and just define a `WI.Key.___` for each character that we end up using.

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #15)
> (In reply to Nikita Vasilyev from comment #13)
> > keyIdentifier seems to work fine for this case.
> > 
> > Option-R
> > keyIdentifier: U+0052 (Latin capital letter R)
> > 
> > `keyIdentifier` is unreadable and non-standard :(
> Why is this "unreadable"?  It matches exactly with the Unicode table.

I don't have Unicode table memorized.

> 
> If `keyIdentifier` is consistent across keyboard layouts, I think we should
> use that going forward, and just define a `WI.Key.___` for each character
> that we end up using.

I haven't found any issues with keyIndentifier besides the ones I already mentioned. It doesn't mean they don't exist.

Regardless, it's going to be large refactoring. I created Bug 195031 - Web Inspector: Use keyIdentifier instead of keyCode for KeyboardShortcut.

[Devin Rousso]

Comment on attachment 362945 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/Main.js:712
> +    if (event.key === ",")

This feels like a hack.  Can we confirm that the even't `keyIdentifier` matches `"U+002C"` instead?

[Nikita Vasilyev]

Created attachment 363399 [details]
Patch

[EWS Watchlist]

Comment on attachment 363399 [details]
Patch

Attachment 363399 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11340092

New failing tests:
jquery/offset.html

[EWS Watchlist]

Created attachment 363412 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Nikita Vasilyev]

Comment on attachment 363399 [details]
Patch

Unrelated test failures.

[Devin Rousso]

Comment on attachment 363399 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 363399 [details]
Patch

Clearing flags on attachment: 363399

Committed r242821: <https://trac.webkit.org/changeset/242821>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.