Skip to content
This repository was archived by the owner on Jun 4, 2024. It is now read-only.

Issue 790 - Copy/paste regression when selecting cells with mouse #818

Merged
merged 3 commits into from
Aug 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
## [Unreleased]
### Fixed
- [#817](https://github.com/plotly/dash-table/pull/817) Fix a regression introduced with [#722](https://github.com/plotly/dash-table/pull/722) causing the tooltips to be misaligned with respect to their parent cell
- [#818](https://github.com/plotly/dash-table/pull/818) Fix a regression causing copy/paste not to work when selecting a range of cells with Shift + mouse click

## [4.9.0] - 2020-07-27
### Added
Expand Down
81 changes: 75 additions & 6 deletions src/dash-table/components/ControlledTable/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,28 @@ export default class ControlledTable extends PureComponent<
this.handleDropdown();
this.adjustTooltipPosition();

const {active_cell} = this.props;

// Check if the focus is inside this table
if (this.containsActiveElement()) {
const active = this.getActiveCellAttributes();

// If there is an active cell and it does not have focus -> transfer focus
if (
active &&
active_cell &&
(active.column_id !== active_cell?.column_id ||
active.row !== active_cell?.row)
) {
const target = this.$el.querySelector(
`td[data-dash-row="${active_cell.row}"][data-dash-column="${active_cell.column_id}"]:not(.phantom-cell)`
) as HTMLElement;
if (target) {
target.focus();
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sure that the active cell is focused if the active element is another data cell in the table (triggered by mouse click to change the selected_cells)

}

const {setState, uiCell, virtualization} = this.props;

if (!virtualization) {
Expand Down Expand Up @@ -194,11 +216,8 @@ export default class ControlledTable extends PureComponent<
}

handleClick = (event: any) => {
const $el = this.$el;

if (
$el &&
!$el.contains(event.target as Node) &&
this.containsActiveElement() &&
/*
* setProps is expensive, it causes excessive re-rendering in Dash.
* so, only call when the table isn't already focussed, otherwise
Expand Down Expand Up @@ -429,6 +448,52 @@ export default class ControlledTable extends PureComponent<
return document.getElementById(this.props.id) as HTMLElement;
}

private containsActiveElement(): boolean {
const $el = this.$el;

return $el && $el.contains(document.activeElement);
}

private getActiveCellAttributes(): {
column_id: string | null;
row: number | null;
} | void {
let activeElement = document.activeElement;
while (activeElement && activeElement.nodeName.toLowerCase() !== 'td') {
activeElement = activeElement.parentElement;
}

if (!activeElement) {
return;
}

const column_id = activeElement.getAttribute('data-dash-column');
const row = activeElement.getAttribute('data-dash-row');

return {column_id, row: +(row ?? 0)};
}

/*#if TEST_COPY_PASTE*/
private preventCopyPaste(): boolean {
if (!this.containsActiveElement()) {
return false;
}

const {active_cell} = this.props;
const active = this.getActiveCellAttributes();

if (
!active ||
active.column_id !== active_cell?.column_id ||
active.row !== active_cell?.row
) {
return true;
}

return false;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am unable to use Ctrl+C/Ctrl+V in a way that triggers copy/paste in Selenium, Chrome does not support document.execCommand('paste') and the ClipboardAPI requires the user to Allow browser permission for usage. These reasons are behind the special "test" build and also behind this long standing regression. Prior to this change, the tests worked but the real build didn't. This tries to better align the test behavior vs. the real world behavior.

/*#endif*/

handleKeyDown = (e: any) => {
const {setProps, is_focused} = this.props;

Expand All @@ -443,16 +508,20 @@ export default class ControlledTable extends PureComponent<

if (ctrlDown && e.keyCode === KEY_CODES.V) {
/*#if TEST_COPY_PASTE*/
this.onPaste({} as any);
e.preventDefault();
if (!this.preventCopyPaste()) {
this.onPaste({} as any);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trigger the test "paste" only if the cell with focus is the active_cell

}
/*#endif*/
return;
}

if (e.keyCode === KEY_CODES.C && ctrlDown && !is_focused) {
/*#if TEST_COPY_PASTE*/
this.onCopy(e as any);
e.preventDefault();
if (!this.preventCopyPaste()) {
this.onCopy(e as any);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trigger the test "copy" only if the cell with focus is the active_cell

}
/*#endif*/
return;
}
Expand Down
14 changes: 11 additions & 3 deletions tests/selenium/test_basic_copy_paste.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import dash
import pytest
from dash.dependencies import Input, Output, State
from dash.exceptions import PreventUpdate

Expand Down Expand Up @@ -113,13 +114,20 @@ def test_tbcp002_sorted_copy_paste_callback(test):
assert target.cell(2, 1).get_text() == "MODIFIED"


def test_tbcp003_copy_multiple_rows(test):
@pytest.mark.parametrize("mouse_navigation", [True, False])
def test_tbcp003_copy_multiple_rows(test, mouse_navigation):
test.start_server(get_app())

target = test.table("table")
with test.hold(Keys.SHIFT):

if mouse_navigation:
with test.hold(Keys.SHIFT):
target.cell(0, 0).click()
target.cell(2, 0).click()
else:
target.cell(0, 0).click()
target.cell(2, 0).click()
with test.hold(Keys.SHIFT):
test.send_keys(Keys.ARROW_DOWN + Keys.ARROW_DOWN)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test navigating to the cell both by selecting a region with the keyboard and the mouse


test.copy()
target.cell(3, 0).click()
Expand Down