Skip to content
This repository was archived by the owner on Aug 29, 2025. It is now read-only.

3.0 issue75 global copy paste #87

Merged
Marc-Andre-Rivet merged 7 commits into from
Sep 13, 2018
Merged

3.0 issue75 global copy paste #87

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6bb2fff
global copy/paste
Sep 12, 2018
c3f3cae
fix tests
Sep 12, 2018
e0d5003
fix tests
Sep 12, 2018
9f0197a
Merge branch 'master' into 3.0-issue75-global-copy-paste
Marc-Andre-Rivet Sep 12, 2018
b9b0ebb
update bundle
Sep 12, 2018
8f1b699
fix regression (partial)
Sep 13, 2018
a0e446d
fix regression, fix additional inserted line breaks
Sep 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions @Types/modules.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ declare module 'sheetclip' {
const value: {
prototype: {
parse: (text: string) => string[][];
stringify: (arr: any[][]) => string;
}
};

Expand Down
7 changes: 6 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,12 @@

First click selects the cell's content and will cause user input to override the cell content.
Second click into the cell will remove the selection and position the cursor accordingly.

## RC14 - Empty dropdown setting value regression fix

Issue: https://github.com/plotly/dash-table/issues/83

## RC15 - Global copy/paste (through browser menu), incorrect pasted data fix

Issue: https://github.com/plotly/dash-table/issues/75
Issue: https://github.com/plotly/dash-table/issues/88
6 changes: 3 additions & 3 deletions dash_table/bundle.js
View file
Open in desktop

Large diffs are not rendered by default.

6 changes: 3 additions & 3 deletions dash_table/demo.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dash_table/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "dash-table",
"version": "3.0.0rc14",
"version": "3.0.0rc15",
"description": "Dash table",
"main": "build/index.js",
"scripts": {
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "dash-table",
"version": "3.0.0rc14",
"version": "3.0.0rc15",
"description": "Dash table",
"main": "build/index.js",
"scripts": {
Expand Down
38 changes: 4 additions & 34 deletions src/core/Clipboard.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,43 +3,13 @@ export default class Clipboard {
private static value: string;
/*#endif*/

public static set(value: string): void {
public static set(_ev: any, value: string): void {
/*#if TEST_COPY_PASTE*/
Clipboard.value = value;
/*#else*/
_ev.clipboardData.setData('text/plain', value);
_ev.preventDefault();

@Marc-Andre-Rivet Marc-Andre-Rivet Sep 12, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Set our value and prevent the normal execution from overriding our value

@Marc-Andre-Rivet Marc-Andre-Rivet Sep 12, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because of the test vs normal code, sometimes the event variable is not used.. prepending with '_' disables the usage check.. a bit lame but somewhat cleaner than an if..endif for the variable IMO

/*#endif*/

const el = document.createElement('textarea');
el.value = value;

// (Adapted from https://hackernoon.com/copying-text-to-clipboard-with-javascript-df4d4988697f)
// Make it readonly to be tamper-proof
el.setAttribute('readonly', '');
// el.style.position = 'absolute';
// Move outside the screen to make it invisible
// el.style.left = '-9999px';
// Append the <textarea> element to the HTML document
document.body.appendChild(el);

// Check if there is any content selected previously
let selected;
if (document.getSelection().rangeCount > 0) {
// Store selection if found
selected = document.getSelection().getRangeAt(0);
}

// Select the <textarea> content
el.select();
// Copy - only works as a result of a user action (e.g. click events)
document.execCommand('copy');
// Remove the <textarea> element
document.body.removeChild(el);
// If a selection existed before copying
if (selected) {
// Unselect everything on the HTML document
document.getSelection().removeAllRanges();
// Restore the original selection
document.getSelection().addRange(selected);
}

@wbrgss wbrgss Sep 12, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(for my own understanding)
Can you explain briefly how this implementation using document.execCommand fit in with the method using the native onCopy event handler together with TableClipboardHelper.toClipboard()?

@Marc-Andre-Rivet Marc-Andre-Rivet Sep 13, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The document.execCommand was not used with native onCopy, it was always being called by a onkeyDown listener deciding that a combination of keys meant we wanted to copy or paste. This method is typically used to put something in the clipboard when there is not a user interaction/trigger for it. It's also why the behavior was incorrect when other valid ways of initiating copy/paste were used.

So instead, the code now listens for copy/paste events and it gets funneled to whoever has focused (the table's focus is a bit fuzzy at times, but the last user interaction that sets focus would focus on the latest table, hopefully.. I have my doubts but won't really know what happens until we test scenarios with multiple tables visible at the same time -- anyway setting the listener at the top element, the event will eventually propagate there)

Now that we listen to native copy/paste events (all sources), we need to make sure the copy contains the data we want, hence the listener on the copy event calling the helper, setting the value in the event and preventing further default processing that would override what we've just set in the event.

Hopefully this all makes sense.

I'll look at the regression tmr!

@wbrgss wbrgss Sep 13, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OK, I think I understand. So listening to all copypaste event sources now eliminates the need for this block with execCommand. I think the code is cleaner without this (the HackerNoon tutorial method), so once that regression is fixed I'm all for it.

@Marc-Andre-Rivet Marc-Andre-Rivet Sep 13, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Exactly. Listening to all, prevent defaults where needed (forgot one) and overriding with our values.

}

public static get(_ev: ClipboardEvent) {
Expand Down
12 changes: 3 additions & 9 deletions src/dash-table/components/Cell/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,19 +99,13 @@ export default class Cell extends Component<ICellProps, ICellState> {
/>);
}

private onPaste = (e: React.ClipboardEvent<Element>) => {
const { onPaste } = this.propsWithDefaults;

onPaste(e);
e.stopPropagation();
}

private renderInput() {
const {
active,
focused,
onClick,
onDoubleClick
onDoubleClick,
onPaste
} = this.propsWithDefaults;

const classes = [
Expand All @@ -135,7 +129,7 @@ export default class Cell extends Component<ICellProps, ICellState> {
onBlur={this.propagateChange}
onChange={this.handleChange}
onKeyDown={this.handleKeyDown}
onPaste={this.onPaste}
onPaste={onPaste}
{...attributes}
/>);
}
Expand Down
17 changes: 4 additions & 13 deletions src/dash-table/components/CellFactory.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as R from 'ramda';
import React from 'react';
import React, { ClipboardEvent } from 'react';

import Cell from 'dash-table/components/Cell';
import { ICellFactoryOptions, SelectedCells } from 'dash-table/components/Table/props';
Expand Down Expand Up @@ -152,17 +152,8 @@ export default class CellFactory {
});
}

private handlePaste = (idx: number, i: number, e: any) => {
const {
is_focused,
selected_cell
} = this.props;

const selected = this.isCellSelected(selected_cell, idx, i);

if (!(selected && is_focused)) {
e.preventDefault();
}
private handlePaste = (e: ClipboardEvent) => {
e.preventDefault();

@Marc-Andre-Rivet Marc-Andre-Rivet Sep 13, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was causing the regression -- input should not react to the paste event and let the table handle it

@wbrgss wbrgss Sep 13, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thinking out loud — can we have a test, or debugging function, that listens for unexpected event triggers and/or default events? If it's the latter it could be removed in production.

@Marc-Andre-Rivet Marc-Andre-Rivet Sep 13, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We could get all object members that start with 'on' (or just a static list) for a given element and do
el.addEventListener(objMember.substring(2), () => { /* do something */ }) for each of those -- we could test if 'defaultPrevented' is true, not much can be done for stopPropagation.. once that in place I'm not sure how to build a useful test from there but it should be possible.

}

private rowSelectCell(idx: number) {
Expand Down Expand Up @@ -273,7 +264,7 @@ export default class CellFactory {
focused={!!is_focused}
onClick={this.getEventHandler(this.handleClick, virtualIdx, index)}
onDoubleClick={this.getEventHandler(this.handleDoubleClick, virtualIdx, index)}
onPaste={this.getEventHandler(this.handlePaste, virtualIdx, index)}
onPaste={this.handlePaste}
onChange={this.getEventHandler(this.handleChange, realIdx, index)}
property={column.id}
selected={R.contains([virtualIdx, index + offset], selected_cell)}
Expand Down
40 changes: 20 additions & 20 deletions src/dash-table/components/ControlledTable/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ import { colIsEditable } from 'dash-table/components/derivedState';
import {
KEY_CODES,
isCtrlMetaKey,
/*#if TEST_COPY_PASTE*/
isCtrlDown,
/*#endif*/
isMetaKey,
isNavKey
} from 'dash-table/utils/unicode';
Expand Down Expand Up @@ -128,28 +130,27 @@ export default class ControlledTable extends Component<ControlledTableProps> {
editable
} = this.props;

Logger.warning(`handleKeyDown: ${e.key}`);

const ctrlDown = isCtrlDown(e);
Logger.trace(`handleKeyDown: ${e.key}`);

// if this is the initial CtrlMeta keydown with no modifiers then pass
if (isCtrlMetaKey(e.keyCode)) {
return;
}

// if paste event onPaste handler registered in Table jsx handles it
/*#if TEST_COPY_PASTE*/
const ctrlDown = isCtrlDown(e);

if (ctrlDown && e.keyCode === KEY_CODES.V) {
/*#if TEST_COPY_PASTE*/
this.onPaste({} as any);
/*#endif*/
e.preventDefault();
return;
}

// copy
if (e.keyCode === KEY_CODES.C && ctrlDown && !is_focused) {
this.onCopy(e);
this.onCopy(e as any);
return;
}
/*#endif*/

@Marc-Andre-Rivet Marc-Andre-Rivet Sep 12, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The test now need to override both copy and paste keyboard scenarios


if (e.keyCode === KEY_CODES.ESCAPE) {
setProps({ is_focused: false });
Expand Down Expand Up @@ -462,7 +463,7 @@ export default class ControlledTable extends Component<ControlledTableProps> {
selected_cell
);

TableClipboardHelper.toClipboard(noOffsetSelectedCells, columns, dataframe);
TableClipboardHelper.toClipboard(e, noOffsetSelectedCells, columns, dataframe);
this.$el.focus();
}

Expand All @@ -473,15 +474,14 @@ export default class ControlledTable extends Component<ControlledTableProps> {
dataframe,
editable,
filtering_settings,
is_focused,
row_deletable,
row_selectable,
setProps,
sorting_settings,
virtual_dataframe_indices
} = this.props;

if (is_focused || !editable) {
if (!editable) {

@Marc-Andre-Rivet Marc-Andre-Rivet Sep 13, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The paste event is no longer global, if this method gets called it comes from a child input and we know we have focus.

return;
}

Expand All @@ -491,6 +491,7 @@ export default class ControlledTable extends Component<ControlledTableProps> {

const noOffsetActiveCell: [number, number] = [active_cell[0], active_cell[1] - columnIndexOffset];


const result = TableClipboardHelper.fromClipboard(
e,
noOffsetActiveCell,
Expand Down Expand Up @@ -595,10 +596,7 @@ export default class ControlledTable extends Component<ControlledTableProps> {

renderFragment = (cells: any[][] | null) => (
cells ?
(<table
onPaste={this.onPaste}
tabIndex={-1}
>
(<table tabIndex={-1}>

@Marc-Andre-Rivet Marc-Andre-Rivet Sep 12, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Pushing onPaste handling to the top component element

<tbody>
{cells.map(
(row, idx) => <tr key={`row-${idx}`}>{row}</tr>)
Expand Down Expand Up @@ -673,12 +671,14 @@ export default class ControlledTable extends Component<ControlledTableProps> {

const grid = this.getFragments(n_fixed_columns, n_fixed_rows);

return (<div id={id}>
return (<div
id={id}
onCopy={this.onCopy}
onKeyDown={this.handleKeyDown}
onPaste={this.onPaste}
>
<div className='dash-spreadsheet-container'>
<div
className={classes.join(' ')}
onKeyDown={this.handleKeyDown}
>
<div className={classes.join(' ')}>
{grid.map((row, rowIndex) => (<div
key={`r${rowIndex}`}
ref={`r${rowIndex}`}
Expand Down
15 changes: 9 additions & 6 deletions src/dash-table/utils/TableClipboardHelper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,23 @@ import { colIsEditable } from 'dash-table/components/derivedState';
import { ActiveCell, Columns, Dataframe, SelectedCells } from 'dash-table/components/Table/props';

export default class TableClipboardHelper {
public static toClipboard(selectedCells: SelectedCells, columns: Columns, dataframe: Dataframe) {
public static toClipboard(e: any, selectedCells: SelectedCells, columns: Columns, dataframe: Dataframe) {
const selectedRows = R.uniq(R.pluck(0, selectedCells).sort());
const selectedCols: any = R.uniq(R.pluck(1, selectedCells).sort());

const value = R.slice(
const df = R.slice(
R.head(selectedRows) as any,
R.last(selectedRows) as any + 1,
dataframe
).map(row =>
R.props(selectedCols, R.props(R.pluck('id', columns) as any, row) as any)
).map(row => R.values(row).join('\t')
).join('\r\n');
);

const value = SheetClip.prototype.stringify(df);

@Marc-Andre-Rivet Marc-Andre-Rivet Sep 13, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Using SheetClip's way of stringifying instead of our own gets rid of the phantom \r and guarantees we are in=line with what it expects when parsing


Logger.trace('TableClipboard -- set clipboard data: ', value);

Clipboard.set(value);
Clipboard.set(e, value);
}

public static fromClipboard(
Expand All @@ -34,7 +37,7 @@ export default class TableClipboardHelper {
overflowRows: boolean = true
): { dataframe: Dataframe, columns: Columns } | void {
const text = Clipboard.get(ev);
Logger.warning('clipboard data: ', text);
Logger.trace('TableClipboard -- get clipboard data: ', text);

if (!text) {
return;
Expand Down
55 changes: 45 additions & 10 deletions tests/e2e/cypress/integration/copy_paste_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,22 +7,57 @@ describe('copy paste', () => {
cy.visit('http://localhost:8082');
});

it('can do BE roundtrip on cell modification', () => {
it('can copy multiple rows', () => {

@Marc-Andre-Rivet Marc-Andre-Rivet Sep 13, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Additional test to see if we can copy multiple rows

DashTable.getCell(0, 0).click();
DOM.focused.type(`10${Key.Enter}`);

DashTable
.getCell(0, 0)
.within(() => cy.get('.cell-value').should('have.html', '10'))
.then(() => {
DashTable.getCell(0, 1)
.within(() => cy.get('.cell-value').should('have.html', 'MODIFIED'));
});
DOM.focused.type(Key.Shift, { release: false });
DashTable.getCell(2, 0).click();

DOM.focused.type(`${Key.Meta}c`);
DashTable.getCell(3, 0).click();
DOM.focused.type(`${Key.Meta}v`);
DashTable.getCell(0, 0).click();

for (let row = 0; row <= 2; ++row) {
DashTable.getCell(row + 3, 0).within(() => cy.get('.cell-value').should('have.html', `${row}`));
}
});

it('can copy multiple rows and columns', () => {

@Marc-Andre-Rivet Marc-Andre-Rivet Sep 13, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Additional test to see if we can copy multiple rows and columns

@Marc-Andre-Rivet Marc-Andre-Rivet Sep 13, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These tests should protect us better from the regression @wbrgss pointed out

DashTable.getCell(0, 1).click();
DOM.focused.type(Key.Shift, { release: false });
DashTable.getCell(2, 2).click();

DOM.focused.type(`${Key.Meta}c`);
DashTable.getCell(3, 1).click();
DOM.focused.type(`${Key.Meta}v`);
DashTable.getCell(0, 0).click();

for (let row = 0; row <= 2; ++row) {
for (let column = 1; column <= 2; ++column) {
let initialValue: string;

DashTable.getCell(row, column).within(() => cy.get('.cell-value').then($cells => initialValue = $cells[0].innerHTML));
DashTable.getCell(row + 3, column).within(() => cy.get('.cell-value').should('have.html', initialValue));
}
}
});

// Commenting this test as Cypress team is having issues with the copy/paste scenario
// LINK: https://github.com/cypress-io/cypress/issues/2386
describe('BE roundtrip on copy-paste', () => {
it('on cell modification', () => {
DashTable.getCell(0, 0).click();
DOM.focused.type(`10${Key.Enter}`);

DashTable
.getCell(0, 0)
.within(() => cy.get('.cell-value').should('have.html', '10'))
.then(() => {
DashTable.getCell(0, 1)
.within(() => cy.get('.cell-value').should('have.html', 'MODIFIED'));
});
});

it('with unsorted, unfiltered data', () => {
DashTable.getCell(0, 0).click();
DOM.focused.type(`${Key.Meta}c`);
Expand Down