-
Notifications
You must be signed in to change notification settings - Fork 6.8k
fix(cdk/testing): account for preventDefault in keyboard events #27483
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
ca57ddb
to
5b7fab9
Compare
5b7fab9
to
6a500fb
Compare
@@ -25,6 +35,15 @@ const incrementalInputTypes = new Set([ | |||
/** Characters whose key code doesn't match their character code. */ | |||
const KEYCODE_MISMATCHES: Record<string, number> = { | |||
',': COMMA, | |||
'.': PERIOD, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I listed just the most common conflicting characters that may show up in a test string. Our approach for mapping characters to keycodes using charCodeAt(0)
is fundamentally flawed for many more edge cases. It has been this way since we introduced test harnesses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a better design we ought to use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we would move away from using keyCode
altogether, but there's a lot of code still depending on it.
The other option is to manually maintain a map of all characters to all key codes, but that'll only work for English letters and some keyboard layouts AFAIK.
I'm leaning more towards the first option but it'll take some effort.
6a500fb
to
4d37803
Compare
Currently we try to mimic the user typing in the `typeInElement` utility by incrementally setting the value and dispatching the same sequence of events. The problem is that we weren't accounting for `preventDefault` which can block some keys from being assigned and some events from firing. This leads to inconsistencies between tests and the sequence of events triggered by a user. It is especially noticeable in components like the chip input where some keys act as separators. These changes update the logic to take `preventDefault` into account and try to mimic the native event sequence as closely as possible. Fixes angular#27475.
4d37803
to
41054b3
Compare
Closing since the TGP revealed some fundamental issues with this approach and how we deal with key events in harnesses in general. We should revisit it once we've moved away from using |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Currently we try to mimic the user typing in the
typeInElement
utility by incrementally setting the value and dispatching the same sequence of events. The problem is that we weren't accounting forpreventDefault
which can block some keys from being assigned and some events from firing. This leads to inconsistencies between tests and the sequence of events triggered by a user. It is especially noticeable in components like the chip input where some keys act as separators.These changes update the logic to take
preventDefault
into account and try to mimic the native event sequence as closely as possible.Fixes #27475.