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

fix keyboard navigation after copy/paste #90

Merged
Marc-Andre-Rivet merged 2 commits into from
Sep 14, 2018
Merged

fix keyboard navigation after copy/paste #90

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
db34bd7
fix keyboard navigation after copy/paste
Sep 13, 2018
970363c
merge master into 3.0-navigation-copy-paste
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
6 changes: 5 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,4 +116,8 @@
## 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
Issue: https://github.com/plotly/dash-table/issues/88

## RC16 - Fix incorrect keyboard navigation

Issue: https://github.com/plotly/dash-table/issues/49
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.0rc15",
"version": "3.0.0rc16",
"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.0rc15",
"version": "3.0.0rc16",
"description": "Dash table",
"main": "build/index.js",
"scripts": {
Expand Down
11 changes: 5 additions & 6 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,9 +5,7 @@ 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 @@ -137,20 +135,23 @@ export default class ControlledTable extends Component<ControlledTableProps> {
return;
}

/*#if TEST_COPY_PASTE*/
const ctrlDown = isCtrlDown(e);

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

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

@wbrgss wbrgss Sep 14, 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.

The more this pops up the more I think we should keep track of custom vs default event handlers.

@Marc-Andre-Rivet Marc-Andre-Rivet Sep 14, 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.

Actually this one was already there, hidden away in onCopy, just made both consistent :)

/*#endif*/

@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.

Default handling of keys ends up changing the focus state, which affects how future keys are handled. 'Enter' is meant to set the focus "deeper" than the table, on the input itself, but not Ctrl+C / Ctrl+V. Exiting explicitly.

@chriddyp chriddyp Sep 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you explain what these /*#endif*/ comments are doing? templating in code for different builds?

@Marc-Andre-Rivet Marc-Andre-Rivet Sep 13, 2018 •
edited
Loading

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.

Yes. The test build has a special build-time variable defined

dash-table/webpack.test.config.js

Line 3 in 5fd75e3

definitions: ['TEST_COPY_PASTE']

The loader reads through the files and keeps/removes the blocks based on whether the variable is defined or not. This is useful for defining build specific behavior or configurations that need to be more flexible than overriding an alias/file and/or not have to carry the conditional logic for tests/dev/etc. into production code. Created this loader a while ago when I encountered test cases I couldn't run on Selenium because it wasn't possible to open a file selector and choose a file..
https://www.npmjs.com/package/webpack-preprocessor

@Marc-Andre-Rivet Marc-Andre-Rivet Sep 13, 2018 •
edited
Loading

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.

Specifically, in this case, Cypress does not have good support of copy & paste events, so instead of letting the browser handle it, triggering the copy/paste processing manually when the key combinations are hit. This creates a gap between the test impl and the real impl but the alternative is to manual test features.

return;
}
/*#endif*/

if (e.keyCode === KEY_CODES.ESCAPE) {
setProps({ is_focused: false });
Expand Down Expand Up @@ -452,8 +453,6 @@ export default class ControlledTable extends Component<ControlledTableProps> {
} = this.props;
const dataframe = virtualizer.dataframe;

e.preventDefault();

const columnIndexOffset =
(row_deletable ? 1 : 0) +
(row_selectable ? 1 : 0);
Expand Down
8 changes: 8 additions & 0 deletions tests/e2e/cypress/integration/navigation_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,14 @@ describe('navigate', () => {
});
});

// Issue: https://github.com/plotly/dash-table/issues/49
it.only('can move after ctrl+c', () => {
DOM.focused.type(`${Key.Meta}c`);
DOM.focused.type(Key.ArrowDown);
DashTable.getCell(4, 3).should('have.class', 'focused');
DashTable.getCell(3, 3).should('not.have.class', 'focused');
});

@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 test makes sure that we can navigate out of the cell once Ctrl+C has been pressed


it('can move down', () => {
DOM.focused.type(Key.ArrowDown);
DashTable.getCell(4, 3).should('have.class', 'focused');
Expand Down