Clean up copy/pasting of tabular content in EuiDataGrid and EuiBasic/InMemoryTable #8019

cee-chen · 2024-09-12T17:04:47Z

Summary

This PR adds a tabular content copy service which listens for copy events, detects hidden characters that both EuiDataGrid and EuiBasicTable now embed, and strips those characters and any undesired newlines/tabs and replaces them with only newlines/tabs that correspond with rows and cells.

The inspiration/initial idea for this workaround came from this StackOverflow response.
- I opted against using https://developer.mozilla.org/en-US/docs/Web/API/Clipboard/readText or writeText() because it only works on https and with permissions. I went with the more old school clipboardData.setData() instead: https://developer.mozilla.org/en-US/docs/Web/API/Element/copy_event

Warning

This will not work as expected for virtualized data grids that scroll (especially if the last cell is not visible and scrolled off the page). There is not a lot EUI can do about this as the cells are literally not rendered in the DOM - at most we could provide an onCopy API for consumers to hook into and provide their own cleaned data, but that would have to be a future feature request/enhancement.

QA

Go to https://eui.elastic.co/pr_8019/storybook/index.html?path=/story/tabular-content-euidatagrid--playground
Ctrl+A to select all or select a few rows
Open Google Spreadsheets and paste into it and confirm the grid row(s) paste in cleanly and as expected
Go to https://eui.elastic.co/pr_8019#/tabular-content/tables#a-basic-table
Copy 2 tables from anywhere on the page
Paste into Google Spreadsheets or a notepad type editor and confirm that the tables are stripped in terms of newlines but the other content is not

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 - N/A
Code quality checklist
- Added or updated jest and cypress tests
  - [ ] Updated visual regression tests
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 - N/A

and move existing `copy_to_clipboard` there

- requires waterfalling `visibleColCount` number down to header cell wrapper for isLastColumn logic

- and as such not appending the correct copy markers

- requires adding an `append` prop to various cell components, for rendering this hidden text outside of the cell content wrapper

- my god this was a StackOverflow journey

- we should be more specific with selectors anyway

works just fine in actual browsers, but typescript really wants document for whatever reason see https://stackoverflow.com/questions/74809554/cant-get-paste-event-to-work-in-typescript :|

cee-chen · 2024-09-17T06:04:51Z

packages/eui/src/services/copy/tabular_copy.tsx

+// Special visually hidden unicode characters that we use to manually clean content
+// and force our own newlines/horizontal tabs
+export const CHARS = {
+  NEWLINE: '↵',
+  TAB: '↦',
+  TABULAR_CONTENT_BOUND: '⁣', // U+2063 - invisible separator
+  NO_COPY_BOUND: '⁢', // U+2062 - invisible times
+};


I'd really appreciate people's thoughts on the (semi-random) characters I've chosen here and if we think they're unlikely enough to be used in consumer content to feel safe using them here!

I'm wondering if the invisible times might be used in a grid somehow as its usage seems to be in maths context.

The invisible times codepoint is used in mathematical type-setting to indicate the multiplication of two terms without a visible multiplication operator, e.g. when type-setting 2x (the multiplication of the number 2 and the variable x), the invisible times codepoint can be inserted in-between: 2 <U+2062> x.

And the invisible seperator ("invisible comma") is apparently commonly used. Question would be if that's a case for a datagrid though.

Do we need to use invisible characters here?
If not, I found this "Angle with Down Zig-zag Arrow" character ⍼ of the "Misc technical" block which seems to have no meaning and hence hopefully is not commonly used? 🤔

I'm good with changing it to whatever! 👍 No strong feelings either way. I was also thinking maybe these scissors characters?

✁ UPPER BLADE SCISSORS
✃ LOWER BLADE SCISSORS

Yeah I'm fine with kind of whatever too, in the end the important thing is it's not commonly used.
We could try the scissors and worst case we update if issues are reported.
... I just had another thought: How about combining 2 random characters? That'll reduce the likelyhood of them being used for sure?

