Skip to content

Conversation

@mgadewoll
Copy link
Contributor

@mgadewoll mgadewoll commented Apr 25, 2025

Summary

closes #8446

This PR updates EuiColorPicker to prevent duplicate output for screen readers when selecting a color using the saturation or hue selection elements.

Additionally this PR adds visual-only tooltips to the saturation and hue slider elements (to provide a visual label for the selected color) and updates some accessible labels to more them more descriptive.

display: default (in a popover)

Screen.Recording.2025-04-25.at.11.20.09.mov

display: inline

Screen.Recording.2025-04-25.at.11.18.57.mov

QA

Note

When testing the saturation element in NVDA, you need to manually switch to focus mode (Insert + Space) on the thumb button element to be able to use the arrow keys for selection. Focusing the button only adds the browse mode cursor with which the arrow keys are used to navigate DOM elements.
We might want to check on this in a further update, using an input element for example would ensure that focus mode is automatically entered upon focusing it.

  • verify that there is no duplicate output when selecting colors using screen readers
  • verify there is no visual or functional regression

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in both MacOS and Windows high contrast 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.
    • If applicable, added the breaking change issue label (and filled out the breaking change checklist)
  • Designer checklist
    • If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)

- moves the aria-live element up to the shared parent comonent
…bleScreenReaderOutput

- the idea is that for screen readers this tooltip is not actually there, so we probably don't want it to add an additional step when pressing the escape key to close something
- change is caused by 1px offset due to the EuiScreenReaderOnly element; weirdly enough this 1px offset is only applied in VRT and not actually in a real render as expected as it's positioned absolute
@mgadewoll mgadewoll self-assigned this Apr 25, 2025
@mgadewoll mgadewoll marked this pull request as ready for review April 25, 2025 12:40
@mgadewoll mgadewoll requested a review from a team as a code owner April 25, 2025 12:40
@weronikaolejniczak weronikaolejniczak self-requested a review April 25, 2025 14:45
onKeyDown={handleOnKeyDown}
/>
<EuiScreenReaderOnly>
{/* Note: using EuiScreenReaderLive didn't work as expected for VoiceOver */}
Copy link
Contributor

Choose a reason for hiding this comment

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

non-blocking question:

Is this note necessary here? Is the usage not working for VO specific for this component?

If not, and EuiScreenReaderOnly doesn't work globally for VO, I would add a note there (e.g. packages/eui/src/components/accessibility/screen_reader_only/screen_reader_only.tsx) and remove this comment.

Copy link
Contributor Author

@mgadewoll mgadewoll Apr 28, 2025

Choose a reason for hiding this comment

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

The intention for the comment here was to ensure we don't update to EuiScreenReaderLive in the future and break it 😅 I have not verified if that's a global issue so far, as it was outside of the scope for this issue.

If you feel the comment is redundant, we can remove it. It's just meant as a safeguard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that I think about it: I actually did look at EuiScreenReaderLive in VO as part of a different request last week and it worked as expected. So it might not be EuiScreenReaderLive specific but the combination of EuiScreenReaderOnly and EuiScreenReaderLive

Screen.Recording.2025-04-24.at.11.44.11.mov

Copy link
Contributor

@weronikaolejniczak weronikaolejniczak left a comment

Choose a reason for hiding this comment

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

There's a teeny tiny difference in the VRT images, the new ones have a 1px space from the top, not sure if that's on purpose or if we should remove to be pixel-perfect? In any case, it works and looks good to me 👌🏻 Great job!

Testing notes

This could be improved: "Color selection dialog dialog. You are in a dialog"

But for the scope of the task, all colors have labels and are not duplicated. Great job! 🥳 🟢

NVDA + Firefox (Win)

NVDA.Firefox.mp4

NVDA + Chrome (Win)

NVDA.Chrome.mp4

JAWS + Firefox (Win) 🔈

JAWS.Firefox.mp4

JAWS + Chrome (Win) 🔈

JAWS.Chrome.mp4

VoiceOver + Safari (Mac)

Screen.Recording.2025-04-28.at.11.39.41.mov

Side-note: There's a Polish letter ń that you make using Alt + N which happens to be the shortcut to launch NVDA. Pretty annoying when you're writing sth to a friend and suddenly a male voice comes on "Welcome to NVDA dialog" 💀

@mgadewoll
Copy link
Contributor Author

mgadewoll commented Apr 28, 2025

There's a teeny tiny difference in the VRT images, the new ones have a 1px space from the top, not sure if that's on purpose or if we should remove to be pixel-perfect? In any case, it works and looks good to me

@weronikaolejniczak Oh right, I only explained it as commit comment, but forgot to add a real PR comment as well 😅
The 1px comes from the EuiScreenReaderOnly element that has 1px dimensions which should not actually be an issue as the element is positioned absolute but it applies in VRT anyway somehow 🤷‍♀️

Screenshot 2025-04-28 at 12 09 42

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @mgadewoll

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @mgadewoll

@mgadewoll mgadewoll merged commit 8fce7a4 into elastic:main Apr 29, 2025
5 checks passed
mgadewoll added a commit to elastic/kibana that referenced this pull request May 6, 2025
`102.0.0` ⏩ `102.1.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)

>[!NOTE]
There is also the sibling PR for `8.19` with old Amsterdam theme ready
for review [here](#220049). It
contains the same changes.

## Changes

- Updated test selector (due to changed tooltip placement in
[#8644](elastic/eui#8644))
- snapshot updates

## Package updates

### `@elastic/eui`

#### [`v102.1.0`](https://github.com/elastic/eui/releases/v102.1.0)

- Update `EuiDataGrid` to use `expand` glyph
([#8646](elastic/eui#8646))

**Accessibility**

- Updated `EuiTableHeaderCell` to output `nameTooltip` directly on
sortable cell elements, ensuring tooltips appear on focus
([#8644](elastic/eui#8644))
- Improved the accessibility of `EuiColorPicker` by:
([#8639](elastic/eui#8639))
  - preventing duplicate color output for screen readers
- adding tooltips with visual color labels for the selected colors on
the saturation and hue sliders
  - updated accessible labels and announcements to be more descriptive

**Dependency updates**

- Updated `typescript` to v5.8.3
([#8626](elastic/eui#8626))
mgadewoll added a commit to elastic/kibana that referenced this pull request May 6, 2025
`102.0.0` ⏩ `102.1.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)

