Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(klabel, kselect, kmultiselect): a11y attributes [KHCP-111026] #2077

Merged
merged 6 commits into from
Mar 21, 2024
Merged

fix(klabel, kselect, kmultiselect): a11y attributes [KHCP-111026] #2077

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6ba83aa
fix(klabel): label tooltip aria-labelledby [KHCP-111026]
portikM Mar 20, 2024
cd5e59c
fix(kmultiselect): attribute bidning
portikM Mar 20, 2024
e2b72aa
fix(kselect, kmultiselect): aria-live attributes
portikM Mar 20, 2024
27985e8
fix(klabel): add aria-describedby
portikM Mar 21, 2024
32a1b69
docs(tooltip): tooltip id prop
portikM Mar 21, 2024
4fc29b4
Merge branch 'alpha' into fix/klabel-tooltip-aria-labelledby
portikM Mar 21, 2024
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
9 changes: 8 additions & 1 deletion src/components/KLabel/KLabel.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
<template>
<label
:id="labelId"
class="k-label"
:class="{ 'required': required }"
>
<slot />

<KTooltip
v-if="hasTooltip"
:aria-labelledby="labelId"
Copy link
Member

Choose a reason for hiding this comment

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

question: I'm not sure this makes sense in the context?

You have a label, which essentially provides info about another element, e.g. input.

This is then saying that the KTooltip is "labeled by the label"

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's providing accessible association between the tooltip and label. Otherwise it's impossible for screen readers to know the tooltip is somehow related to the label.

Copy link
Member

Choose a reason for hiding this comment

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

What I mean is I think the relationship here is backwards - the tooltip provides extra info about the label, not the other way around

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I get what you are saying but aria-labelledby needs to be set on an interactive elements - which in this case is the tooltip. Doing the other way around (setting it on label element) would be incorrect.

Copy link
Member

Choose a reason for hiding this comment

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

But isn't saying "the tooltip is described by the label" incorrect?

The tooltip is offering additional information within the label itself, about the label content

Copy link
Member Author

Choose a reason for hiding this comment

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

Labelled by, not described by - those are 2 different attributes. And that actually might be a good solution here - to set aria-describedby on label element to point at tooltip content.. only thing I'm not sure about is whether the two are interchangeable (if we did add aria-describedby on label would we still need to keep aria-labelledby on the tooltip). Let me do some reading and get back to you.

Copy link
Member

Choose a reason for hiding this comment

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

You should be able to drop labelledby; aria-attributes are additive, meaning since the Tooltip is contained within the label, it's inferred

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, updated now

v-bind="tooltipAttributes"
class="label-tooltip"
position-fixed
Expand All @@ -24,11 +27,12 @@

<script setup lang="ts">
import type { PropType } from 'vue'
import { computed, useSlots } from 'vue'
import { computed, useAttrs, useSlots } from 'vue'
import KTooltip from '@/components/KTooltip/KTooltip.vue'
import type { TooltipAttributes } from '@/types'
import { InfoIcon } from '@kong/icons'
import { KUI_COLOR_TEXT_NEUTRAL } from '@kong/design-tokens'
import { v4 as uuidv4 } from 'uuid'

