-
Notifications
You must be signed in to change notification settings - Fork 76
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
fix(input, input-number, input-text, input-date-picker, input-time-picker, filter, menu-item): ignore canceled events and enforce cancellations where needed #8952
Changes from all commits
2530251
4e78c32
939b5b9
936a3e9
0240864
a7b57be
44d3c43
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -415,14 +415,15 @@ export class InputText | |
//-------------------------------------------------------------------------- | ||
|
||
keyDownHandler = (event: KeyboardEvent): void => { | ||
if (this.readOnly || this.disabled) { | ||
if (this.readOnly || this.disabled || event.defaultPrevented) { | ||
return; | ||
} | ||
|
||
if (this.isClearable && event.key === "Escape") { | ||
this.clearInputTextValue(event); | ||
event.preventDefault(); | ||
} | ||
if (event.key === "Enter" && !event.defaultPrevented) { | ||
if (event.key === "Enter") { | ||
if (submitForm(this)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the input text, we are only calling preventDefault if the input is inside a form. I wonder if we should call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not according to our conventions. We should only be canceling native events if the component reacts to an event for a specific interaction. |
||
event.preventDefault(); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -269,6 +269,11 @@ export class CalciteMenuItem implements LoadableComponent, T9nComponent, Localiz | |
const { hasSubmenu, href, layout, open, submenuItems } = this; | ||
const key = event.key; | ||
const targetIsDropdown = event.target === this.dropdownActionEl; | ||
|
||
if (event.defaultPrevented) { | ||
return; | ||
} | ||
|
||
if (key === " " || key === "Enter") { | ||
if (hasSubmenu && (!href || (href && targetIsDropdown))) { | ||
this.open = !open; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this example, shouldn't we be preventing default for enter and space key regardless? Currently, it is only preventing default if the key is space, component has href and target is a dropdown. if (key === " " || key === "Enter") {
if (hasSubmenu && (!href || (href && targetIsDropdown))) {
this.open = !open;
}
if (!(href && targetIsDropdown) && key !== "Enter") {
this.selectMenuItem(event);
}
if (key === " " || (href && targetIsDropdown)) {
event.preventDefault();
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are if/else statement blocks that won't be executed (e.g., |
||
|
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.
Something I'm noticing is that we're having to call
event.preventDefault()
for every key. This is fine, but maybe it would be good to have a dom utility function to handle this better for us.Something like:
usage:
what do you think @jcfranco?
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.
The proposed util would reduce explicit calls to the method, but does duplicate the keys.
Before
After