>[!IMPORTANT]
This PR is a direct sibling to this [upgrade
PR](#220039) to `main`. The
difference is that it adds a standalone EUI package with the previous
"Amsterdam" theme.
Apart from the theme difference, **there are no further changes added**.

## Changes

- Updated test selector (due to changed tooltip placement in
[#8644](elastic/eui#8644))
- snapshot updates

## Package updates

### `@elastic/eui`

#### [`v102.1.0`](https://github.com/elastic/eui/releases/v102.1.0)

- Update `EuiDataGrid` to use `expand` glyph
([#8646](elastic/eui#8646))

**Accessibility**

- Updated `EuiTableHeaderCell` to output `nameTooltip` directly on
sortable cell elements, ensuring tooltips appear on focus
([#8644](elastic/eui#8644))
- Improved the accessibility of `EuiColorPicker` by:
([#8639](elastic/eui#8639))
  - preventing duplicate color output for screen readers
- adding tooltips with visual color labels for the selected colors on
the saturation and hue sliders
  - updated accessible labels and announcements to be more descriptive

**Dependency updates**

- Updated `typescript` to v5.8.3
([#8626](elastic/eui#8626))
mgadewoll added a commit to mgadewoll/kibana that referenced this pull request May 6, 2025
`102.0.0` ⏩ `102.1.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)

>[!NOTE]
There is also the sibling PR for `8.19` with old Amsterdam theme ready
for review [here](elastic#220049). It
contains the same changes.

## Changes

- Updated test selector (due to changed tooltip placement in
[elastic#8644](elastic/eui#8644))
- snapshot updates

## Package updates

### `@elastic/eui`

#### [`v102.1.0`](https://github.com/elastic/eui/releases/v102.1.0)

- Update `EuiDataGrid` to use `expand` glyph
([elastic#8646](elastic/eui#8646))

**Accessibility**

- Updated `EuiTableHeaderCell` to output `nameTooltip` directly on
sortable cell elements, ensuring tooltips appear on focus
([elastic#8644](elastic/eui#8644))
- Improved the accessibility of `EuiColorPicker` by:
([elastic#8639](elastic/eui#8639))
  - preventing duplicate color output for screen readers
- adding tooltips with visual color labels for the selected colors on
the saturation and hue sliders
  - updated accessible labels and announcements to be more descriptive

**Dependency updates**

- Updated `typescript` to v5.8.3
([elastic#8626](elastic/eui#8626))

(cherry picked from commit bca8299)

# Conflicts:
#	x-pack/solutions/observability/plugins/infra/public/components/asset_details/tabs/processes/__snapshots__/processes_table.test.tsx.snap
akowalska622 pushed a commit to akowalska622/kibana that referenced this pull request May 29, 2025
`102.0.0` ⏩ `102.1.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)

>[!NOTE]
There is also the sibling PR for `8.19` with old Amsterdam theme ready
for review [here](elastic#220049). It
contains the same changes.

## Changes

- Updated test selector (due to changed tooltip placement in
[elastic#8644](elastic/eui#8644))
- snapshot updates

## Package updates

### `@elastic/eui`

#### [`v102.1.0`](https://github.com/elastic/eui/releases/v102.1.0)

- Update `EuiDataGrid` to use `expand` glyph
([elastic#8646](elastic/eui#8646))

**Accessibility**

- Updated `EuiTableHeaderCell` to output `nameTooltip` directly on
sortable cell elements, ensuring tooltips appear on focus
([elastic#8644](elastic/eui#8644))
- Improved the accessibility of `EuiColorPicker` by:
([elastic#8639](elastic/eui#8639))
  - preventing duplicate color output for screen readers
- adding tooltips with visual color labels for the selected colors on
the saturation and hue sliders
  - updated accessible labels and announcements to be more descriptive

**Dependency updates**

- Updated `typescript` to v5.8.3
([elastic#8626](elastic/eui#8626))
qn895 pushed a commit to qn895/kibana that referenced this pull request Jun 3, 2025
`102.0.0` ⏩ `102.1.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)

>[!NOTE]
There is also the sibling PR for `8.19` with old Amsterdam theme ready
for review [here](elastic#220049). It
contains the same changes.

## Changes

- Updated test selector (due to changed tooltip placement in
[elastic#8644](elastic/eui#8644))
- snapshot updates

## Package updates

### `@elastic/eui`

#### [`v102.1.0`](https://github.com/elastic/eui/releases/v102.1.0)

- Update `EuiDataGrid` to use `expand` glyph
([elastic#8646](elastic/eui#8646))

**Accessibility**

- Updated `EuiTableHeaderCell` to output `nameTooltip` directly on
sortable cell elements, ensuring tooltips appear on focus
([elastic#8644](elastic/eui#8644))
- Improved the accessibility of `EuiColorPicker` by:
([elastic#8639](elastic/eui#8639))
  - preventing duplicate color output for screen readers
- adding tooltips with visual color labels for the selected colors on
the saturation and hue sliders
  - updated accessible labels and announcements to be more descriptive

**Dependency updates**

- Updated `typescript` to v5.8.3
([elastic#8626](elastic/eui#8626))
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.

[EuiColorPicker][A11y] Accessibility improvements

3 participants