We can use two random characters for NO_COPY_BOUND fairly easily, but I'm a little more hesitant for TABULAR_CONTENT_BOUND, as that complicates the .split() logic I'm using & would possibly require a regex at that point. Do you have any suggestions for characters? 👀

@mgadewoll I misread your comment - I thought you meant using two different characters for the start & end bounds! Using two characters does seem super smart - I've made that tweak here (9a22861) while trying to keep the symbols still fairly relevant to the intent. Let me know if it looks good to you!

packages/eui/src/services/copy/tabular_copy.tsx

mgadewoll · 2024-09-17T08:37:18Z

packages/eui/src/services/copy/tabular_copy.tsx

+// Special visually hidden unicode characters that we use to manually clean content
+// and force our own newlines/horizontal tabs
+export const CHARS = {
+  NEWLINE: '↵',
+  TAB: '↦',
+  TABULAR_CONTENT_BOUND: '⁣', // U+2063 - invisible separator
+  NO_COPY_BOUND: '⁢', // U+2062 - invisible times
+};


I'm wondering if the invisible times might be used in a grid somehow as its usage seems to be in maths context.

The invisible times codepoint is used in mathematical type-setting to indicate the multiplication of two terms without a visible multiplication operator, e.g. when type-setting 2x (the multiplication of the number 2 and the variable x), the invisible times codepoint can be inserted in-between: 2 <U+2062> x.

And the invisible seperator ("invisible comma") is apparently commonly used. Question would be if that's a case for a datagrid though.

Do we need to use invisible characters here?
If not, I found this "Angle with Down Zig-zag Arrow" character ⍼ of the "Misc technical" block which seems to have no meaning and hence hopefully is not commonly used? 🤔

mgadewoll · 2024-09-17T08:45:42Z

packages/eui/src/components/datagrid/body/header/data_grid_header_row.tsx

@@ -80,6 +83,7 @@ const EuiDataGridHeaderRow = memo(
          <EuiDataGridControlHeaderCell
            key={controlColumn.id}
            index={index + leadingControlColumns.length + columns.length}
+            visibleColCount={visibleColCount}


Currently the leading/trailing columns have a visually hidden (SR only) column title.
This one will be copied though it's not visible. Is that expected or should visually hidden content not be copied? 🤔

Screen reader text will generally be copied unless it's been marked with special characters to remove it from copied text (which is what I did for our enter key instructions). In this case I think it's entirely appropriate to copy.

@jughosta did request a configurable feature for control columns in Slack:

J: Would it be possible to exclude control columns and icons from the output too? Maybe we could allow some customization options.
C: In theory with this approach consumers could render their own 'exclude' bounds with the character I've created for that 🤔 but are you saying you'd prefer a prop like columns={[{ id: 'someColumn', excludeWhenCopying: true }]}?

I'm not totally sure I have time for that right now though, so I wanted to ship this PR as-is (since it's a large improvement) and consider adding that in a future follow-up PR. My 2c is that users deleting a single column from a spreadsheet is very few clicks in comparison to completely cleaning newlines from every cell.

Yeah I agree. It's not much effort to delete columns that are unwanted and it's already a great improvement and optionally removing columns could be a follow-up if required.

I would not say that it would be simple for users to remove columns from the output as the output might become misaligned.

For example, I skip the controls and start my selection from text "Green..."

and after copying I get the following:

Agree that the PR already brings a huge improvement although many users would still run into the similar problems if we don't address it.

yes, agreed, this was also the first thing I was running into, so this might be a common problem, would it be possible to exclude control columns by default?

I also want to mention that I'd really prefer to push control column management to a follow-up feature request if possible, unless you all strongly feel like it's a blocker.

I spiked this out on a Friday as a "I wonder if I can get this working" experiment, but I have other work I need to switch back to and I don't know if I want to spend a lot more time on this one PR that already ballooned to a 1k+ diff 😅

I'm open to holding the PR for breaking bugs or strong performance concerns, but in terms of perfection I'd rather go for incremental improvements for now.

