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.

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

Merged
merged 30 commits into from
Apr 24, 2024
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
4be5ab8
feat(picker): add stubbed assistive cover implementation
liamdebeasi Apr 16, 2024
a71f009
feat: implement arrow up, down, home, end, and aria label
liamdebeasi Apr 16, 2024
0224617
fix: do not focus underlying scroll container
liamdebeasi Apr 16, 2024
77502c8
fix: remove focus ring
liamdebeasi Apr 16, 2024
8ca4fe6
fix: old div no longer receives keyboard focus
liamdebeasi Apr 22, 2024
004ee5f
fix: valuetext uses label from active option
liamdebeasi Apr 22, 2024
b460f31
feat: implement pageup/pagedown behaviors
liamdebeasi Apr 22, 2024
a5a7a4d
remove log
liamdebeasi Apr 22, 2024
0359786
add correct max count
liamdebeasi Apr 22, 2024
36271d7
add comments
liamdebeasi Apr 22, 2024
5dbad1a
more comments
liamdebeasi Apr 22, 2024
98b6a92
use single selector
liamdebeasi Apr 22, 2024
aa8f2b6
strict null checks
liamdebeasi Apr 22, 2024
5fcee5d
init var to null
liamdebeasi Apr 22, 2024
cd70c0f
get valuenow without extra iteration
liamdebeasi Apr 22, 2024
8630ccc
fix: max count is correct
liamdebeasi Apr 22, 2024
a2d6685
add attribute tests
liamdebeasi Apr 22, 2024
bf6b621
remove debug background
liamdebeasi Apr 22, 2024
aabd368
add accessible labels for datetime picker integration
liamdebeasi Apr 22, 2024
d3fd294
fix: announce values as uses slides
liamdebeasi Apr 23, 2024
0917f0e
revert refactor, fix ios behavior
liamdebeasi Apr 23, 2024
251c2fd
typo
liamdebeasi Apr 23, 2024
cddae95
fix: nvda does not announce wrong valuetext value
liamdebeasi Apr 23, 2024
c0c62c9
remove unneeded tests
liamdebeasi Apr 23, 2024
5a2b23e
remove log
liamdebeasi Apr 23, 2024
7139d44
improve behavior on mobile
liamdebeasi Apr 23, 2024
33bd32d
lint
liamdebeasi Apr 23, 2024
c6b8b87
Merge remote-tracking branch 'origin/feature-8.1' into FW-1891
liamdebeasi Apr 23, 2024
5a213d2
Update core/src/components/picker-column/picker-column.tsx
liamdebeasi Apr 24, 2024
294d771
chore: remove redundant type
liamdebeasi Apr 24, 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
18 changes: 18 additions & 0 deletions core/src/components/picker-column/picker-column.scss
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

:host {
display: flex;
position: relative;

align-items: center;

Expand All @@ -19,6 +20,23 @@
text-align: center;
}

/**
* Renders an invisible element on top of the column that receives focus
* events. This allows screen readers to navigate the column.
*/
.assistive-focusable {
@include position(0, 0, 0, 0);
position: absolute;

z-index: 1;
pointer-events: none;
}

// Hide the focus ring since screen readers will show their own
.assistive-focusable:focus {
outline: none;
}

.picker-opts {
/**
* This padding must be set here and not on the
Expand Down
177 changes: 165 additions & 12 deletions core/src/components/picker-column/picker-column.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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');
Expand All @@ -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);
Expand Down Expand Up @@ -206,6 +215,10 @@ export class PickerColumn implements ComponentInterface {
}
}

connectedCallback() {
this.ariaLabel = this.el.getAttribute('aria-label') ?? 'Select a value';
Copy link
Contributor Author

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.

Copy link
Contributor

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.

}

private centerPickerItemInView = (target: HTMLElement, smooth = true, canExitInputMode = true) => {
const { isColumnVisible, scrollEl } = this;

Expand Down Expand Up @@ -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(() => {
Expand Down Expand Up @@ -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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
Expand All @@ -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;
}}
Expand Down
Loading
Loading