Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fixed double quotes copy and paste bug #2202

Merged
merged 11 commits into from
Sep 7, 2022
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ export default class TableClipboardHelper {
? TableClipboardHelper.localCopyWithoutHeaders
: TableClipboardHelper.lastLocalCopy;
const values =
localDf === text ? localCopy : SheetClip.prototype.parse(text);
localDf === text ? localCopy : TableClipboardHelper.parse(text);

return applyClipboardToData(
values,
Expand All @@ -111,4 +111,13 @@ export default class TableClipboardHelper {
overflowRows
);
}

private static parse(str: string) {
const temprows = str.split('\n');
if (temprows.length > 1 && temprows[temprows.length - 1] === '') {
temprows.pop();
}
const rows = temprows.map(row => row.split('\t'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for tracking down the original SheetClip.parse. I'm a little worried that this simple replacement - while fixing the problem you encountered with multiple quotes within a cell - will cause other problems due to extra cases that were covered by SheetClip and missed here. I'm thinking particularly about cells that have tab or newline characters in them, which presumably are the whole reason for all that monkey business with quotes, so if a cell starts and ends with quote characters it can have tabs and newlines inside it and these don't cause the parser to move to the next column or row.

Then of course we could ask what happens if there are ALSO quote characters inside the cell text. I'd have guessed these would be escaped (with a backslash before the quote) but I see no code in the original SheetClip.parse to handle that so perhaps the quote character is doubled? That would explain their "replace two quotes with one" step, it's just applied too broadly.

Anyway I think the goal of SheetClip as expressed in its head comment is our goal as well:

This tiny library transforms JavaScript arrays to strings that are pasteable by LibreOffice, OpenOffice, Google Docs and Microsoft Excel.

Meaning we want to be able to copy and paste rectangular sections of dash table to and from common spreadsheet programs, as well as to other dash tables, regardless of what text is in those cells. So I think this requires a little more investigation about how such cells are encoded by spreadsheets - and it may mean we have more work to do on the copy side as well as on paste.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes a lot of sense -- I was wondering if this solution was too simplistic.

One thought I have (which may also be too simplistic) is that when parsing pasted cells we could first "escape" quotes by adding a backslash before each quote and then keep the SheetClip.parse function the same except for changing .replace(/""/g, '"') steps to something like .replace('\"', '"'). Does that general idea sound okay to you or am I oversimplifying?

Regardless, I'm happy to keep looking at this. Thank you again for your help!

return rows;
}
}
32 changes: 32 additions & 0 deletions components/dash-table/tests/selenium/test_basic_copy_paste.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,17 @@ def get_app():
cell_selectable=False,
sort_action="native",
),
DataTable(
id="table4",
data=[{"string": 'a""b', "int": 10}, {"string": "hello", "int": 11}],
columns=[
{"name": "string", "id": "string"},
{"name": "int", "id": "int"},
],
editable=True,
sort_action="native",
include_headers_on_copy_paste=True,
),
]
)

Expand Down Expand Up @@ -334,3 +345,24 @@ def test_tbcp010_copy_from_unselectable_cells_table(test):
assert target.cell(1, 1).get_text() == source.cell(2, 2).get_text()

assert test.get_log_errors() == []


def test_tbcp011_copy_double_quotes(test):
test.start_server(get_app())

source = test.table("table4")
target = test.table("table2")

source.cell(0, 0).click()
with test.hold(Keys.SHIFT):
source.cell(0, 1).click()

test.copy()
target.cell(0, 0).click()
test.paste()

for row in range(1):
for col in range(2):
assert target.cell(row, col).get_text() == source.cell(row, col).get_text()

assert test.get_log_errors() == []