Skip to content
Draft
Show file tree
Hide file tree
Changes from 1 commit
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
17 changes: 16 additions & 1 deletion src/components/KInput/KInput.vue
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@

<input
:id="inputId"
ref="inputRef"
:aria-describedby="helpText ? helpTextId : undefined"
:aria-invalid="error || hasError || charLimitExceeded ? 'true' : undefined"
class="input"
Expand Down Expand Up @@ -90,7 +91,7 @@
</template>

<script setup lang="ts">
import { computed, ref, watch, useSlots, useAttrs, onMounted, nextTick, useId } from 'vue'
import { computed, ref, watch, useSlots, useAttrs, onMounted, nextTick, useId, useTemplateRef } from 'vue'
import type { InputProps, InputEmits, InputSlots } from '@/types'
import useUtilities from '@/composables/useUtilities'
import KLabel from '@/components/KLabel/KLabel.vue'
Expand Down Expand Up @@ -139,6 +140,7 @@ const inputId = computed((): string => attrs.id ? String(attrs.id) : defaultId)
const helpTextId = useId()
const strippedLabel = computed((): string => stripRequiredLabel(label, isRequired.value))
const hasLabelTooltip = computed((): boolean => !!(labelAttributes?.info || slots['label-tooltip']))
const input$ = useTemplateRef('inputRef')

// we need this so we can create a watcher for programmatic changes to the modelValue
const value = computed({
Expand Down Expand Up @@ -245,6 +247,19 @@ const getValue = (): string | number => {
return currValue.value || modelValueChanged.value ? currValue.value : modelValue
}

const focus = () => {
input$.value?.focus?.()
}

const blur = () => {
input$.value?.blur?.()
}

defineExpose({
focus,
blur,
})
Comment on lines +250 to +261
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: can we just expose the input ref rather than focus and blur methods?


watch(() => error, (newVal, oldVal) => {
if (newVal !== oldVal) {
// bump the key to trigger the transition
Expand Down
36 changes: 24 additions & 12 deletions src/components/KSelect/KSelect.vue
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
hide-close-icon
@close="() => onClose(toggle, isToggled.value)"
@open="() => onOpen(toggle)"
@popover-click="() => onPopoverClick(toggle)"
@popover-click="toggle"
>
<div
:id="selectWrapperId"
Expand Down Expand Up @@ -58,9 +58,9 @@
@blur="onInputBlur"
@click="onInputClick"
@focus="onInputFocus"
@keydown="(evt: KeyboardEvent) => triggerFocus(evt, isToggled)"
@keydown.enter="onInputEnter"
@keypress="onInputKeypress"
@keyup="(evt: any) => triggerFocus(evt, isToggled)"
@keyup.enter.stop
@update:model-value="onQueryChange"
>
Expand Down Expand Up @@ -448,20 +448,32 @@ const clearSelection = (): void => {
}

const selectItemsRef = useTemplateRef('kSelectItems')
const NAVIGATION_KEYS = ['ArrowDown', 'ArrowUp']
const OPEN_KEYS = [' ', 'Enter', ...NAVIGATION_KEYS]

const triggerFocus = (evt: any, isToggled: Ref<boolean>): void => {
const triggerFocus = (evt: KeyboardEvent, isToggled: Ref<boolean>): void => {
// Ignore `esc` key
if (evt.keyCode === 27) {
if (evt.key === 'Escape' && isToggled.value) {
isToggled.value = false
popperRef.value?.hidePopover?.()
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't setting isToggled.value = false already closing the popper?

Copy link
Contributor Author

@jw-foss jw-foss Jun 4, 2025

Choose a reason for hiding this comment

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

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.

return
}

const inputElem = selectWrapperRef.value?.children[0] as HTMLInputElement
if (!isToggled.value && inputElem) { // simulate click to trigger dropdown open
inputElem.click()
// per https://www.w3.org/WAI/ARIA/apg/patterns/combobox/examples/combobox-select-only/
if (!isToggled.value && OPEN_KEYS.includes(evt.key)) { // simulate click to trigger dropdown open
if (NAVIGATION_KEYS.includes(evt.key)) {
evt.preventDefault()
}
// TODO: when it is opened by 'Home', 'End', 'ArrowUp', 'ArrowDown' keys, it should handle
// setting focus to different item
Comment on lines +467 to +468
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand this comment.

popperRef.value?.showPopover?.()
nextTick(() => selectItemsRef.value?.setFocus('current'))
}

if ((evt.key === 'ArrowDown' || evt.key === 'ArrowUp') && isToggled.value) {
// TODO: we need to remove this part since the popover should trap the focus
Copy link
Member

Choose a reason for hiding this comment

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

How do you then escape the popover?

// when it got opened, the focus should not be set on the input element.
if (NAVIGATION_KEYS.includes(evt.key) && isToggled.value) {
evt.preventDefault()
selectItemsRef.value?.setFocus(evt.key === 'ArrowDown' ? 'down' : 'up')
}
}
Expand Down Expand Up @@ -501,10 +513,6 @@ const onSelectWrapperClick = (event: Event): void => {
}
}

const onPopoverClick = (toggle: () => void) => {
toggle()
}

const onClose = (toggle: () => void, isToggled: boolean) => {
isDropdownOpen.value = false

Expand All @@ -518,6 +526,10 @@ const onClose = (toggle: () => void, isToggled: boolean) => {
if (isToggled) {
toggle()
}

nextTick(() => {
inputRef.value?.focus?.()
})
}

const onOpen = (toggle: () => void) => {
Expand Down
19 changes: 16 additions & 3 deletions src/components/KSelect/KSelectItems.vue
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
<template>
<div ref="itemsContainer">
<div
ref="itemsContainer"
>
Comment on lines +2 to +4
Copy link
Member

Choose a reason for hiding this comment

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

nit: this formatting change isn't necessary

<KSelectItem
v-for="item in nonGroupedItems"
:key="item.key"
Expand Down Expand Up @@ -89,7 +91,7 @@ const getGroupItems = (group: string) => items?.filter(item => item.group === gr

const itemsContainerRef = useTemplateRef('itemsContainer')

const moveItemFocus = (direction: 'down' | 'up'): void => {
const moveItemFocus = (direction: 'down' | 'up' | 'current'): void => {
Copy link
Member

Choose a reason for hiding this comment

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

current being a value for a direction param is a bit confusing?

const container = itemsContainerRef.value
if (!container) {
return
Expand All @@ -102,6 +104,11 @@ const moveItemFocus = (direction: 'down' | 'up'): void => {

const selectedItem = container.querySelector<HTMLButtonElement>(SELECTED_ITEM_SELECTOR)

if (selectedItem && direction === 'current') {
selectedItem.focus()
return
}

if (selectedItem) {
const index = [...items].indexOf(selectedItem)
items[direction === 'down' ? index + 1 : index - 1]?.focus()
Expand All @@ -110,7 +117,13 @@ const moveItemFocus = (direction: 'down' | 'up'): void => {
}
}

const onKeyPress = ({ target, key } : KeyboardEvent) => {
const onKeyPress = (e : KeyboardEvent) => {
const { target, key } = e
if (key === 'Enter') {
e.stopPropagation()
e.stopImmediatePropagation()
}
Copy link
Member

Choose a reason for hiding this comment

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

What would happen if we didn’t stop here? Might be worth adding a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


if (key === 'ArrowDown' || key === 'ArrowUp') {
// all selectable items
const selectableItems = itemsContainerRef.value?.querySelectorAll<HTMLButtonElement>(SELECTABLE_ITEM_SELECTOR)
Expand Down
Loading