const props = defineProps({
info: {
Expand Down Expand Up @@ -60,8 +64,11 @@ const props = defineProps({
})

const slots = useSlots()
const attrs = useAttrs()

const hasTooltip = computed((): boolean => !!(props.help || props.info || slots.tooltip))

const labelId = attrs.id ? String(attrs.id) : uuidv4()
</script>

<style lang="scss" scoped>
Expand Down
86 changes: 44 additions & 42 deletions src/components/KMultiselect/KMultiselect.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
v-if="label"
v-bind="labelAttributes"
:data-testid="labelAttributes['data-testid'] ? labelAttributes['data-testid'] : 'multiselect-label'"
:for="multiselectId"
:for="multiselectWrapperId"
:required="isRequired"
>
{{ strippedLabel }}
Expand Down Expand Up @@ -37,6 +37,7 @@
:id="multiselectWrapperId"
ref="multiselectElement"
class="multiselect-trigger"
v-bind="modifiedAttrs"
:class="{ focused: isFocused, hovered: isHovered, disabled: isDisabled, readonly: isReadonly }"
data-testid="multiselect-trigger"
role="listbox"
Expand All @@ -46,7 +47,6 @@
<div v-if="collapsedContext">
<KInput
:id="multiselectId"
v-bind="modifiedAttrs"
ref="multiselectInputElement"
autocapitalize="off"
autocomplete="off"
Expand Down Expand Up @@ -173,47 +173,49 @@
@update:model-value="onQueryChange"
/>
</div>
<KMultiselectItems
:items="sortedItems"
@selected="handleItemSelect"
>
<template #content="{ item }">
<slot
class="multiselect-item"
:item="item"
name="item-template"
/>
</template>
</KMultiselectItems>
<KMultiselectItem
v-if="enableItemCreation && uniqueFilterStr && !$slots.empty"
key="multiselect-add-item"
class="multiselect-add-item"
data-testid="multiselect-add-item"
:item="{ label: `${filterString} (Add new value)`, value: 'add_item' }"
@selected="handleAddItem"
<div aria-live="polite">
Copy link
Member Author

@portikM portikM Mar 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Note for PR review: the change here is adding a wrapper div with aria-live attribute

<KMultiselectItems
:items="sortedItems"
@selected="handleItemSelect"
>
<template #content="{ item }">
<slot
class="multiselect-item"
:item="item"
name="item-template"
/>
</template>
</KMultiselectItems>
<KMultiselectItem
v-if="enableItemCreation && uniqueFilterStr && !$slots.empty"
key="multiselect-add-item"
class="multiselect-add-item"
data-testid="multiselect-add-item"
:item="{ label: `${filterString} (Add new value)`, value: 'add_item' }"
@selected="handleAddItem"
>
<template #content>
<div class="select-item-description">
{{ filterString }}
<span class="select-item-new-indicator">(Add new value)</span>
</div>
</template>
</KMultiselectItem>
<KMultiselectItem
v-if="!sortedItems.length && !$slots.empty && !enableItemCreation"
key="multiselect-empty-item"
class="multiselect-empty-item"
data-testid="multiselect-empty-item"
:item="{ label: 'No results', value: 'no_results', disabled: true }"
/>
</div>
<div
v-if="$slots.empty && !loading && !sortedItems.length"
class="multiselect-empty"
data-propagate-clicks="false"
>
<template #content>
<div class="select-item-description">
{{ filterString }}
<span class="select-item-new-indicator">(Add new value)</span>
</div>
</template>
</KMultiselectItem>
<KMultiselectItem
v-if="!sortedItems.length && !$slots.empty && !enableItemCreation"
key="multiselect-empty-item"
class="multiselect-empty-item"
data-testid="multiselect-empty-item"
:item="{ label: 'No results', value: 'no_results', disabled: true }"
/>
</div>
<div
v-if="$slots.empty && !loading && !sortedItems.length"
class="multiselect-empty"
data-propagate-clicks="false"
>
<slot name="empty" />
<slot name="empty" />
</div>
</div>
<div
v-if="hasDropdownFooter"
Expand Down
114 changes: 58 additions & 56 deletions src/components/KSelect/KSelect.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,68 +101,70 @@
</Transition>
</div>
<template #content>
<div
v-if="enableFiltering && loading"
class="select-loading"
data-propagate-clicks="false"
data-testid="select-loading"
>
<slot name="loading">
<ProgressIcon class="loading-icon" />
</slot>
</div>
<div
v-else
class="select-items-container"
data-propagate-clicks="false"
>
<KSelectItems
:items="filteredItems"
@selected="handleItemSelect"
<div :aria-live="enableFiltering ? 'polite' : 'off'">
Copy link
Member Author

Choose a reason for hiding this comment

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

Note for PR review: the change here is adding a wrapper div with aria-live attribute

<div
v-if="enableFiltering && loading"
class="select-loading"
data-propagate-clicks="false"
data-testid="select-loading"
>
<template #content="{ item }">
<slot
class="select-item-label select-item-desc"
:item="item"
name="item-template"
/>
</template>
</KSelectItems>
<KSelectItem
v-if="!filteredItems.length && !$slots.empty && !enableItemCreation"
:item="{ label: 'No results', value: 'no_results', disabled: true }"
/>
<KSelectItem
v-if="!filteredItems.length && uniqueFilterQuery && !$slots.empty && enableItemCreation"
key="select-add-item"
class="select-add-item"
data-testid="select-add-item"
:item="{ label: `${filterQuery} (Add new value)`, value: 'add_item' }"
@selected="handleAddItem"
<slot name="loading">
<ProgressIcon class="loading-icon" />
</slot>
</div>
<div
v-else
class="select-items-container"
data-propagate-clicks="false"
>
<template #content>
<div class="select-item-description">
{{ filterQuery }}
<span class="select-item-new-indicator">(Add new value)</span>
</div>
</template>
</KSelectItem>
<KSelectItems
:items="filteredItems"
@selected="handleItemSelect"
>
<template #content="{ item }">
<slot
class="select-item-label select-item-desc"
:item="item"
name="item-template"
/>
</template>
</KSelectItems>
<KSelectItem
v-if="!filteredItems.length && !$slots.empty && !enableItemCreation"
:item="{ label: 'No results', value: 'no_results', disabled: true }"
/>
<KSelectItem
v-if="!filteredItems.length && uniqueFilterQuery && !$slots.empty && enableItemCreation"
key="select-add-item"
class="select-add-item"
data-testid="select-add-item"
:item="{ label: `${filterQuery} (Add new value)`, value: 'add_item' }"
@selected="handleAddItem"
>
<template #content>
<div class="select-item-description">
{{ filterQuery }}
<span class="select-item-new-indicator">(Add new value)</span>
</div>
</template>
</KSelectItem>
<div
v-if="hasDropdownFooter && dropdownFooterTextPosition === 'static'"
class="dropdown-footer dropdown-footer-static"
>
<slot name="dropdown-footer-text">
{{ dropdownFooterText }}
</slot>
</div>
</div>
<div
v-if="hasDropdownFooter && dropdownFooterTextPosition === 'static'"
class="dropdown-footer dropdown-footer-static"
v-if="!loading && !filteredItems.length && $slots.empty"
class="select-empty"
data-propagate-clicks="false"
>
<slot name="dropdown-footer-text">
{{ dropdownFooterText }}
</slot>
<slot name="empty" />
</div>
</div>
<div
v-if="!loading && !filteredItems.length && $slots.empty"
class="select-empty"
data-propagate-clicks="false"
>
<slot name="empty" />
</div>
<div
v-if="hasDropdownFooter && dropdownFooterTextPosition === 'sticky'"
class="dropdown-footer dropdown-footer-sticky"
Expand Down