Not a blocker. Thanks for improving the selection!

Agreed, not a blocker, thx for taking care of this so quickly 🙏 Btw it's interesting how Security is using it, for me this is a edge case

Agreed this is not a blocker and it's already a massive improvement! 🤘 The issue described here is true for native HTML tables too, so I wouldn't be too concerned. There's a risk trying to be too smart around control columns could result in unexpected behaviour for consumers anyway (or maybe not, hard to guess). The suggestion for consumers to selectively exclude what they don't want copied using the same characters seems safe and reasonable IMO, especially if that's already possible for control columns.

@cee-chen It looks like the utils from tabular_copy.tsx are exported publicly, is that right? If so it should already be easy from our side to exclude the control columns in Discover if we want to.

You'll be able to exclude screen reader only control column text, but the control columns themselves will always generate a hidden horizontal tab character - that is currently outside the scope of what's overrideable right now in this PR.

For the former use case, usage would look something like this:

import { EuiDataGrid, tabularCopyMarkers } from '@elastic/eui'; <EuiDataGrid leadingControlColumns={[{ id: 'selection', width: 32, headerCellRender: () => ( <EuiScreenReaderOnly> <span> {tabularCopyMarkers.hiddenNoCopyBoundary} Example text that does not show on copy {tabularCopyMarkers.hiddenNoCopyBoundary} </span> </EuiScreenReaderOnly> ), }]} />

mgadewoll · 2024-09-17T08:51:43Z

packages/eui/cypress/support/copy/select_and_copy.tsx

