Fixed detection of KeyboardEvents

[zupolgec]

Chrome and Safari (and, I suppose, all Chromium and Webkit browsers) have this behaviour where an Event (not a KeyboardEvent) with type "keydown" (and another with "keyup") is triggered on autocomplete input.

https://bugs.chromium.org/p/chromium/issues/detail?id=581537

This fix updates the isKeyEvent function by adding a check on the Event object type, and accepting only KeyboardEvent. This solves a problem of ghost key events that don't have a type property and cause an error when keyToModifier is called with undefined argument.

The first attempt to solve this was by fixing the keyToModifier function (zupolgec@2a1f807), but since the isListeningForASpecificKeyThatHasntBeenPressed function is doing nothing on such fake Events, I thought that this is the best solution.

[HugoDF]

@zupolgec is there any way we can add a unit test for this?

[calebporzio]

Yep, this will at least need a test

[SimoTod]

@zupolgec Good finding but unfortunately the propose solution would introduce a worse bug.
Currently Alpine logs an error in console when using the autofill but the page will work correctly and show the right data. Your fix is treating any keyboard events triggered by the autofill as generic ones and never check for keyboard modifiers which means that the callback will be executed regardless the modifiers.

See this codepen: https://codepen.io/SimoTod/pen/ZEbOOoX (it assumes that your browser will ask you to autofill a field with name="email").
It should increment the counter only when you press ? but it does increment it even when you use the autofill.

A probably better solution would be to change
if (isListeningForASpecificKeyThatHasntBeenPressed(e, modifiers)) {
to something like
if (!e.key || isListeningForASpecificKeyThatHasntBeenPressed(e, modifiers)) {.

A comment above the changed line to explain that we need to check e.key because of a chromium bug would help.

I'm not sure you can autofill a field in jest (you can look into it) but you could create an Event with the same fields as the ones generated in chromium, trigger it on an input field, mock the console.error methods and check that:

1. no errors are logged
2. no updates are triggered

Please, make sure you run the same test on an unpatched version and you get a failure for 1).

I hope it makes sense.

[zupolgec]

@SimoTod I understand and you have a valid point.

The question is: if the user is listening to keydown/keyup events and a generic Event with type keydown/keyup but without key property is brodcasted, should we execute the callback? If there is a modifier should we stop 'cause it can't be matched?

I checked what other libraries are doing on autocomplete.

Alpine (now) (https://codepen.io/zupolgec/pen/YzyWxPK)
with modifier: ignores the callback but triggers an error
w/o modifier: executes the callback

Opinion: almost perfect behaviour, only problem is the error

Alpine (with my old proposal) (https://codepen.io/zupolgec/pen/ExVygLB)
with modifier: executes the callback
w/o modifier: executes the callback

Opinion: wrong, if I have a modifier it shouldn't trigger

Alpine (with your proposal) (https://codepen.io/zupolgec/pen/eYpzBdY)
with modifier: ignores the callback
w/o modifier: ignores the callback

Opinion: wrong, if I want to listen to keydown events and Chromium browsers decided that a keydown event is triggered on autocomplete, I should be able to (https://bugs.chromium.org/p/chromium/issues/detail?id=581537 is not a bug, is a "feature")

Vue (https://codepen.io/zupolgec/pen/QWjEKga)
with modifier: executes the callback
w/o modifier: executes the callback

Opinion: works exactly like my old proposal, so is wrong and maybe I should send this PR to them too 😀

Angular (https://codepen.io/zupolgec/pen/gOaMLPv)
with modifier: ignores the callback
w/o modifier: executes the callback

Opinion: that's the correct behaviour

---

So, I updated my PR with the correct behaviour and a unit test.

You can see it working here: https://codepen.io/zupolgec/pen/YzyWxPK

[SimoTod]

It makes sense, to me the autofill is not a keypress but i didn't look into the reason why chromium sends it. If they say it's correct, I'm happy with your second proposal.

I do think Vue is wrong and angular right so good job, thanks for looking into it.

One final thing, i think you still get an error in console when you have multiple modifiers (like keydown.cmd.e).
I'm on my mobile so i can't verify, could you double check please? Thanks

[zupolgec]

There are no issues with multiple modifiers since the fake keydown event has no active modifiers and so the second keyToModifier function call is not triggered.

[SimoTod]

cool 👍

[zupolgec]

I moved the fix inside the keyToModifier function, I think it makes more sense like this!

[calebporzio]

Great work on this. Thank you!