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 28 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
7 changes: 7 additions & 0 deletions core/src/components/datetime/datetime.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1687,6 +1687,7 @@ export class Datetime implements ComponentInterface {

return (
<ion-picker-column
aria-label="Select a date"
class="date-column"
color={this.color}
disabled={disabled}
Expand Down Expand Up @@ -1806,6 +1807,7 @@ export class Datetime implements ComponentInterface {

return (
<ion-picker-column
aria-label="Select a day"
class="day-column"
color={this.color}
disabled={disabled}
Expand Down Expand Up @@ -1849,6 +1851,7 @@ export class Datetime implements ComponentInterface {

return (
<ion-picker-column
aria-label="Select a month"
class="month-column"
color={this.color}
disabled={disabled}
Expand Down Expand Up @@ -1891,6 +1894,7 @@ export class Datetime implements ComponentInterface {

return (
<ion-picker-column
aria-label="Select a year"
class="year-column"
color={this.color}
disabled={disabled}
Expand Down Expand Up @@ -1964,6 +1968,7 @@ export class Datetime implements ComponentInterface {

return (
<ion-picker-column
aria-label="Select an hour"
color={this.color}
disabled={disabled}
value={activePart.hour}
Expand Down Expand Up @@ -2003,6 +2008,7 @@ export class Datetime implements ComponentInterface {

return (
<ion-picker-column
aria-label="Select a minute"
color={this.color}
disabled={disabled}
value={activePart.minute}
Expand Down Expand Up @@ -2045,6 +2051,7 @@ export class Datetime implements ComponentInterface {

return (
<ion-picker-column
aria-label="Select a day period"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open to other ideas for naming here. Wasn't sure what else to call the AM/PM picker

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with the naming here.

style={isDayPeriodRTL ? { order: '-1' } : {}}
color={this.color}
disabled={disabled}
Expand Down
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
186 changes: 185 additions & 1 deletion core/src/components/picker-column/picker-column.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,15 @@ export class PickerColumn implements ComponentInterface {
private isColumnVisible = false;
private parentEl?: HTMLIonPickerElement | null;
private canExitInputMode = true;
private assistiveFocusable?: HTMLElement;
private updateValueTextOnScroll = false;

@State() ariaLabel: string | null = null;

@Watch('aria-label')
ariaLabelChanged(newValue: string) {
this.ariaLabel = newValue;
}

@State() isActive = 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 All @@ -222,6 +235,7 @@ export class PickerColumn implements ComponentInterface {
* of these can cause a scroll to occur.
*/
this.canExitInputMode = canExitInputMode;
this.updateValueTextOnScroll = false;
scrollEl.scroll({
top,
left: 0,
Expand Down Expand Up @@ -396,8 +410,24 @@ export class PickerColumn implements ComponentInterface {
activeEl = newActiveElement;
this.setPickerItemActiveState(newActiveElement, true);

/**
* Set the aria-valuetext even though the value prop has not been updated yet.
* This enables some screen readers to announce the value as the users drag
* as opposed to when their release their pointer from the screen.
*
* When the value is programmatically updated, we will smoothly scroll
* to the new option. However, we do not want to update aria-valuetext mid-scroll
* as that can cause the old value to be briefly set before being set to the
* correct option. This will cause some screen readers to announce the old value
* again before announcing the new value. The correct valuetext will be set on render.
*/
if (this.updateValueTextOnScroll) {
this.assistiveFocusable?.setAttribute('aria-valuetext', this.getOptionValueText(newActiveElement));
}

timeout = setTimeout(() => {
this.isScrolling = false;
this.updateValueTextOnScroll = true;
enableHaptics && hapticSelectionEnd();

/**
Expand Down Expand Up @@ -481,6 +511,159 @@ export class PickerColumn implements ComponentInterface {
});
}

/**
* 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;
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;
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) => {
/**
* The below operations should be inverted when running on a mobile device.
* For example, swiping up will dispatch an "ArrowUp" event. On desktop,
* this should cause the previous option to be selected. On mobile, swiping
* up causes a view to scroll down. As a result, swiping up on mobile should
* cause the next option to be selected. The Home/End operations remain
* unchanged because those always represent the first/last options, respectively.
*/
const mobile = isPlatform('mobile');
let newOption = null;
liamdebeasi marked this conversation as resolved.
Show resolved Hide resolved
switch (ev.key) {
case 'ArrowDown':
newOption = mobile ? this.findPreviousOption() : this.findNextOption();
break;
case 'ArrowUp':
newOption = mobile ? this.findNextOption() : this.findPreviousOption();
break;
case 'PageUp':
newOption = mobile ? this.findNextOption(5) : this.findPreviousOption(5);
break;
case 'PageDown':
newOption = mobile ? this.findPreviousOption(5) : 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();
}
};

/**
* Utility to generate the correct text for aria-valuetext.
*/
private getOptionValueText = (el?: HTMLIonPickerColumnOptionElement) => {
return el ? el.getAttribute('aria-label') ?? el.innerText : '';
};

/**
* 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 } = this;
const valueText = this.getOptionValueText(activeItem);

/**
* When using the picker, the valuetext provides important context that valuenow
* does not. Additionally, using non-zero valuemin/valuemax values can cause
* WebKit to incorrectly announce numeric valuetext values (such as a year
* like "2024") as percentages: https://bugs.webkit.org/show_bug.cgi?id=273126
*/
return (
<div
ref={(el) => (this.assistiveFocusable = el)}
class="assistive-focusable"
role="slider"
tabindex={this.disabled ? undefined : 0}
aria-label={this.ariaLabel}
aria-valuemin={0}
aria-valuemax={0}
aria-valuenow={0}
Comment on lines +657 to +659
Copy link
Contributor Author

@liamdebeasi liamdebeasi Apr 23, 2024

Choose a reason for hiding this comment

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

I tested VoiceOver, TalkBack, and NVDA. None of them announce these values if aria-valuetext is set. Setting these all to zero also avoids the WebKit bug noted.

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 +677,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