fix(kselect): keyboard operation issues #2780
Conversation
* Fixes the issue that `tab` focus on the select trigger automatically opens the select dropdown. * Fixes the issue when `selection` occurred, the dropdown gets closed, the focus won't restore back to the trigger.
jw-foss
requested review from
a team,
Justineo,
adamdehaven and
jillztom
as code owners
✅ Deploy Preview for kongponents-sandbox ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for kongponents ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
I noticed that if I clicked outside the select menu to close it, the focus still returns to the select itself, is this intentional? |
| if (evt.keyCode === 27) { | ||
| if (evt.key === 'Escape' && isToggled.value) { | ||
| isToggled.value = false | ||
| popperRef.value?.hidePopover?.() |
There was a problem hiding this comment.
Shouldn't setting isToggled.value = false already closing the popper?
There was a problem hiding this comment.
Actually the isToggled does not control the state of popper this gets tricky when someone wants to mutate the state of the popper it's like we are controlling two states at the same time.
KToggle does not provide any control over KPopper, because the initial state of KToggle and KPopper were both false, it is our liability to sync the state.
| if (key === 'Enter') { | ||
| e.stopPropagation() | ||
| e.stopImmediatePropagation() | ||
| } |
There was a problem hiding this comment.
What would happen if we didn’t stop here? Might be worth adding a comment.
There was a problem hiding this comment.
This should be dismissed, I only added this because previously we are using keyup on the trigger element KInput, and keydown on the SelectItem component. Removed.
Preview package from this PR in consuming applicationIn consuming application project install preview version of kongponents generated by this PR: |
Looks like a bug, converting this to a draft for fixing minor issues. |
| const focus = () => { | ||
| inputRef.value?.focus?.() | ||
| } | ||
|
|
||
| const blur = () => { | ||
| inputRef.value?.blur?.() | ||
| } | ||
|
|
||
| defineExpose({ | ||
| focus, | ||
| blur, | ||
| }) |
There was a problem hiding this comment.
suggestion: can we just expose the input ref rather than focus and blur methods?
| // TODO: when it is opened by 'Home', 'End', 'ArrowUp', 'ArrowDown' keys, it should handle | ||
| // setting focus to different item |
There was a problem hiding this comment.
I'm not sure I understand this comment.
| } | ||
|
|
||
| if ((evt.key === 'ArrowDown' || evt.key === 'ArrowUp') && isToggled.value) { | ||
| // TODO: we need to remove this part since the popover should trap the focus |
There was a problem hiding this comment.
How do you then escape the popover?
| <div | ||
| ref="itemsContainer" | ||
| > |
There was a problem hiding this comment.
nit: this formatting change isn't necessary
| const itemsContainerRef = useTemplateRef('itemsContainer') | ||
|
|
||
| const moveItemFocus = (direction: 'down' | 'up'): void => { | ||
| const moveItemFocus = (direction: 'down' | 'up' | 'current'): void => { |
There was a problem hiding this comment.
current being a value for a direction param is a bit confusing?
|
Hi @adamdehaven, thank you for reviewing my PR! Right now this PR has a few flaws, and I am trying to fix it before having a review 🙇 |
tabfocus on the select trigger automatically opens the select dropdown.selectionoccurred, the dropdown gets closed, the focus won't restore back to the trigger.Summary
Root cause
As the screencast shows, when we tried to navigate via keyboards, the
Tabkey comes into play, whenever weTabonto a select trigger, the dropdown automatically opens, and it does not attach focus state to the select items. According to the W3C standard https://www.w3.org/WAI/ARIA/apg/patterns/combobox/examples/combobox-select-only/ theTabaction should only set focus onto the trigger element (input in this case), withopen keyssuch asspace bar,enter,arrow downandarrow up(HomeandEndwas not included in this PR), to open the popup, and sets focus on the selected item if any or first item in the selection list.CleanShot.2025-06-04.at.14.01.26.mp4
Patch made
After this patch, the behavior should be aligned with the standard to allow user to set focus onto the trigger element, and they can use the
open keyshere to open the popup(dropdown), see the screenshot down below 👇CleanShot.2025-06-04.at.13.36.31.mp4
/cc @portikM
KHCP-15521