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

Conversation

liamdebeasi
Copy link
Contributor

@liamdebeasi liamdebeasi commented Apr 22, 2024

Issue number: resolves #25221


What is the current behavior?

The wheel picker is hard to use with screen readers because users need to manually swipe over every option. Not only is this tedious, it also means it's impossible to move between columns/leave the picker by swiping only because the act of swiping selects a value.

What is the new behavior?

We did investigations into how native handles the wheel picker, and both iOS and Android do it differently. Unfortunately, neither of those patterns can be implemented exactly using web tech (at least not without a lot of brittle/hacky code).

As a result, we settled on a solution that follows web accessibility best practices and addresses the original issue:

  • Each picker column now renders an invisible element. This element receives focus and handles keyboard navigation. This element also has role="slider" which means that screen readers should synthesize keyboard events when swiping. This allows us to resolve the original issue. Users can also double tap to slide up/down to adjust values instead.

Note: Chrome currently does not synthesize keyboard events. However, this is something that could be fixed in the future (since Chrome is not following the spec right now). Android users can double tap to slide up/down in the meantime.

Does this introduce a breaking change?

  • Yes
  • No

Other information

Dev build: 8.0.0-dev.11713893672.15a2b23e

Reviews: Please test this on iOS and Android!

I recommend testing on src/components/datetime/test/prefer-wheel since it lets you the core functionality as well as the datetime-specific integrations (the aria labels)

Docs PR: ionic-team/ionic-docs#3612

Copy link

vercel bot commented Apr 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ionic-framework ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 24, 2024 2:43pm

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

Comment on lines 476 to 477
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.

@liamdebeasi liamdebeasi changed the title Fw 1891 fix(picker): make picker columns easier to select with assistive technology Apr 22, 2024
@@ -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.

@liamdebeasi liamdebeasi marked this pull request as ready for review April 23, 2024 18:16
@liamdebeasi liamdebeasi requested a review from a team as a code owner April 23, 2024 18:16
@liamdebeasi liamdebeasi changed the base branch from main to feature-8.1 April 23, 2024 20:01
@github-actions github-actions bot added package: angular @ionic/angular package package: vue @ionic/vue package package: react @ionic/react package labels Apr 23, 2024
@github-actions github-actions bot removed package: angular @ionic/angular package package: vue @ionic/vue package package: react @ionic/react package labels Apr 23, 2024
Copy link
Contributor

@sean-perkins sean-perkins left a comment

Choose a reason for hiding this comment

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

Moving into device testing next, small suggestion on a type adjustment for stricter types.

core/src/components/picker-column/picker-column.tsx Outdated Show resolved Hide resolved
core/src/components/picker-column/picker-column.tsx Outdated Show resolved Hide resolved
@@ -206,6 +215,10 @@ export class PickerColumn implements ComponentInterface {
}
}

connectedCallback() {
this.ariaLabel = this.el.getAttribute('aria-label') ?? 'Select a value';
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.

@@ -2045,6 +2051,7 @@ export class Datetime implements ComponentInterface {

return (
<ion-picker-column
aria-label="Select a day period"
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.

liamdebeasi and others added 2 commits April 24, 2024 10:39
Co-authored-by: Sean Perkins <13732623+sean-perkins@users.noreply.github.com>
@liamdebeasi liamdebeasi changed the title fix(picker): picker column is easier to select with assistive technology feat(picker): picker column is easier to select with assistive technology Apr 24, 2024
@liamdebeasi liamdebeasi merged commit e38e2e4 into feature-8.1 Apr 24, 2024
44 checks passed
@liamdebeasi liamdebeasi deleted the FW-1891 branch April 24, 2024 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: datetime month/year picker is hard to use with screen readers
2 participants