-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
feat(picker): picker column is easier to select with assistive technology #29371
Changes from 18 commits
4be5ab8
a71f009
0224617
77502c8
8ca4fe6
004ee5f
b460f31
a5a7a4d
0359786
36271d7
5dbad1a
98b6a92
aa8f2b6
5fcee5d
cd70c0f
8630ccc
a2d6685
bf6b621
aabd368
d3fd294
0917f0e
251c2fd
cddae95
c0c62c9
5a2b23e
7139d44
33bd32d
c6b8b87
5a213d2
294d771
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 |
---|---|---|
|
@@ -32,6 +32,13 @@ export class PickerColumn implements ComponentInterface { | |
private parentEl?: HTMLIonPickerElement | null; | ||
private canExitInputMode = true; | ||
|
||
@State() ariaLabel: string | null = null; | ||
|
||
@Watch('aria-label') | ||
ariaLabelChanged(newValue: string) { | ||
this.ariaLabel = newValue; | ||
} | ||
|
||
@State() isActive = false; | ||
|
||
@Element() el!: HTMLIonPickerColumnElement; | ||
|
@@ -103,7 +110,8 @@ export class PickerColumn implements ComponentInterface { | |
const ev = entries[entries.length - 1]; | ||
|
||
if (ev.isIntersecting) { | ||
const { activeItem, el } = this; | ||
const { activeItem } = this.activeItem; | ||
const { el } = this; | ||
|
||
this.isColumnVisible = true; | ||
|
||
|
@@ -150,7 +158,8 @@ export class PickerColumn implements ComponentInterface { | |
} | ||
|
||
componentDidRender() { | ||
const { el, activeItem, isColumnVisible, value } = this; | ||
const { activeItem } = this.activeItem; | ||
const { el, isColumnVisible, value } = this; | ||
|
||
if (isColumnVisible && !activeItem) { | ||
const firstOption = el.querySelector('ion-picker-column-option'); | ||
|
@@ -171,7 +180,7 @@ export class PickerColumn implements ComponentInterface { | |
/** @internal */ | ||
@Method() | ||
async scrollActiveItemIntoView(smooth = false) { | ||
const activeEl = this.activeItem; | ||
const { activeItem: activeEl } = this.activeItem; | ||
|
||
if (activeEl) { | ||
this.centerPickerItemInView(activeEl, smooth, false); | ||
|
@@ -206,6 +215,10 @@ export class PickerColumn implements ComponentInterface { | |
} | ||
} | ||
|
||
connectedCallback() { | ||
this.ariaLabel = this.el.getAttribute('aria-label') ?? 'Select a value'; | ||
} | ||
|
||
private centerPickerItemInView = (target: HTMLElement, smooth = true, canExitInputMode = true) => { | ||
const { isColumnVisible, scrollEl } = this; | ||
|
||
|
@@ -300,7 +313,7 @@ export class PickerColumn implements ComponentInterface { | |
const { el, scrollEl } = this; | ||
|
||
let timeout: ReturnType<typeof setTimeout> | undefined; | ||
let activeEl: HTMLIonPickerColumnOptionElement | undefined = this.activeItem; | ||
let { activeItem: activeEl } = this.activeItem; | ||
|
||
const scrollCallback = () => { | ||
raf(() => { | ||
|
@@ -465,22 +478,161 @@ export class PickerColumn implements ComponentInterface { | |
this.el.classList.remove('picker-column-active'); | ||
}; | ||
|
||
get activeItem() { | ||
get activeItem(): { | ||
activeItem?: HTMLIonPickerColumnOptionElement; | ||
activeItemIndex: number; | ||
} { | ||
const { value } = this; | ||
const options = Array.from(this.el.querySelectorAll<HTMLIonPickerColumnOptionElement>('ion-picker-column-option')); | ||
return options.find((option) => { | ||
const options = this.el.querySelectorAll<HTMLIonPickerColumnOptionElement>('ion-picker-column-option'); | ||
|
||
for (let i = 0; i <= options.length - 1; i++) { | ||
const option = options[i]; | ||
|
||
/** | ||
* If the whole picker column is disabled, the current value should appear active | ||
* If the current value item is specifically disabled, it should not appear active | ||
*/ | ||
if (!this.disabled && option.disabled) { | ||
return false; | ||
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. My understanding is this if statement had more checks than it needed to. According to the comment, the picker column's disabled state has no bearing on whether or not an option should be rendered as active, so we don't need to check it. |
||
if (option.value === value && !option.disabled) { | ||
return { | ||
activeItem: option, | ||
activeItemIndex: i, | ||
}; | ||
} | ||
} | ||
|
||
return option.value === value; | ||
}); | ||
return { activeItem: undefined, activeItemIndex: -1 }; | ||
} | ||
|
||
/** | ||
* Find the next enabled option after the active option. | ||
* @param stride - How many options to "jump" over in order to select the next option. | ||
* This can be used to implement PageUp/PageDown behaviors where pressing these keys | ||
* scrolls the picker by more than 1 option. For example, a stride of 5 means select | ||
* the enabled option 5 options after the active one. Note that the actual option selected | ||
* may be past the stride if the option at the stride is disabled. | ||
*/ | ||
private findNextOption = (stride: number = 1) => { | ||
liamdebeasi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const { activeItem } = this.activeItem; | ||
if (!activeItem) return null; | ||
|
||
let prevNode = activeItem; | ||
let node = activeItem.nextElementSibling as HTMLIonPickerColumnOptionElement | null; | ||
while (node != null) { | ||
if (stride > 0) { | ||
stride--; | ||
} | ||
|
||
if (node.tagName === 'ION-PICKER-COLUMN-OPTION' && !node.disabled && stride === 0) { | ||
return node; | ||
} | ||
prevNode = node; | ||
|
||
// Use nextElementSibling instead of nextSibling to avoid text/comment nodes | ||
node = node.nextElementSibling as HTMLIonPickerColumnOptionElement | null; | ||
} | ||
|
||
return prevNode; | ||
}; | ||
|
||
/** | ||
* Find the next enabled option after the active option. | ||
* @param stride - How many options to "jump" over in order to select the next option. | ||
* This can be used to implement PageUp/PageDown behaviors where pressing these keys | ||
* scrolls the picker by more than 1 option. For example, a stride of 5 means select | ||
* the enabled option 5 options before the active one. Note that the actual option selected | ||
* may be past the stride if the option at the stride is disabled. | ||
*/ | ||
private findPreviousOption = (stride: number = 1) => { | ||
const { activeItem } = this.activeItem; | ||
if (!activeItem) return null; | ||
|
||
let nextNode = activeItem; | ||
let node = activeItem.previousElementSibling as HTMLIonPickerColumnOptionElement | null; | ||
while (node != null) { | ||
if (stride > 0) { | ||
stride--; | ||
} | ||
|
||
if (node.tagName === 'ION-PICKER-COLUMN-OPTION' && !node.disabled && stride === 0) { | ||
return node; | ||
} | ||
|
||
nextNode = node; | ||
|
||
// Use previousElementSibling instead of previousSibling to avoid text/comment nodes | ||
node = node.previousElementSibling as HTMLIonPickerColumnOptionElement | null; | ||
} | ||
|
||
return nextNode; | ||
}; | ||
|
||
private onKeyDown = (ev: KeyboardEvent) => { | ||
let newOption = null; | ||
liamdebeasi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
switch (ev.key) { | ||
case 'ArrowDown': | ||
newOption = this.findNextOption(); | ||
break; | ||
case 'ArrowUp': | ||
newOption = this.findPreviousOption(); | ||
break; | ||
case 'PageUp': | ||
newOption = this.findPreviousOption(5); | ||
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. A stride of 5 was based on how browsers handle PageUp/PageDown in scroll snapping containers |
||
break; | ||
case 'PageDown': | ||
newOption = this.findNextOption(5); | ||
break; | ||
case 'Home': | ||
/** | ||
* There is no guarantee that the first child will be an ion-picker-column-option, | ||
* so we do not use firstElementChild. | ||
*/ | ||
newOption = this.el.querySelector<HTMLIonPickerColumnOptionElement>('ion-picker-column-option:first-of-type'); | ||
break; | ||
case 'End': | ||
/** | ||
* There is no guarantee that the last child will be an ion-picker-column-option, | ||
* so we do not use lastElementChild. | ||
*/ | ||
newOption = this.el.querySelector<HTMLIonPickerColumnOptionElement>('ion-picker-column-option:last-of-type'); | ||
break; | ||
default: | ||
break; | ||
} | ||
|
||
if (newOption !== null) { | ||
this.value = newOption.value; | ||
|
||
// This stops any default browser behavior such as scrolling | ||
ev.preventDefault(); | ||
} | ||
}; | ||
|
||
/** | ||
* Render an element that overlays the column. This element is for assistive | ||
* tech to allow users to navigate the column up/down. This element should receive | ||
* focus as it listens for synthesized keyboard events as required by the | ||
* slider role: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/slider_role | ||
*/ | ||
private renderAssistiveFocusable = () => { | ||
const { activeItem, activeItemIndex } = this.activeItem; | ||
const { el } = this; | ||
const valueText = activeItem ? activeItem.getAttribute('aria-label') ?? activeItem.innerText : ''; | ||
return ( | ||
<div | ||
class="assistive-focusable" | ||
role="slider" | ||
tabindex={this.disabled ? undefined : 0} | ||
aria-label={this.ariaLabel} | ||
aria-valuemin={0} | ||
aria-valuemax={el.childElementCount - 1} | ||
aria-valuenow={activeItemIndex >= 0 ? activeItemIndex : 0} | ||
aria-valuetext={valueText} | ||
aria-orientation="vertical" | ||
onKeyDown={(ev) => this.onKeyDown(ev)} | ||
></div> | ||
); | ||
}; | ||
|
||
render() { | ||
const { color, disabled, isActive, numericInput } = this; | ||
const mode = getIonMode(this); | ||
|
@@ -494,10 +646,11 @@ export class PickerColumn implements ComponentInterface { | |
['picker-column-disabled']: disabled, | ||
})} | ||
> | ||
{this.renderAssistiveFocusable()} | ||
<slot name="prefix"></slot> | ||
<div | ||
aria-hidden="true" | ||
class="picker-opts" | ||
tabindex={disabled ? undefined : 0} | ||
ref={(el) => { | ||
this.scrollEl = el; | ||
}} | ||
|
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.
I could be convinced to not have a default value here since the label isn't super helpful. However, I was concerned about breaking the existing experience since not using an
aria-label
on the slider role could cause tools such as Axe to start failing.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.
I think a label is better than no label here. It is also incredibly straightforward for developers to customize it.