[ZEPPELIN-4407] Add copy to clipboard (TSV/CSV) for table results#5261
[ZEPPELIN-4407] Add copy to clipboard (TSV/CSV) for table results#5261kkalyan wants to merge 3 commits into
Conversation
Adds "Copy as TSV" and "Copy as CSV" options to the table result export dropdown in both the Angular (zeppelin-web-angular) and classic AngularJS (zeppelin-web) UIs. - Header row (column names) is always included in the copied text - Cell values containing the delimiter, double-quotes, or newlines are RFC 4180 quoted automatically - Uses navigator.clipboard.writeText with a document.execCommand fallback for older browsers - In the Angular table visualization, "Copy visible data" copies only the currently filtered/paginated rows, matching the existing "Export visible" behaviour - Existing "CSV" / "TSV" download items renamed to "Download as CSV/TSV" for clarity Closes apache#3496 (original AngularJS-only implementation by @amakaur)
…s in tests Karma tests failed because spyOn(navigator.clipboard, 'writeText') was called twice — once in beforeEach and again inside two test bodies. Playwright tests failed because grantPermissions ran before the CI skip, and Firefox/WebKit reject unknown permissions (clipboard-read/clipboard-write).
tbonelee
left a comment
There was a problem hiding this comment.
Thanks for the suggestion.
Overall logics looks good to me, and I only left a few feedbacks.
You could also add a test cases which handles double quote delimiter in both zeppelin-web and zeppelin-web-angular
| navigator.clipboard.writeText(text).catch(() => { | ||
| const el = document.createElement('textarea'); | ||
| el.value = text; | ||
| el.style.position = 'absolute'; | ||
| el.style.left = '-9999px'; | ||
| document.body.appendChild(el); | ||
| el.select(); | ||
| document.execCommand('copy'); | ||
| document.body.removeChild(el); | ||
| }); |
There was a problem hiding this comment.
| navigator.clipboard.writeText(text).catch(() => { | |
| const el = document.createElement('textarea'); | |
| el.value = text; | |
| el.style.position = 'absolute'; | |
| el.style.left = '-9999px'; | |
| document.body.appendChild(el); | |
| el.select(); | |
| document.execCommand('copy'); | |
| document.body.removeChild(el); | |
| }); | |
| // TODO: Refactor the duplicated copy-to-clipboard logics | |
| const fallbackCopy = () => { | |
| const el = document.createElement('textarea'); | |
| el.value = text; | |
| el.style.position = 'absolute'; | |
| el.style.left = '-9999px'; | |
| document.body.appendChild(el); | |
| el.select(); | |
| document.execCommand('copy'); | |
| document.body.removeChild(el); | |
| }; | |
| // navigator.clipboard is undefined in non-secure contexts (e.g. plain HTTP), | |
| // where writeText would throw synchronously before the catch could run. | |
| if (navigator.clipboard) { | |
| navigator.clipboard.writeText(text).catch(fallbackCopy); | |
| } else { | |
| fallbackCopy(); | |
| } |
There was a problem hiding this comment.
Done — extracted fallbackCopy as a named function and added the if (navigator.clipboard) guard before calling writeText, so the fallback triggers correctly in non-HTTPS contexts too. Applied the same pattern to both result.component.ts and table-visualization.component.ts.
| navigator.clipboard.writeText(text).catch(() => { | ||
| const el = document.createElement('textarea'); | ||
| el.value = text; | ||
| el.style.position = 'absolute'; | ||
| el.style.left = '-9999px'; | ||
| document.body.appendChild(el); | ||
| el.select(); | ||
| document.execCommand('copy'); | ||
| document.body.removeChild(el); | ||
| }); |
There was a problem hiding this comment.
| navigator.clipboard.writeText(text).catch(() => { | |
| const el = document.createElement('textarea'); | |
| el.value = text; | |
| el.style.position = 'absolute'; | |
| el.style.left = '-9999px'; | |
| document.body.appendChild(el); | |
| el.select(); | |
| document.execCommand('copy'); | |
| document.body.removeChild(el); | |
| }); | |
| // TODO: Refactor the duplicated copy-to-clipboard logics | |
| const fallbackCopy = () => { | |
| const el = document.createElement('textarea'); | |
| el.value = text; | |
| el.style.position = 'absolute'; | |
| el.style.left = '-9999px'; | |
| document.body.appendChild(el); | |
| el.select(); | |
| document.execCommand('copy'); | |
| document.body.removeChild(el); | |
| }; | |
| // navigator.clipboard is undefined in non-secure contexts (e.g. plain HTTP), | |
| // where writeText would throw synchronously before the catch could run. | |
| if (navigator.clipboard) { | |
| navigator.clipboard.writeText(text).catch(fallbackCopy); | |
| } else { | |
| fallbackCopy(); | |
| } |
There was a problem hiding this comment.
Done — same change applied here in table-visualization.component.ts: extracted fallbackCopy and added the navigator.clipboard existence guard.
| let text = ''; | ||
| for (let titleIndex in tableData.columns) { | ||
| if (tableData.columns.hasOwnProperty(titleIndex)) { | ||
| text += tableData.columns[titleIndex].name + delimiter; |
There was a problem hiding this comment.
We should escape not only the row value but the header value also. You could extract the escape function inside the closure and use that for this.
There was a problem hiding this comment.
Fixed — extracted an escape() closure that is now shared by both the header row and cell values, so header names containing the delimiter, double quotes, or newlines are properly RFC 4180 quoted.
| let dsvRow = ''; | ||
| for (let index in row) { | ||
| if (row.hasOwnProperty(index)) { | ||
| let stringValue = (row[index]).toString(); |
There was a problem hiding this comment.
Null check is needed at here.
| let stringValue = (row[index]).toString(); | |
| let stringValue = (value === null || value === undefined) ? '' : String(value); |
There was a problem hiding this comment.
Fixed — the escape() function now does (value === null || value === undefined) ? '' : String(value) before checking for special characters.
- Extract fallbackCopy function and guard navigator.clipboard existence in both result.component.ts and table-visualization.component.ts so the fallback works in plain HTTP (non-secure) contexts - Refactor result.controller.js: extract escape() closure shared by headers and cells; fix null/undefined cell value handling - Add test: header values containing double quotes are RFC 4180 quoted (classic UI Karma test + Angular E2E Playwright test)
What is this PR for?
I've seen users downloading CSV and opening in spreadsheet viewer and copying the text.
Adding way to copy CSV/TSV directly. This is orginally implemented by @amakaur #3496 but it was closed due to lack of tests. I'm picking it up now.
Changes
Angular UI (
zeppelin-web-angular)result.component— paragraph toolbar dropdown: renamed existing items to "Download as CSV/TSV", added divider, then "Copy as TSV" and "Copy as CSV"table-visualization.component— inner table Export menu: added "Copy all data as TSV/CSV" and "Copy visible data as TSV/CSV" (mirrors the existing "Export visible" scope)Classic AngularJS UI (
zeppelin-web)result-chart-selector.html— same dropdown restructure: Download / divider / Copyresult.controller.js— new$scope.copyToClipboard(delimiter)functionBehaviour
navigator.clipboard.writeTextwith adocument.execCommand('copy')fallback for older browsersWhat type of PR is it?
Feature
What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-4407
How should this be tested?
%sh printf "col1\tcol2\na\t1\nb\t2\n")"hello, world"→ the CSV copy should quote it correctlyTests
zeppelin-web/src/app/notebook/paragraph/result/result.controller.test.js— 4 new specs covering TSV copy, CSV copy, delimiter quoting, and double-quote escapingzeppelin-web-angular/e2e/tests/notebook/paragraph/copy-to-clipboard.spec.ts— 3 new specs (skipped on CI, require a live interpreter)Questions
screenshots