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

[EuiRange]: Add conditional aria-valuetext when ticks prop is passed #7675

Merged
merged 20 commits into from
Apr 16, 2024

Conversation

1Copenut
Copy link
Contributor

@1Copenut 1Copenut commented Apr 11, 2024

Summary

This PR closes #7613.

PR adds aria-valuenow attribute to <EuiRange /> component, and conditionally adds aria-valuetext when range sliders have tick marks. Users can extend the aria-valuetext attribute by passing an optional accessibleLabel string to the ticks array of objects now.

PR will be easiest to review by commit. I tried to keep each commit small and focused on one aspect of the changeset.

Video demonstrating the aria-valuenow and aria-valuetext when value and tick[value] line up.

EuiRangeSlider-ariavaluetext.mov

QA

Remove or strikethrough items that do not apply to your PR.

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA
  • Code quality checklist
  • Release checklist
    • A changelog entry exists and is marked appropriately.

Copy link
Contributor Author

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

Added thoughts to how I approached the helper functions, and why I went the way I did.

Comment on lines 119 to 154
handleAriaValueNow = (currentVal: string | number): number | undefined => {
if (!currentVal) return;

let ariaValueNow;
const target = Number(currentVal); // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/Number#return_value

if (!Number.isNaN(target)) {
ariaValueNow = target;
return ariaValueNow;
}

return ariaValueNow; // undefined
};

handleAriaValueText = (
ticks: EuiRangeTick[],
currentVal: string | number
): string | undefined => {
let ariaValueText;
const target = ticks.find(
(tick) => tick.value.toString() === currentVal.toString()
);

if (target && !target.accessibleLabel) {
ariaValueText = target.value.toString();
return ariaValueText;
}

if (target && target.accessibleLabel) {
ariaValueText = `${target.value.toString()}, (${target.accessibleLabel})`;
return ariaValueText;
}

return ariaValueText; // undefined
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both of these functions required some heady type coercion to get acceptable return values. aria-valuenow requires a number, aria-valuetext a string. I took what TypeScript was giving me, and used native methods and guard clauses to return early when a match was found.

If no match was found / NaN, then I returned undefined because there was nothing meaningful to populate the ARIA attributes with. And I tried to hew closely to the first rule of ARIA.

The first rule of ARIA is also why I opted not to include this functionality for EuiDualRange. The "thumbs" already announce their current position accurately and we have no true input[type="slider"] to hang the ARIA attributes on. We'd be introducing a potentially worse UX.

@1Copenut 1Copenut changed the title Feature/range tick ariavaluetext [EuiRange]: Add aria-valuenow and conditional aria-valuetext when ticks prop is passed Apr 11, 2024
src/components/form/range/range_slider.tsx Outdated Show resolved Hide resolved
src/components/form/range/range.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@mgadewoll mgadewoll left a comment

Choose a reason for hiding this comment

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

🚢 🐈‍⬛ Thanks for the changes and verification! LGTM 👍

@1Copenut
Copy link
Contributor Author

Buildkite test this

Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

I'd like the question in #7675 (comment) to be resolved first as I think that has a potential baseline improvement OOTB rather than requiring consumers to apply an extra prop, in addition to the cleanup comments left

@cee-chen
Copy link
Member

BTW, is the PR title still accurate? it looks like we're no longer applying aria-valuenow? (which is totally valid, just want to make sure I'm understanding the intention correctly!)

@1Copenut 1Copenut changed the title [EuiRange]: Add aria-valuenow and conditional aria-valuetext when ticks prop is passed [EuiRange]: Add conditional aria-valuetext when ticks prop is passed Apr 12, 2024
@1Copenut
Copy link
Contributor Author

I just updated the PR title to represent that we are only adding aria-valuetext now.

@1Copenut 1Copenut requested a review from cee-chen April 15, 2024 19:34
{ label: '8GB', value: 42, accessibleLabel: 'eight gigabytes' },
{ label: '16GB', value: 56, accessibleLabel: 'sixteen gigabytes' },
{ label: '32GB', value: 70, accessibleLabel: 'thirty-two gigabytes' },
{ label: '64GB', value: 84, accessibleLabel: 'sixty-four gigabytes' },
Copy link
Member

Choose a reason for hiding this comment

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

😍 Now this is an example that makes sense to me! Huzzah!

Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com>
Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com>
1Copenut and others added 2 commits April 16, 2024 11:55
This is more clear and succinct. Using it exactly as is.

Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com>
Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com>
Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Thanks so much for going through feedback rounds with me Trevor! This feels really really solid in terms of feature and code! 🎉

@1Copenut
Copy link
Contributor Author

Buildkite test this

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @1Copenut

@1Copenut 1Copenut merged commit 4c68787 into elastic:main Apr 16, 2024
7 checks passed
@1Copenut 1Copenut deleted the feature/range-tick-ariavaluetext branch April 16, 2024 19:23
cee-chen added a commit to elastic/kibana that referenced this pull request May 3, 2024
`v94.1.0-backport.0` ⏩ `v94.2.1-backport.0`

