-
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
Conversation
This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions. |
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.
@@ -893,6 +893,7 @@ export class InputDatePicker | |||
} | |||
|
|||
if (key === "Enter") { | |||
event.preventDefault(); |
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:
export function preventDefaultForKeys(event: KeyboardEvent, keys: KeyboardEvent["key"][]): void {
if (!event.defaultPrevented && keys.includes(event.key)) {
event.preventDefault();
}
}
usage:
preventDefaultForKeys(["ArrowDown", "ArrowUp", "ArrowLeft", "ArrowRight"], [" ", "Enter"]);
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
onKeyDown(event: KeyboardEvent): void {
if (event.key === "ArrowDown" {
/* ... */
event.preventDefault();
} else if (event.key === "ArrowLeft" {
/* ... */
event.preventDefault();
}
}
After
onKeyDown(event: KeyboardEvent): void {
preventDefaultForKeys(["ArrowDown", "ArrowUp", "ArrowLeft", "ArrowRight"], [" ", "Enter"]);
if (event.key === "ArrowDown" {
/* ... */
} else if (event.key === "ArrowLeft" {
/* ... */
}
}
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 comment
The 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 event.preventDefault()
for the enter key all the time? cc @jcfranco. Is it inconsistent to only call this when inside a form?
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 it inconsistent to only call this when inside a form?
Not according to our conventions. We should only be canceling native events if the component reacts to an event for a specific interaction.
packages/calcite-components/src/components/input-time-picker/input-time-picker.tsx
Outdated
Show resolved
Hide resolved
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 comment
The 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 comment
The 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., Enter
+ !hasSubmenu
), so I wouldn't agree we should cancel regardless of key pressed.
This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions. |
Looking good, @Elijbet! Is there another way to describe what this PR does? I'm not sure |
🤖 I have created a release *beep* *boop* --- <details><summary>@esri/calcite-design-tokens: 2.2.0</summary> ## [2.2.0](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-design-tokens@2.1.2...@esri/calcite-design-tokens@2.2.0) (2024-04-30) ### Features * Ensure all components inherit font-family ([#8388](#8388)) ([90f8923](90f8923)) </details> <details><summary>@esri/eslint-plugin-calcite-components: 1.2.0</summary> ## [1.2.0](https://github.com/Esri/calcite-design-system/compare/@esri/eslint-plugin-calcite-components@1.1.0...@esri/eslint-plugin-calcite-components@1.2.0) (2024-04-30) ### Features * **enforce-ref-last-prop:** Add fixer ([#8671](#8671)) ([688bc51](688bc51)) ### Bug Fixes * **slider:** Improve snapping + step logic ([#8169](#8169)) ([8b83042](8b83042)) </details> <details><summary>@esri/calcite-components: 2.8.0</summary> ## [2.8.0](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components@2.7.1...@esri/calcite-components@2.8.0) (2024-04-30) ### Features * Add calciteInvalid event for form validation ([#9211](#9211)) ([a504508](a504508)) * Add validity property to form components ([#9210](#9210)) ([e49902b](e49902b)) * **carousel:** Add Carousel and Carousel Item components ([#8995](#8995)) ([b1ba428](b1ba428)) * **enforce-ref-last-prop:** Add fixer ([#8671](#8671)) ([688bc51](688bc51)) * Ensure all components inherit font-family ([#8388](#8388)) ([90f8923](90f8923)) * **input-time-zone:** Add readonly support ([#9111](#9111)) ([153a122](153a122)) * **input-time-zone:** Allow clearing value ([#9168](#9168)) ([193bb7d](193bb7d)) * **list-item-group:** Update list item group styles ([#9072](#9072)) ([c734849](c734849)) * **navigation-logo:** Add heading level ([#9054](#9054)) ([c2beba8](c2beba8)) * **pagination:** Add navigation methods ([#9154](#9154)) ([5ca6a5f](5ca6a5f)) * **slider:** Add support for custom label formatting ([#9179](#9179)) ([710d1ee](710d1ee)) * **slider:** Allow configuring fill behavior ([#9170](#9170)) ([1b2cdbf](1b2cdbf)) * **tile, tile-group:** Support single, multi, single-persist, none selection modes and icon and border selection appearances ([#9159](#9159)) ([2d77470](2d77470)) ### Bug Fixes * **action:** Maintain equal height when text is not enabled in a small scale ([#9051](#9051)) ([407824a](407824a)) * **chip-group:** Improve programmatic Chip selection behavior ([#9213](#9213)) ([b7a4c77](b7a4c77)) * **combobox:** Update the focus and hover chevron icon color ([#9187](#9187)) ([a1317da](a1317da)) * **combobox, input-time-zone:** Improve readOnly behavior and styling ([#9222](#9222)) ([606d80f](606d80f)) * **combobox:** Fix aria-role for proper voiceover support ([#9039](#9039)) ([eebe8ca](eebe8ca)) * **combobox:** Update the focused chevron icon color ([#9202](#9202)) ([27ac02d](27ac02d)) * **date-picker:** Ensure `proximitySelectionDisabled` resets range on post-range-commit selection ([#9171](#9171)) ([f466b14](f466b14)) * **date-picker:** Restore focus on date when navigating month with arrow/page keys ([#9063](#9063)) ([db109e0](db109e0)) * Fix memory leak affecting components using conditionally-rendered slots ([#9208](#9208)) ([28701b6](28701b6)) * **input-date-picker:** Update the focus and hover chevron icon color ([#9146](#9146)) ([ca895e9](ca895e9)) * **input-time-zone:** Ensure selected item is properly displayed when there are other items with the same offset ([#9134](#9134)) ([1f94903](1f94903)) * **input, input-number, input-text, input-date-picker, input-time-picker, filter, menu-item:** Ignore canceled events and enforce cancellations where needed ([#8952](#8952)) ([d0fa977](d0fa977)) * **list, list-item, list-item-group:** Improve drag experience by indenting items ([#9169](#9169)) ([88aab49](88aab49)) * **list, list-item:** Support keyboard sorting in screen readers ([#9038](#9038)) ([b2e1b9b](b2e1b9b)) * **list:** Closed list-items no longer render extra space ([#9031](#9031)) ([3985004](3985004)) * **panel:** Support cancelling the esc key event to prevent closing a panel ([#9032](#9032)) ([ecfa5f1](ecfa5f1)) * **preset:** Update the focus outline color ([#9098](#9098)) ([725f47c](725f47c)) * **radio-button:** Display validation message in parent group ([#9218](#9218)) ([b1e869f](b1e869f)) * **select:** Update the focus and hover chevron icon color and select focus and hover background color ([#9160](#9160)) ([b187340](b187340)) * **slider:** Fix fill placement and tick styling when ranged ([#9204](#9204)) ([a84d831](a84d831)) * **slider:** Improve snapping + step logic ([#8169](#8169)) ([8b83042](8b83042)) * **slider:** Rename `highlightPlacement` to `fillPlacement` ([#9195](#9195)) ([9e5a713](9e5a713)) * **slider:** Style ticks according to the fill placement ([#9196](#9196)) ([5eb4e10](5eb4e10)) * **stepper-item:** Remove delay in highlighting item ([#8996](#8996)) ([bcf23dd](bcf23dd)) * **tree-item:** Do not select when chevron clicked ([#9115](#9115)) ([99ad8f1](99ad8f1)) ### Dependencies * The following workspace dependencies were updated * devDependencies * @esri/calcite-design-tokens bumped from ^2.2.0-next.0 to ^2.2.0 * @esri/eslint-plugin-calcite-components bumped from ^1.2.0-next.1 to ^1.2.0 </details> <details><summary>@esri/calcite-components-angular: 2.8.0</summary> ## [2.8.0](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components-angular@2.7.1...@esri/calcite-components-angular@2.8.0) (2024-04-30) ### Miscellaneous Chores * **@esri/calcite-components-angular:** Synchronize components versions ### Dependencies * The following workspace dependencies were updated * dependencies * @esri/calcite-components bumped from ^2.8.0-next.20 to ^2.8.0 </details> <details><summary>@esri/calcite-components-react: 2.8.0</summary> ## [2.8.0](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components-react@2.7.1...@esri/calcite-components-react@2.8.0) (2024-04-30) ### Miscellaneous Chores * **@esri/calcite-components-react:** Synchronize components versions ### Dependencies * The following workspace dependencies were updated * dependencies * @esri/calcite-components bumped from ^2.8.0-next.20 to ^2.8.0 </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Related Issue: #7686
Summary
Check
event.defaultPrevented
before handlingkeyDown
events andevent.preventDefault
for keys we are handling.