From b43c11ce26d8d58653deec1ce7aa1b31f8f55f70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andre=CC=81=20Rivet?= Date: Thu, 20 Aug 2020 08:52:31 -0400 Subject: [PATCH 1/3] failing test for mouse based navigation (tbcp003/True) --- .../components/ControlledTable/index.tsx | 34 +++++++++++++++++-- tests/selenium/test_basic_copy_paste.py | 15 +++++--- 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/src/dash-table/components/ControlledTable/index.tsx b/src/dash-table/components/ControlledTable/index.tsx index b21d51822..ba50e7f98 100644 --- a/src/dash-table/components/ControlledTable/index.tsx +++ b/src/dash-table/components/ControlledTable/index.tsx @@ -429,6 +429,32 @@ export default class ControlledTable extends PureComponent< return document.getElementById(this.props.id) as HTMLElement; } + /*#if TEST_COPY_PASTE*/ + private preventCopyPaste(): boolean { + const {active_cell} = this.props; + let activeElement = document.activeElement; + while (activeElement && activeElement.nodeName.toLowerCase() !== 'td') { + activeElement = activeElement.parentElement; + } + + if (!activeElement) { + return true; + } + + const column_id = activeElement.getAttribute('data-dash-column'); + const row = activeElement.getAttribute('data-dash-row'); + + if ( + column_id !== active_cell?.column_id || + +(row ?? 0) !== active_cell?.row + ) { + return true; + } + + return false; + } + /*#endif*/ + handleKeyDown = (e: any) => { const {setProps, is_focused} = this.props; @@ -443,16 +469,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); + } /*#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); + } /*#endif*/ return; } diff --git a/tests/selenium/test_basic_copy_paste.py b/tests/selenium/test_basic_copy_paste.py index c35cb1d07..fb88df661 100644 --- a/tests/selenium/test_basic_copy_paste.py +++ b/tests/selenium/test_basic_copy_paste.py @@ -1,4 +1,5 @@ import dash +import pytest from dash.dependencies import Input, Output, State from dash.exceptions import PreventUpdate @@ -113,13 +114,19 @@ 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): - target.cell(0, 0).click() - target.cell(2, 0).click() + + target.cell(0, 0).click() + if mouse_navigation: + with test.hold(Keys.SHIFT): + target.cell(2, 0).click() + else: + with test.hold(Keys.SHIFT): + test.send_keys(Keys.ARROW_DOWN + Keys.ARROW_DOWN) test.copy() target.cell(3, 0).click() From ded5e339ac1c6f9cce4decf3f50a16cf19368d73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andre=CC=81=20Rivet?= Date: Thu, 20 Aug 2020 09:39:16 -0400 Subject: [PATCH 2/3] try to fix mismatched active element vs. active_cell --- .../components/ControlledTable/index.tsx | 59 +++++++++++++++---- tests/selenium/test_basic_copy_paste.py | 3 +- 2 files changed, 51 insertions(+), 11 deletions(-) diff --git a/src/dash-table/components/ControlledTable/index.tsx b/src/dash-table/components/ControlledTable/index.tsx index ba50e7f98..cd3f8a2dc 100644 --- a/src/dash-table/components/ControlledTable/index.tsx +++ b/src/dash-table/components/ControlledTable/index.tsx @@ -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(); + } + } + } + const {setState, uiCell, virtualization} = this.props; if (!virtualization) { @@ -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 @@ -429,24 +448,44 @@ export default class ControlledTable extends PureComponent< return document.getElementById(this.props.id) as HTMLElement; } - /*#if TEST_COPY_PASTE*/ - private preventCopyPaste(): boolean { - const {active_cell} = this.props; + 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 true; + 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 ( - column_id !== active_cell?.column_id || - +(row ?? 0) !== active_cell?.row + !active || + active.column_id !== active_cell?.column_id || + active.row !== active_cell?.row ) { return true; } diff --git a/tests/selenium/test_basic_copy_paste.py b/tests/selenium/test_basic_copy_paste.py index fb88df661..957caa8bf 100644 --- a/tests/selenium/test_basic_copy_paste.py +++ b/tests/selenium/test_basic_copy_paste.py @@ -120,11 +120,12 @@ def test_tbcp003_copy_multiple_rows(test, mouse_navigation): target = test.table("table") - target.cell(0, 0).click() if mouse_navigation: with test.hold(Keys.SHIFT): + target.cell(0, 0).click() target.cell(2, 0).click() else: + target.cell(0, 0).click() with test.hold(Keys.SHIFT): test.send_keys(Keys.ARROW_DOWN + Keys.ARROW_DOWN) From 055e1deaafd09298e18c6cdcdcb3c03af192cbc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andre=CC=81=20Rivet?= Date: Thu, 20 Aug 2020 13:51:59 -0400 Subject: [PATCH 3/3] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 98f286445..70917f091 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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