_[Questions? Please see our Kibana upgrade
FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_

---

##
[`v94.2.1-backport.0`](https://github.com/elastic/eui/releases/v94.2.1-backport.0)

**This is a backport release only intended for use by Kibana.**

- Reverted the `EuiFlexGroup`/`EuiFlexItem` `component` prop feature due
to Kibana typing issues

## [`v94.2.1`](https://github.com/elastic/eui/releases/v94.2.1)

**Bug fixes**

- Fixed an `EuiTabbedContent` edge case bug that occurred when updated
with a completely different set of `tabs`
([#7713](elastic/eui#7713))
- Fixed the `@storybook/test` dependency to be listed in
`devDependencies` and not `dependencies`
([#7719](elastic/eui#7719))

## [`v94.2.0`](https://github.com/elastic/eui/releases/v94.2.0)

- Updated `getDefaultEuiMarkdownPlugins()` to allow excluding the
following plugins in addition to `tooltip`:
([#7676](elastic/eui#7676))
  - `checkbox`
  - `linkValidator`
  - `lineBreaks`
  - `emoji`
- Updated `EuiSelectable`'s `isPreFiltered` prop to allow passing a
configuration object, which allows disabling search highlighting in
addition to search filtering
([#7683](elastic/eui#7683))
- Updated `EuiFlexGroup` and `EuiFlexItem` prop types to support passing
any valid React component type to the `component` prop and ensure proper
type checking of the extra props forwarded to the `component`.
([#7688](elastic/eui#7688))
- Updated `EuiSearchBar` to allow the `@` special character in query
string searches ([#7702](elastic/eui#7702))
- Added a new, optional `optionMatcher` prop to `EuiSelectable` and
`EuiComboBox` allowing passing a custom option matcher function to these
components and controlling option filtering for given search string
([#7709](elastic/eui#7709))

**Bug fixes**

- Fixed an `EuiPageTemplate` bug where prop updates would not cascade
down to child sections
([#7648](elastic/eui#7648))
- To cascade props down to the sidebar, `EuiPageTemplate` now explicitly
requires using the `EuiPageTemplate.Sidebar` rather than
`EuiPageSidebar`
- Fixed `EuiFieldNumber`'s typing to accept an icon configuration shape
([#7666](elastic/eui#7666))
- Fixed `EuiFieldText` and `EuiFieldNumber` to render the correct
paddings for icon shapes set to `side: 'right'`
([#7666](elastic/eui#7666))
- Fixed `EuiFieldText` and `EuiFieldNumber` to fully ignore
`icon`/`prepend`/`append` when `controlOnly` is set to true
([#7666](elastic/eui#7666))
- Fixed `EuiColorPicker`'s input not setting the correct right padding
for the number of icons displayed
([#7666](elastic/eui#7666))
- Visual fixes for `EuiRange`s with `showInput`:
([#7678](elastic/eui#7678))
  - Longer `append`/`prepend` labels no longer cause a background bug
  - Inputs can no longer overwhelm the actual range in width
- Fixed a visual text alignment regression in `EuiTableRowCell`s with
the `row` header scope
([#7681](elastic/eui#7681))
- Fixed `toolTipProps` type on `EuiSuperUpdateButton` to use
`Partial<EuiToolTipProps>`
([#7692](elastic/eui#7692))
- Fixes missing prop type for `popperProps` on `EuiDatePicker`
([#7694](elastic/eui#7694))
- Fixed a focus bug with `EuiDataGrid`s with `leadingControlColumns`
when moving columns to the left/right
([#7701](elastic/eui#7701))
([#7698](elastic/eui#7698))
- Fixed `EuiSuperDatePicker` to validate date string with respect of
locale on `EuiAbsoluteTab`.
([#7705](elastic/eui#7705))
- Fixed a visual bug with `EuiSuperDatePicker`'s absolute tab on small
mobile screens ([#7708](elastic/eui#7708))
- Fixed i18n of empty and loading state messages for the
`FieldValueSelectionFilter` component
([#7718](elastic/eui#7718))

**Dependency updates**

- Updated `@hello-pangea/dnd` to v16.6.0
([#7599](elastic/eui#7599))
- Updated `remark-rehype` to v8.1.0
([#7601](elastic/eui#7601))

**Accessibility**

- Improved `EuiBasicTable` and `EuiInMemoryTable`'s selection checkboxes
to have unique aria-labels per row
([#7672](elastic/eui#7672))
- Added `aria-valuetext` attributes to `EuiRange`s with tick labels for
improved screen reader UX
([#7675](elastic/eui#7675))
- Updated `EuiAccordion` to keep focus on accordion trigger instead of
moving to content on click/keypress
([#7696](elastic/eui#7696))
- Added `aria-disabled` attribute to `EuiHorizontalSteps` when status is
"disabled" ([#7699](elastic/eui#7699))

---------

Co-authored-by: Tomasz Kajtoch <tomasz.kajtoch@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EuiRange] Add an aria-valuetext prop so screen readers can announce label instead of value.
5 participants