Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/core/binding.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,13 @@ export class Binding {
private willBeInvokedByEvent(event: Event): boolean {
const eventTarget = event.target

if (event instanceof KeyboardEvent && this.action.shouldIgnoreKeyboardEvent(event)) {
return false
if (event.type === "keydown") {
// when accepting an autocomplete suggestion, Chrome dispatches a keydown event,
// but it isn't a KeyboardEvent instance
const keyboardEvent: KeyboardEvent = event instanceof KeyboardEvent ? event : new KeyboardEvent("keydown")
if (this.action.shouldIgnoreKeyboardEvent(keyboardEvent)) {
Comment on lines +89 to +93
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The underlying Action.shouldIgnoreKeyboardEvent function ultimately accesses properties on the KeyboardEvent instance, so instantiating a new KeyboardEvent instance without those properties might lead to unexpected behavior.

As an alternative, what do you think about using typecasting instead?

Suggested change
if (event.type === "keydown") {
// when accepting an autocomplete suggestion, Chrome dispatches a keydown event,
// but it isn't a KeyboardEvent instance
const keyboardEvent: KeyboardEvent = event instanceof KeyboardEvent ? event : new KeyboardEvent("keydown")
if (this.action.shouldIgnoreKeyboardEvent(keyboardEvent)) {
// When accepting an autocomplete suggestion, Chrome dispatches a keydown event that is not a KeyboardEvent instance
if (event.type === "keydown" && this.action.shouldIgnoreKeyboardEvent(event as KeyboardEvent)) {

That assumes that Chrome's synthetic event does have the properties required to behave like a KeyboardEvent. That might not be the case, but my hunch is that it'd behave better as the syntetic event than as an event that Stimulus constructs itself.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My intention was to use a newly created keyboard event where all of those keys are blank (so nothing matches). Typecasting probably works, but that synthetic event doesn't have any of the KeyboardEvent properties (besides type). The code would be a lot cleaner, though.

return false
}
}

if (event instanceof MouseEvent && this.action.shouldIgnoreMouseEvent(event)) {
Expand Down
8 changes: 8 additions & 0 deletions src/tests/modules/core/action_keyboard_filter_tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -197,4 +197,12 @@ export default class ActionKeyboardFilterTests extends LogControllerTestCase {
await this.triggerEvent(button, "jquery.a")
this.assertActions({ name: "log2", identifier: "a", eventType: "jquery.a", currentTarget: button })
}

async "test ignore events dispatched by autocomplete"() {
const button = this.findElement("#button10")
await this.nextFrame
await this.triggerEvent(button, "keydown", {})

this.assertNoActions()
}
}