Bug 192947 - Web Inspector: Keyboard shortcut for settings tab too greedy on non-US keyboards
Summary: Web Inspector: Keyboard shortcut for settings tab too greedy on non-US keyboards
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: Safari 12
Hardware: Mac All
: P2 Minor
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-12-20 12:40 PST by ljt
Modified: 2019-03-12 13:10 PDT (History)
7 users (show)

See Also:


Attachments
Patch (19.09 KB, patch)
2019-02-25 12:07 PST, Nikita Vasilyev
hi: review-
Details | Formatted Diff | Diff
Patch (1.31 KB, patch)
2019-02-25 17:33 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Patch (1.28 KB, patch)
2019-03-01 18:54 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-highsierra (1.41 MB, application/zip)
2019-03-01 22:54 PST, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description ljt 2018-12-20 12:40:06 PST
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.
Comment 1 Radar WebKit Bug Importer 2018-12-20 16:08:54 PST
<rdar://problem/46886779>
Comment 2 Nikita Vasilyev 2019-02-25 12:06:58 PST
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.
Comment 3 Nikita Vasilyev 2019-02-25 12:07:56 PST
Created attachment 362917 [details]
Patch
Comment 4 Nikita Vasilyev 2019-02-25 12:13:37 PST
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.)
Comment 5 Devin Rousso 2019-02-25 13:37:37 PST
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`.
Comment 6 Nikita Vasilyev 2019-02-25 14:23:38 PST
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)

?
Comment 7 Devin Rousso 2019-02-25 14:26:37 PST
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.
Comment 8 Nikita Vasilyev 2019-02-25 14:42:43 PST
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.
Comment 9 Nikita Vasilyev 2019-02-25 14:47:11 PST
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).
Comment 10 Devin Rousso 2019-02-25 15:01:19 PST
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>".
Comment 11 Nikita Vasilyev 2019-02-25 16:26:46 PST
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".
Comment 12 Nikita Vasilyev 2019-02-25 16:38:44 PST
(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.
Comment 13 Nikita Vasilyev 2019-02-25 16:50:00 PST
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 :(
Comment 14 Nikita Vasilyev 2019-02-25 17:33:38 PST
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.
Comment 15 Devin Rousso 2019-02-25 18:39:27 PST
(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.
Comment 16 Nikita Vasilyev 2019-02-25 19:01:54 PST
(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.
Comment 17 Devin Rousso 2019-03-01 16:12:14 PST
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?
Comment 18 Nikita Vasilyev 2019-03-01 18:54:05 PST
Created attachment 363399 [details]
Patch
Comment 19 EWS Watchlist 2019-03-01 22:54:53 PST
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
Comment 20 EWS Watchlist 2019-03-01 22:54:54 PST
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
Comment 21 Nikita Vasilyev 2019-03-01 22:59:58 PST
Comment on attachment 363399 [details]
Patch

Unrelated test failures.
Comment 22 Devin Rousso 2019-03-12 13:07:54 PDT
Comment on attachment 363399 [details]
Patch

r=me
Comment 23 WebKit Commit Bot 2019-03-12 13:10:37 PDT
Comment on attachment 363399 [details]
Patch

Clearing flags on attachment: 363399

Committed r242821: <https://trac.webkit.org/changeset/242821>
Comment 24 WebKit Commit Bot 2019-03-12 13:10:39 PDT
All reviewed patches have been landed.  Closing bug.