+    Cypress.automation('remote:debugger:protocol', {
+      command: 'Browser.grantPermissions',
+      params: {
+        permissions: ['clipboardReadWrite', 'clipboardSanitizedWrite'],


I went down so many google/stackoverflow rabbit holes to figure out how to get this working haha 🫠 honestly until the very end I wasn't really sure it was going to be, but very glad to have a regression test for this!

packages/eui/src/services/copy/tabular_copy.tsx

cee-chen · 2024-09-17T16:55:25Z

@kertal Before we ship this, do you see any issues or concerns with this approach in production Kibana/Discover?

kertal · 2024-09-17T17:14:41Z

@kertal Before we ship this, do you see any issues or concerns with this approach in production Kibana/Discover?

@cee-chen just a quick first take on it, @jughosta, could you check tomorrow, since you implemented and closed #192395 for this approach. many thx 🙏. we could also create a shared a-la-carte with this PR (if current state of EUI/Kibana is allowing to do so)

also FYI @davismcphee because of his focus on Discover rendering performance, and optimizations around the grid in Discover

kertal · 2024-09-18T11:16:40Z

Shared Kibana/EUI PR instance to test btw: https://kertal-pr-192756-eui-v95-11-0.kbndev.co/app/r/s/KzSxC

…nsumer + use 2 characters for increased chances + add more `data` attributes to help identify usages

- in case consumers want to use it outside SR text - thanks Davis for the prompt!

elasticmachine · 2024-09-18T21:53:21Z

💚 Build Succeeded

Buildkite Build
Commit: 1e749f2

History

💚 Build #2751 succeeded 729f192
💔 Build #2749 failed 2055f0e
💔 Build #2733 failed 68e2f99

cee-chen changed the title ~~Clean up copy/pasting or tabular content in EuiDataGrid and EuiBasic/InMemoryTable~~ Clean up copy/pasting of tabular content in EuiDataGrid and EuiBasic/InMemoryTable Sep 12, 2024

jughosta mentioned this pull request Sep 13, 2024

[Discover] Provide cleaner text selection for the grid elastic/kibana#192395

Closed

2 tasks

cee-chen added 4 commits September 16, 2024 17:44

[setup] Create new copy/ dir in services

86d48a5

and move existing `copy_to_clipboard` there

Create copy service

dc87591

[EuiDataGrid] Set up copy markers for row cells

1b617a8

[EuiDataGrid] Set up copy markers for header columns/row

f0b7d6e

- requires waterfalling `visibleColCount` number down to header cell wrapper for isLastColumn logic

cee-chen force-pushed the tabular-copying branch from 68e2f99 to 3d837bf Compare September 17, 2024 04:06

cee-chen added 3 commits September 16, 2024 22:40

[EuiDataGrid] Fix footer rows not appending last/first column classes

8f1fd8b

- and as such not appending the correct copy markers

[EuiBasicTable] Set up copy markers for table cells

8a3f6e1

- requires adding an `append` prop to various cell components, for rendering this hidden text outside of the cell content wrapper

Add E2E Cypress tests for checking final copied content

44f9338

- my god this was a StackOverflow journey

cee-chen force-pushed the tabular-copying branch from 2055f0e to 43cc859 Compare September 17, 2024 05:41

cee-chen added 3 commits September 16, 2024 23:01

Fix failing Cypress test due to new copy marker DOM

7089f50

- we should be more specific with selectors anyway

Fix bizarre typescript complaint

fedb57d

works just fine in actual browsers, but typescript really wants document for whatever reason see https://stackoverflow.com/questions/74809554/cant-get-paste-event-to-work-in-typescript :|

changelog

729f192

cee-chen marked this pull request as ready for review September 17, 2024 06:01

cee-chen requested a review from a team as a code owner September 17, 2024 06:01

cee-chen force-pushed the tabular-copying branch from 43cc859 to 729f192 Compare September 17, 2024 06:03

cee-chen commented Sep 17, 2024

View reviewed changes

mgadewoll reviewed Sep 17, 2024

View reviewed changes

cee-chen added 2 commits September 18, 2024 14:20

[PR feedback] Use non-invisible characters to reduce likelihood of co…

9a22861

…nsumer + use 2 characters for increased chances + add more `data` attributes to help identify usages

Rename exported noCopy marker util

1e749f2

- in case consumers want to use it outside SR text - thanks Davis for the prompt!

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up copy/pasting of tabular content in EuiDataGrid and EuiBasic/InMemoryTable #8019

Clean up copy/pasting of tabular content in EuiDataGrid and EuiBasic/InMemoryTable #8019

cee-chen commented Sep 12, 2024 •

edited

Loading

cee-chen Sep 17, 2024

mgadewoll Sep 17, 2024

cee-chen Sep 17, 2024

mgadewoll Sep 17, 2024

cee-chen Sep 17, 2024

cee-chen Sep 18, 2024

mgadewoll Sep 17, 2024

mgadewoll Sep 17, 2024

cee-chen Sep 17, 2024 •

edited

Loading

mgadewoll Sep 17, 2024

jughosta Sep 18, 2024

kertal Sep 18, 2024

cee-chen Sep 18, 2024 •

edited

Loading

jughosta Sep 18, 2024

kertal Sep 18, 2024

davismcphee Sep 18, 2024

cee-chen Sep 18, 2024 •

edited

Loading

mgadewoll Sep 17, 2024

cee-chen Sep 17, 2024

cee-chen commented Sep 17, 2024

kertal commented Sep 17, 2024 •

edited

Loading

kertal commented Sep 18, 2024

elasticmachine commented Sep 18, 2024

Clean up copy/pasting of tabular content in EuiDataGrid and EuiBasic/InMemoryTable #8019

Are you sure you want to change the base?

Clean up copy/pasting of tabular content in EuiDataGrid and EuiBasic/InMemoryTable #8019

Conversation

cee-chen commented Sep 12, 2024 • edited Loading

Summary

QA

General checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen commented Sep 17, 2024

kertal commented Sep 17, 2024 • edited Loading

kertal commented Sep 18, 2024

elasticmachine commented Sep 18, 2024

💚 Build Succeeded

History

cee-chen commented Sep 12, 2024 •

edited

Loading

cee-chen Sep 17, 2024 •

edited

Loading

cee-chen Sep 18, 2024 •

edited

Loading

cee-chen Sep 18, 2024 •

edited

Loading

kertal commented Sep 17, 2024 •

edited

Loading