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

[EuiDataGrid] Reduce hidden SR text when copying text from multiple cells #6817

Merged
merged 2 commits into from
May 26, 2023

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented May 26, 2023

Summary

This PR attempts to provide an interim workaround to #6804 while #6806 is blocked due to a Chromium bug.

It adds the hidden attribute to SR text of cells that aren't currently being focused, which prevents the text from being selected/copied in all browsers. The currently focused cell should not be hidden and should still read out its cell position to screen readers.

It's not 100% perfect as the cell position of the currently focused cell will still be end up in a copy/paste selection, but that's infinitely easier to edit once vs having to modify every single cell.

QA

  • Go to http://localhost:8030/#/tabular-content/data-grid-advanced#ref-methods with a screen reader open
  • Confirm that using the up/down arrow navigation keys to navigate between cells still reads out the column name and cell column/row index
  • Confirm that clicking the "focus cell" button to a specific cell still reads out the SR info
  • Attempt to copy and paste multiple cells (see below screenshot). Confirm that only the currently focused cell copies with SR text and all other cells do not

screencap

General checklist

  • Checked in Chrome, Safari, Edge, and Firefox
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@cee-chen cee-chen requested a review from 1Copenut May 26, 2023 19:59
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6817/

Copy link
Contributor

@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.

👍 This drives really well. I tested it with VO (Safari and FF) and NVDA (Chrome and FF). I had the screen reader running and used the mouse or track pad to highlight between 1 and 5 rows and paste them into Notes or Notepad respectively.

Notes does some odd things with Chrome/Edge in formatting, but none of the other browsers do this. 🤞 the Chromium fix that you're waiting on cleans this up too. Small nit but worth noting.

@cee-chen
Copy link
Contributor Author

Thanks for the zippy review Trevor!

Notes does some odd things with Chrome/Edge in formatting, but none of the other browsers do this

It was already doing this in prod for formatting, so yeah just a Chromium thing I think. The only thing that matters for this PR is that the SR text isn't in there 🤞

@cee-chen cee-chen merged commit 48c957b into elastic:main May 26, 2023
@cee-chen cee-chen deleted the datagrid/cell-copy-multiple branch May 26, 2023 21:03
@cee-chen cee-chen self-assigned this May 26, 2023
cee-chen added a commit to cee-chen/eui that referenced this pull request May 30, 2023
cee-chen added a commit to elastic/kibana that referenced this pull request May 31, 2023
This is a backport EUI upgrade to Kibana v8.8.1 containing an
EuiDataGrid bugfix requested by the Discover team:
elastic/eui#6804 (comment)

## [`77.1.4`](https://github.com/elastic/eui/tree/v77.1.4)

- Updated `EuiDataGrid` to only render screen reader text announcing
cell position if the cell is currently focused. This should improve the
ability to copy and paste multiple cells without SR text.
([#6817](elastic/eui#6817))
cee-chen added a commit to elastic/kibana that referenced this pull request Jun 5, 2023
## Summary

`eui@81.0.0` ⏩ `eui@81.2.0`

- Most changes to source code in this PR are CSS cleanups/deprecations
in `EuiSuperDatePicker`/`EuiDatePickerRange`
- One team (ML) had a `inline` specific usage of `EuiDatePickerRange`
that they reached out to us about via Slack, and that we have fixed in
this PR.
- All other usages of date picker components should have remained
working as-is with no changes, but please ping us if you see otherwise!

___

## [`81.2.0`](https://github.com/elastic/eui/tree/v81.2.0)

- Updated `EuiSuperDatePicker` to accept an object configuration for
`isDisabled` ([#6821](elastic/eui#6821))

**Bug fixes**

- Fixed broken `EuiSuperDatePicker` styles
([#6821](elastic/eui#6821))

## [`81.1.0`](https://github.com/elastic/eui/tree/v81.1.0)

- Added `EuiInlineEditText` and `EuiInlineEditTitle` components
([#6757](elastic/eui#6757))
- Updated `EuiDatePickerRange` to support `inline` display
([#6795](elastic/eui#6795))
- Added an `onError` callback prop to `EuiErrorBoundary`
([#6810](elastic/eui#6810))
- Updated `EuiDataGrid` to only render screen reader text announcing
cell position if the cell is currently focused. This should improve the
ability to copy and paste multiple cells without SR text.
([#6817](elastic/eui#6817))

**Bug fixes**

- Fixed `EuiDatePicker`'s `inline` display to correctly render and
prevent user interaction when `disabled` or `readOnly`
([#6795](elastic/eui#6795))
- Fixed `EuiDatePicker`'s `inline` display to correctly render
`isInvalid` and `isLoading` icons
([#6795](elastic/eui#6795))

**CSS-in-JS conversions**

- Converted `EuiDatePickerRange` to Emotion
([#6795](elastic/eui#6795))

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants