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.

Sign up for GitHub

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

Jump to bottom

[EuiDataGrid] Skip screen reader text when selecting text from grid #6804

Closed
Tracked by #182611
jughosta opened this issue May 23, 2023 · 8 comments
Closed
Tracked by #182611

[EuiDataGrid] Skip screen reader text when selecting text from grid #6804

jughosta opened this issue May 23, 2023 · 8 comments
Assignees
@cee-chen
Labels
bug data grid

Comments

@jughosta
Copy link
Contributor

jughosta commented May 23, 2023 •
edited

As a user when selecting text in the grid, I would expect to get only the selection to my clipboard. Instead, screen reader text is being added too.

Screenshot 2023-05-23 at 19 49 40

  • Expected result: only the originaly selected text.

  • Actual:

May 23, 2023 @ 19:49:14.276
- timestamp, column 3, row 1



200
- response, column 4, row 1
https://cdn.elastic-elastic-elastic.org/styles/app.css
- url, column 5, row 1
125.112.150.127
- clientip, column 6, row 1
win xp
- machine.os, column 7, row 1
[success, security]
- tags, column 8, row 1

- openDetails, column 1, row 2

- select, column 2, row 2
May 23, 2023 @ 19:48:36.092
- timestamp, column 3, row 2
200
- response, column 4, row 2
https://artifacts.elastic.co/downloads/beats/metricbeat/metricbeat-6.3.2-i686.rpm
- url, column 5, row 2
35.169.122.161
- clientip, column 6, row 2
ios
- machine.os, column 7, row 2
[success, info]
- tags, column 8, row 2

- openDetails, column 1, row 3

- select, column 2, row 3
May 23, 2023 @ 19:45:42.944
- timestamp, column 3, row 3
200

Initially discussed in #6561 (comment)

Feedback from the community:
https://discuss.elastic.co/t/copy-paste-from-kibana-without-row-and-column-information/320048/
https://discuss.elastic.co/t/copy-paste-from-kibana-without-row-and-column-information-2/334026
@cee-chen
Copy link
Member

cee-chen commented May 23, 2023 •
edited

This will need thorough cross-browser testing, but I believe we should be able to simply use user-select: none CSS on the below EuiScreenReaderOnly text to prevent it from showing up in copy/paste:

eui/src/components/datagrid/body/data_grid_cell.tsx

Lines 95 to 108 in 329eb41

<EuiScreenReaderOnly>
<p>
{'- '}
<EuiI18n
token="euiDataGridCell.position"
default="{columnId}, column {col}, row {row}"
values={{
columnId: column?.displayAsText || rest.columnId,
col: colIndex + 1,
row: ariaRowIndex,
}}
/>
</p>
</EuiScreenReaderOnly>

In fact, this feels so generally useful (as opposed to a one-off fix), that I think we should make this a new prop, e.g. <EuiScreenReaderOnly preventCopy={true} />

@cee-chen cee-chen added the bug label May 23, 2023
@cee-chen cee-chen mentioned this issue May 23, 2023
Closed
9 tasks
@cee-chen
Copy link
Member

user-select: none works perfectly except in Chromium, which has a bizarre bug around rich text pasting still having the hidden text, but plain text pasting not having it (https://bugs.chromium.org/p/chromium/issues/detail?id=1083125). We'll likely wait to deploy a fix until Chromium has a resolution - it looks like they're really close to one.

@cee-chen cee-chen self-assigned this May 23, 2023
@cee-chen cee-chen mentioned this issue May 26, 2023
Merged
6 tasks
@cee-chen
Copy link
Member

@jughosta Until the linked Chromium bug is fixed, I'm going to add a workaround for now that makes it so only the current open/focused cell has copy-able SR context. So there will still be extra text for 1 cell, but that's much easier to edit out / significantly better than every single cell 😅 I won't close this bug out until #6806 lands however.

BTW, in your opinion, is this bug severe enough to warrant a backport to the next Kibana 8.8 release? Or is the next 8.9 fine?

@jughosta
Copy link
Contributor Author

Great! Thanks, @cee-chen ! Not only Discover will benefit from it but also other Kibana pages which don't have CSV export.

I would say yes. Let's confirm with @davismcphee.

@davismcphee
Copy link
Contributor

@cee-chen @jughosta IMO it depends -- if @cee-chen considers the fix low risk and easy to backport to 8.8, I'd say go for it. If not, the issue has existed on our end for a while at this point, so it wouldn't be a huge deal to ship it in 8.9. Also, thanks for the quick fix!

@cee-chen
Copy link
Member

cee-chen commented May 30, 2023 •
edited

It's a fairly easy-to-backport fix for 8.8. I can do a quick release and PR for Kibana sometime in the next week here. Thanks y'all!

@cee-chen cee-chen mentioned this issue May 31, 2023
Merged
cee-chen added a commit to elastic/kibana that referenced this issue May 31, 2023
@cee-chen
[8.8] Upgrade EUI to v77.1.4 (#158770)
564ff93 
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
Copy link
Member

The workaround is merged into Kibana and should be in the next v8.8.1. Whenever Chromium is finally updated to enable a full fix, I'll backport that as well and close out this issue then.

@dej611 dej611 mentioned this issue Jun 28, 2023
Open
@kertal kertal mentioned this issue Mar 18, 2024
Open
@timductive timductive mentioned this issue May 3, 2024
Open
@cee-chen
Copy link
Member

I'm going to mark this issue as closed for now because it looks like Chromium has no intention of fixing the bug anytime soon, and the workaround suffices in the meanwhile.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cee-chen cee-chen

Labels
bug data grid
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[EuiScreenReaderOnly] Add preventCopy prop cee-chen/eui
3 participants
@cee-chen @jughosta @davismcphee