141 move in cell by valentijnnieman · Pull Request #172 · plotly/dash-table

valentijnnieman · 2018-10-24T20:15:32Z

Closes #141, this allows users to use keyboard navigation (arrow keys) when editing a cell, i.e. when a cell's input is focused.

Marc-Andre-Rivet · 2018-10-24T20:43:16Z

tests/cypress/tests/server/dash_test.ts

+                expect($container.first()[0].innerText).to.equal(`[249][0] = ${initialValue} -> abc`);
+            });
+        });
+    });


Would like to see a test on edit first + arrowup

Marc-Andre-Rivet · 2018-10-24T20:51:36Z

src/dash-table/components/CellInput/index.tsx


    handleKeyDown = (e: KeyboardEvent) => {
-        if (e.keyCode !== KEY_CODES.ENTER && e.keyCode !== KEY_CODES.TAB) {
+        if (!isNavKey(e.keyCode)


I think this is wrong. Prior to this change, once you started modifying the content of the cell, the up/down arrow would send you to the beginning/end of the cell content. The left/right arrow in this mode would move you one character over to the left/right. Now, when editing up/down will leave & save (which is the desired behavior), but it's now impossible to navigate the content with left/right.

I think the change should only apply to up/down and leave left/right as is (user can still navigate to beginning/end of content by typing cmd/ctrl + left/right)

In all cases there is a potential usability issue here: either the user cannot navigate the cell content or they get a different behavior on up/down vs. left/right.

Fair point. I'm checking out Google spreadsheets and I'm noticing that if you click on the cell, you can edit it, and the arrow keys navigate the table. If you double click, you can edit the cell, and the arrow keys will move inside the input. You can edit the content and navigate, or if you really want to do specific edits you double click. Is that something we should do here as well? Was that the idea all along?

I am not, to be honest, super up to date with web usability/accessibility these days, but I could see that this might be an issue, since we're overriding the default <input> behaviour here.

@cldougl Would you take a look at this as well?

Marc-Andre-Rivet · 2018-10-24T20:54:49Z

Pulling @chriddyp as reviewer since he opened the issue.

Marc-Andre-Rivet

left/right arrow change in behavior affects usability negatively in my opinion / I think this should be reverted
want to make sure we engage in a usability discussion about the whole change

chriddyp · 2018-10-24T21:55:50Z

In all cases there is a potential usability issue here: either the user cannot navigate the cell content or they get a different behavior on up/down vs. left/right.

Yeah, this is subtle. I recommend looking at the behaviour of excel (if you have it), google spreadsheet, or the spreadsheet component we use in chart studio: https://plot.ly/create

In spreadsheets, there are two modes for cell editing. I've marked which cases work on master with an ✅ and which ones don't with an ✖️

focussed mode

✅ a cell is in focussed when you click on it (once) or when you navigate to it with the keyboard
✅ arrowing (in any direction) or tabbing in focussed mode changes the cells, keeps it in focussed mode
✅ typing in focussed mode overrides the cell's contents with what you have typed
✖️ enter moves the cell down (Excel) or into edit mode (Google Docs)
- we do this the google docs way rather than the excel way. we should do it the excel way if possible. i'm not concerned about this for launch.
✅ pasting a string will replace the cells contents with that string
✅ pasting a group of cells will paste the contents across those cells
visually, edit is distinct from focussed mode by:
a. ✅ not having a cursor
b. ✖️ not having any highlighted text
- 🌈 seeing the highlighted text when in focussed mode does not look right and it makes it harder to distinguish between the two modes. this would be a 'nice to have' for launch.
✖️ typing text keeps the cell in focussed mode: after typing, arrowing moves the cell, it does not move the cursor
- ☝️ this is the main issue with our current behavior and what I meant by up/down arrows should move cells up and down even if focussed inside the input #141. Not supporting this feels particularly un-intuitive when you attempt to fill out a series of cells in a column.
✅ delete deletes the cell
✅ escape reverts the edit back to what the cell's contents originally were

edit mode

✖️ a cell is in edit mode when you double-click
- slow double click works, but not the typical fast double-click
✅ a cursor for editing the contents of the text appears
in google docs, you can also get in edit mode by pressing enter while in focussed mode
✖️ In excel, F2 also gets you into edit mode
- i'm not concerned about this for launch
✅ arrowing left-right in edit mode changes the cursor by single character.
✅ arrowing up-down in edit mode moves the cursor to the front or the back of the cells contents
✅ enter moves the focus down one cell into focussed mode
✅ tab moves the focus to the right one cell into focussed mode
✖️ pasting a string will paste the contents in the cursor location
- i'm not concerned about this for launch
✖️ pasting a series of cells isn't allowed in excel. in google docs it will paste it as a multi-line string.
- we actually do neither of these - we have the same behaviour as focussed: it pastes into several cells. That's OK for now, since the way we distinguish between these two states isn't super tight or visually apparent. In the future, it'd be nice to tighten this up.
visually, edit is distinct from focussed mode by:
a. ✅ having a cursor
b. ✅ allowing text to highlight
✅ delete deletes the letter
✅ escape reverts the cell back to what the cell's contents originally were and transitions the cell into focussed mode
So, we're very close to having the exact excel-like behavior in master. The main thing that I'm concerned about for launch is Item 8 under focussed. The other ✖️ 's we should get to, but we can do it after launch.

I hope that clears things up. This stuff is really tricky, I have to manually go through the motions every time 🙈

valentijnnieman · 2018-10-24T21:59:34Z

@chriddyp Ah, so that sounds kind of what I'm describing in the comment above (#172 (comment)), yes? That makes sense to me. Thanks for the detailed list, really very interesting to think about these issues in depth and how users interact with this stuff!

chriddyp · 2018-10-24T22:15:39Z

Also note that there is a 3rd mode which I'll call "multi-select":

in "multi-select", there is the concept for an individual active cell which you can activate with tab:

multi-select

✅ shift-arrow-keys brings you into multi-select
✅ shift-click brings you into multi-select
✅ delete clears the contents of each cell
✅ tabbing around cells moves the "active" cell around
✅ arrow transitions the cell into focussed, and moves the active cell down
- ✅ note that the active cell may not necessarily be the first cell, as the user may have tabbed around in multi-select mode first
✅ the active cell acts like a focussed cell w.r.t typing and entering
✅ deleting will always delete the contents, even if the active cell has moved around

it's good just to be aware of this behavior as it also affects all of these navigation keys

chriddyp · 2018-10-24T22:16:29Z

i'll make new issues for the rest of the unsupported items and move them into the priority #2 column.

chriddyp · 2018-10-24T23:01:44Z

issue for the remaining items: #174

Marc-Andre-Rivet · 2018-10-25T13:11:31Z

@valentijnnieman Updated issue with additional tests in #141 (comment)

As is the PR breaks

edit mode

5. ✅ arrowing left-right in edit mode changes the cursor by single character.
6. ✅ arrowing up-down in edit mode moves the cursor to the front or the back of the cells

…1-move-in-cell

chriddyp

💯 💯 💯 the interactions are working very well now! Looks like you knocked off a few other items as well, like the differentiated styles between the focussed and active state.

💃 from me for the UI side of things

Marc-Andre-Rivet · 2018-10-30T17:59:53Z

tests/cypress/tests/server/copy_paste_test.ts

        it('on cell modification', () => {
            DashTable.getCell(0, 0).click();
            DOM.focused.type(`10${Key.Enter}`);
+            DashTable.getCell(1, 1).click();


Since navigation_test 'does focus on next cell input on text + "enter"' passes, this should work without the click. Test is moot right now :)

🐮 Test is moo!

Marc-Andre-Rivet

Lgtm :)

valentijnnieman added 4 commits October 24, 2018 15:32

Propagate to ControlledTable on navKeys

dfa9b84

Test if arrow keys can navigate and save cell value

0d60454

Add keyboard navigation tests for focused inputs

68f5043

Update changelog

c36364f

valentijnnieman requested a review from Marc-Andre-Rivet October 24, 2018 20:15

chriddyp temporarily deployed to dash-table-review-pr-172 October 24, 2018 20:15 Inactive

Marc-Andre-Rivet reviewed Oct 24, 2018

View reviewed changes

Marc-Andre-Rivet requested a review from chriddyp October 24, 2018 20:54

Marc-Andre-Rivet suggested changes Oct 24, 2018

View reviewed changes

valentijnnieman requested a review from cldougl October 24, 2018 21:37

chriddyp mentioned this pull request Oct 24, 2018

remaining spreadsheet navigation and editing UI updates #174

Open

Marc-Andre-Rivet mentioned this pull request Oct 25, 2018

up/down arrows should move cells up and down even if focussed inside the input #141

Closed

Add onMouseUp functionality for arrow key navigation

85cbd4c

chriddyp temporarily deployed to dash-table-review-pr-172 October 29, 2018 17:56 Inactive

Calculate derivedHandlers for onMouseUp

17f2117

chriddyp temporarily deployed to dash-table-review-pr-172 October 29, 2018 19:04 Inactive

Add test for caret selection on focused input

ba76b5a

chriddyp temporarily deployed to dash-table-review-pr-172 October 29, 2018 20:34 Inactive

Merge branch 'master' of https://github.com/plotly/dash-table into 14…

ce87a93

…1-move-in-cell

chriddyp temporarily deployed to dash-table-review-pr-172 October 29, 2018 20:41 Inactive

Rebuild bundle

2f6345e

chriddyp temporarily deployed to dash-table-review-pr-172 October 29, 2018 20:54 Inactive

Jump to next cell on enter key

a222ea1

chriddyp temporarily deployed to dash-table-review-pr-172 October 30, 2018 14:34 Inactive

Fix focused input test by doubleclicking instead of enter

6a9ba60

chriddyp temporarily deployed to dash-table-review-pr-172 October 30, 2018 15:26 Inactive

Remove .only from copy paste test

a633f50

chriddyp temporarily deployed to dash-table-review-pr-172 October 30, 2018 15:27 Inactive

chriddyp approved these changes Oct 30, 2018

View reviewed changes

Marc-Andre-Rivet reviewed Oct 30, 2018

View reviewed changes

Remove artifact from it's tomb of copy_paste_test

eec1f44

chriddyp deployed to dash-table-review-pr-172 October 30, 2018 18:05 View deployment

Marc-Andre-Rivet approved these changes Oct 30, 2018

View reviewed changes

valentijnnieman merged commit 5be420b into master Oct 30, 2018

valentijnnieman deleted the 141-move-in-cell branch October 30, 2018 18:17

Uh oh!

Conversation

valentijnnieman commented Oct 24, 2018

Uh oh!

Marc-Andre-Rivet Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

Marc-Andre-Rivet Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

Marc-Andre-Rivet Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

valentijnnieman Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

valentijnnieman Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

Marc-Andre-Rivet commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Marc-Andre-Rivet left a comment

Choose a reason for hiding this comment

Uh oh!

chriddyp commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

valentijnnieman commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chriddyp commented Oct 24, 2018

Uh oh!

chriddyp commented Oct 24, 2018

Uh oh!

chriddyp commented Oct 24, 2018

Uh oh!

Marc-Andre-Rivet commented Oct 25, 2018

Uh oh!

chriddyp left a comment

Choose a reason for hiding this comment

Uh oh!

Marc-Andre-Rivet Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentijnnieman Oct 30, 2018

Choose a reason for hiding this comment

Uh oh!

Marc-Andre-Rivet left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Marc-Andre-Rivet commented Oct 24, 2018 •

edited

Loading

chriddyp commented Oct 24, 2018 •

edited

Loading

valentijnnieman commented Oct 24, 2018 •

edited

Loading

Marc-Andre-Rivet Oct 30, 2018 •

edited

Loading