Datagrid remaining keyboard shortcuts

[ffknob]

Summary

(Related to #2482)

Implements some of the missing key shortcuts for the data grid: Home, End, Ctrl+Home, Ctrl+End, Page Down, Page Up

Checklist

- [ ] Checked in dark mode
- [ ] Checked in mobile

• Checked in Firefox
• Checked in IE11

- [ ] Props have proper autodocs
- [ ] Added documentation examples

• Added or updated jest tests
• Checked for breaking changes and labeled appropriately
• Checked for accessibility including keyboard-only and screenreader modes
• A changelog entry exists and is marked appropriately

[elasticmachine]

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

[myasonik]

@chandlerprall Should we move the whole switch statement to an if/else? Having to do this feels silly...

[ffknob]

I took the liberty to go ahead and make the change as suggested by @myasonik . Feel free to let me know if you want the switch statement back.

[myasonik]

@ffknob could you also add tests for each of the new keyboard shortcuts added?

[ffknob]

@ffknob could you also add tests for each of the new keyboard shortcuts added?

Just did... Good opportunity to learn a little bit about testing framework.

[myasonik]

I tried taking a look at why setFocusedCell wouldn't work after the pagination change and I couldn't figure out a solution. I can spend some more time on it later this week but we shouldn't merge until this is fixed.

My best guess is that changing the page changes what setting [x, y] refers to so it's effectively trying to do that on a page that doesn't exist anymore. @chandlerprall maybe you know a solution off the top of your head?

[ffknob]

I'm having a hard time trying to simply resolve the merge conflict in the CHANGELOG.md. I am not a git expert, so maybe you could give me a hand here

1. I have forked the repo, cloned my fork, added a remote upstream pointing to the original repo and then set branch --set-upstream-to=upstream/master.
2. I then have checked master out and pulled from the upstream. After that I did a git push origin to resync my fork's master
3. After that I checked my branch out and did a merge master, which changed a lot of files and resulted in a simple conflict in CHANGELOG.md
4. I tried to solve this conflict manually (I think I managed to do that)
5. But when I run add CHANGELOG.md and then commit I get a lot of errors from the test phase of the Yarn flow, errors that I didn't get before and that are not related to my changes, something like:

src-docs/src/views/selectable/selectable_single.tsx:34:10 - error TS7006: Parameter 'list' implicitly has an 'any' type.

34         {list => list}
            ~~~~

src/components/badge/badge.tsx:27:5 - error TS2304: Cannot find name 'Omit'.

27 } & Omit<HTMLAttributes<HTMLButtonElement>, 'onClick' | 'color'>;
       ~~~~

src/components/badge/badge.tsx:29:22 - error TS2304: Cannot find name 'Omit'.

29 type WithSpanProps = Omit<HTMLAttributes<HTMLSpanElement>, 'onClick' | 'color'>;

What did I do wrong?

[ffknob]

I'm having a hard time trying to simply resolve the merge conflict in the CHANGELOG.md. I am not a git expert, so maybe you could give me a hand here

1. I have forked the repo, cloned my fork, added a remote upstream pointing to the original repo and then set branch --set-upstream-to=upstream/master.
2. I then have checked master out and pulled from the upstream. After that I did a git push origin to resync my fork's master
3. After that I checked my branch out and did a merge master, which changed a lot of files and resulted in a simple conflict in CHANGELOG.md
4. I tried to solve this conflict manually (I think I managed to do that)
5. But when I run add CHANGELOG.md and then commit I get a lot of errors from the test phase of the Yarn flow, errors that I didn't get before and that are not related to my changes, something like:

src-docs/src/views/selectable/selectable_single.tsx:34:10 - error TS7006: Parameter 'list' implicitly has an 'any' type.

34         {list => list}
            ~~~~

src/components/badge/badge.tsx:27:5 - error TS2304: Cannot find name 'Omit'.

27 } & Omit<HTMLAttributes<HTMLButtonElement>, 'onClick' | 'color'>;
       ~~~~

src/components/badge/badge.tsx:29:22 - error TS2304: Cannot find name 'Omit'.

29 type WithSpanProps = Omit<HTMLAttributes<HTMLSpanElement>, 'onClick' | 'color'>;

What did I do wrong?

Never mind... It turns out I had to yarn first in order to get the dependencies before I tryied to commit.

[ffknob]

I tried taking a look at why setFocusedCell wouldn't work after the pagination change and I couldn't figure out a solution. I can spend some more time on it later this week but we shouldn't merge until this is fixed.

My best guess is that changing the page changes what setting [x, y] refers to so it's effectively trying to do that on a page that doesn't exist anymore. @chandlerprall maybe you know a solution off the top of your head?

eui/src/components/datagrid/data_grid_body.tsx

Line 187 in e929330

[onCellFocus, rowMap, pagination]

I don't think setCellFocus() in data_grid_body.tsx is being fired for the next/previous page event... When I console.log() its entry point I get a log for every other movement key pressed and even for the mouse selection of a cell, but not for the Pg Down/Pg Up events (although the page do gets changed).

Perhaps something is missing in this hook in order to fire the callback up? I tried adding pagination!.pageIndex with no luck.

(I have almost no experience with React, but I'll try to investigate further... we are almost there to get this mergeable)

[chandlerprall]

Thanks for putting all of this together! I haven't reviewed all of the changes, only looked so far at handling of page up & page down, and have a couple changes:

• page down / up are swapped, page down should advance forward a page and page up move back
• preventDefault call should happen inside the props.pagination conditional
• props.pagination.pageIndex was modified directly instead of interacting with the existing callback. Directly manipulating props violates React's principles and introduces subtle bugs.
• moves focus to the top of the already-focused columns

refactored:

} else if (keyCode === keyCodes.PAGE_DOWN) {
      if (props.pagination) {
        event.preventDefault();
        const totalRowCount = props.rowCount;
        const pageIndex = props.pagination.pageIndex;
        const pageSize = props.pagination.pageSize;
        const pageCount = Math.ceil(totalRowCount / pageSize);
        if (pageIndex < pageCount) {
          props.pagination.onChangePage(props.pagination.pageIndex + 1);
          setFocusedCell([focusedCell[0], 0]);
        }
      }
    } else if (keyCode === keyCodes.PAGE_UP) {
      if (props.pagination) {
        event.preventDefault();
        const pageIndex = props.pagination.pageIndex;
        if (pageIndex > 0) {
          props.pagination.onChangePage(props.pagination.pageIndex - 1);
          setFocusedCell([focusedCell[0], 0]);
        }
      }
    }

I don't yet have a solution for re-focusing the target cell after pagination. The code below correctly sets the value (you can page down & tab & shift+tab and the correct cell is focused). The problem is on pagination all the cells unmount and are recreated, and there is no existing lifecycle point for a cell to focus itself on mount, especially since it needs to know if focus was previously owned on the table. Thoughts include:

• cells add a data- attribute with their column/row position and DataGrid can query for the intended cell, calling .focus() on it
• data grid can create a context
  • cells could pass their DOM node to the data grid context, and data grid could directly call .focus() on the cell
  • cells can subscribe to a refocus event on the context, and the active cell can re-focus its own DOM

I'm leaning toward the last option as it relies on a pure (thin) abstraction instead of sharing/leaking DOM elements.

[ffknob]

• page down / up are swapped, page down should advance forward a page and page up move back

Yes, instinctively I think this is the best way (when I was testing I kept trying to go forward by pressing Page Down), but I only swap those on purpouse because I thought that the reasoning should be "Page Up" for incresing page numbers and "Page Down" for decresing them... Bu as I said, as a future user of this feature I totally agree with you, they were swapped

• preventDefault call should happen inside the props.pagination conditional

Ok

• props.pagination.pageIndex was modified directly instead of interacting with the existing
  callback. Directly manipulating props violates React's principles and introduces subtle bugs.

I understand that, but this way I couldn't get the tests to pass anymore... It looked a lot like the problem was with the paging (LEFT/RIGHT/HOME/END worked, but the PG UP/PG DOWN had no effect, the cell text value received was always like no page movement had happened)

• moves focus to the top of the already-focused columns

Nice, but I changed a little bit to focus not only on the same column, but also in the same row. Let me know if you think that's not the best approach and I revert to same column/top row.

refactored:

Thanks for that!

I don't yet have a solution for re-focusing the target cell after pagination. The code below correctly sets the value (you can page down & tab & shift+tab and the correct cell is focused). The problem is on pagination all the cells unmount and are recreated, and there is no existing lifecycle point for a cell to focus itself on mount, especially since it needs to know if focus was previously owned on the table. Thoughts include:

• cells add a data- attribute with their column/row position and DataGrid can query for the intended cell, calling .focus() on it

• data grid can create a context

  • cells could pass their DOM node to the data grid context, and data grid could directly call .focus() on the cell
  • cells can subscribe to a refocus event on the context, and the active cell can re-focus its own DOM

I'm leaning toward the last option as it relies on a pure (thin) abstraction instead of sharing/leaking DOM elements.

This goes way beyond my React knowledge (I work with Angular). If you could point me an article I'd gladly try to solve this for finally finishing this PR. Did you mean the React.createContext() API?

Thanks for the code review!

[chandlerprall]

Nice, but I changed a little bit to focus not only on the same column, but also in the same row. Let me know if you think that's not the best approach and I revert to same column/top row.

I thought this was stated in the spec, but I just checked and there isn't a mention either way. @myasonik do you have any thoughts here?

This goes way beyond my React knowledge (I work with Angular). If you could point me an article I'd gladly try to solve this for finally finishing this PR. Did you mean the React.createContext() API?

I think the steps here are:

• create a DataGrid context, using React.createContext
• use <DataGridContext.Provider value={...}>...existing data grid children...</DataGridContext.Provider> to expose the context to any children.
  • for value's, er, value, we need an object with a onFocusUpdate method, which will be called by each cell with a listener method; it should return a new function that will unsubscribe the listener
  • onFocusUpdate stores the listeners it receives and calls them in response to sellCellFocus, passing the new focusedCell array to the callbacks
• As EuiDataGridCell is a React.Component (class component) instead of a function component, we cannot use the useContext hook, instead the legacy contextType enables the component to see the context's value
  • on mount, EuiDataGridCell should call onFocusUpdate as provide its callback (e.g. its existing updateFocus method). Store the unsubscribe method returned by onFocusUpdate on the EuiDataGridCell instance
  • on unmount, call the stored unsubscribe method

That should set up the overall communication. There's probably something I left out, or a small edge case lurking in the details - feel free to ask for any clarification, etc! We've been wanting to move some other props into a context, so this is a big step toward supporting that in addition to solving the page up/down focus.

[myasonik]

For page up and page down, I think staying on the same row and column position is appropriate 👍 (I wanted to restate the whole sentence to make sure we're on the same page. So if I misunderstood please let me know!)

[ffknob]

Hi @chandlerprall
Not quite there yet, but I gave a shot on the roadmap you sent in your last post. It was challenging for me because I know almost 0 React, but I think I managed to:

• Create a context for the DataGrid component
• Pass it through a Provider
• Catch this context in the gran child component (Cell)
• In the cell component I call onFocusUpdate in order to register its updateFocus in the data grid compoent and also to get a unsubscribe function
• In my tests I console.loged this and it seems that the cells are now communicating with the data grid
• I also created a local structure to hold all the updateFocus and delete them as the cells unsubscribe
• The last part would be to call the respective updateFocus for the new focused cell, but here I am having trouble calling this from the createKeyDownHandler... my matrix looks always empty from there (but when I console.log the cells subscriptions I can see that it do gets populated)

So I am currently stucked in perhaps some Javascript context/scope issue. I have to stop for today but next time I'll try to figure this out and also take a look at the test case @myasonik mentioned.

Sorry if I misunderstood something from the steps you described.

[ffknob]

Some progress...
I used the useState in order to solve my issue with the updateFocus matrix not being available when needed. It seems to work.

The problem I have now, and I think this will be the last one before we can definetly see if this will work or not, is to where to call updateFocus.

Here is the order in which things are happening currently:

1. Datagrid component is instantiated
2. All cells subscribe to the datagrid registering its updateFocus functions in the componentDidMount
3. Page Down
4. Problem: I'm calling updateFocus at the wrong moment, so it gets called before the current page cells unsubscribe and the next page cells subscribe, so it ends up calling the current page focused cell updteFocus, which doesn't solve the issue
5. All current page cells call its unsubscribeCell function in the componentWillUnmount
6. All next page cells register its updateFocus functions in the componentDidMount
7. Here is when the updateFocus should be called for the cell at the new page, I just don't know in which point fo the code should I call the update...

I know that the point where I am currently calling updateFocus is not the right place to do so, but as of now I don't know where it should be called...

[chandlerprall]

Nice! That implementation is slick, and I especially like the nested arrays on cellsUpdateFocus, gives a much cleaner way to call the specific cell's update.

The calls to updateFocus are really close - we usually solve this in EUI by wrapping the call in a requestAnimationFrame, e.g. requestAnimationFrame(() => updateFocus([focusedCell[0], rowIndex]));

[ffknob]

requestAnimationFrame(

It worked! Nice tip

The only thing is that if the user is really fast (smashing the Page Down button) it will probably hit the "not focused in the grid" state and then perform a page down in the page...

[chandlerprall]

Thanks for all the effort on this, @ffknob ! I'm ready to do a full review pass on the changes, and will include a look at that not focused in the grid state (I definitely encountered it when testing the requestAnimationFrame solution).

Knowing you are newer to React, I don't want to throw a bunch of suggestions/requests your way without reasons. Would you prefer:

• github review - detailed inline comments against your PR
• PR against your branch - I can make the changes in a new PR against your branch, leaving comments in that PR with explanations

[ffknob]

Thanks for all the effort on this, @ffknob ! I'm ready to do a full review pass on the changes, and will include a look at that not focused in the grid state (I definitely encountered it when testing the requestAnimationFrame solution).

Knowing you are newer to React, I don't want to throw a bunch of suggestions/requests your way without reasons. Would you prefer:

• github review - detailed inline comments against your PR
• PR against your branch - I can make the changes in a new PR against your branch, leaving comments in that PR with explanations

Hi @chandlerprall

at this point I think I prefer that you "PR against my branch". You'll definetly "Reactify" this implementation way faster than I could and I think I now have runned out of all little React that I knew... I'll definetly learn by your refactory and will appreciate any comments that will may leave in the PR.
I think this would be the best way to fast track this changes into master.

Thank you for all the support throughout this PR. It was really a very good opportunity to start to learn about the project, React, Jest and more.

Will be waiting for your PR!

[chandlerprall]

@ffknob PR is created

There was really only one issue, where the props.pagination object was being modified directly instead of respecting the data/callback lifecycle. The other modifications are mostly for readability.

I didn't address the page up/down focus moving away from the grid during the page change, as it started ballooning into unrelated changes. I'll follow up with that in a future datagrid pass.

[chandlerprall]

jenkins test this

[ffknob]

@ffknob PR is created

There was really only one issue, where the props.pagination object was being modified directly instead of respecting the data/callback lifecycle. The other modifications are mostly for readability.

I didn't address the page up/down focus moving away from the grid during the page change, as it started ballooning into unrelated changes. I'll follow up with that in a future datagrid pass.

Thanks for that @chandlerprall

I think I managed to get your refactory merged into my branch and then updated this PR. (let me know if I did something wrong)
Do you think we are good to go now?

[chandlerprall]

Thanks! Everything looks good to me. I've asked @myasonik if he wants to take a final look and he should have time today for that.

[myasonik]

I didn't address the page up/down focus moving away from the grid during the page change, as it started ballooning into unrelated changes. I'll follow up with that in a future datagrid pass.

I just went through the PR and it works as expected (in that, your focus goes to the correct cell) in FF, Chrome, and Safari! I couldn't test IE11 though, Browserstack refused to load today for some reason... Could someone else check it? But that's pretty good results for something we expected to be a bug!

This looks great, thank you for all your work @ffknob and thank you for the assist @chandlerprall!

[myasonik]

Once someone can verify that nothing breaks in IE11, this is good to merge

[myasonik]

@ffknob I hope you don't mind, I updated the PR description a touch (mostly touching the checklist)

[ffknob]

@ffknob I hope you don't mind, I updated the PR description a touch (mostly touching the checklist)

Not at all

[ffknob]

I didn't address the page up/down focus moving away from the grid during the page change, as it started ballooning into unrelated changes. I'll follow up with that in a future datagrid pass.

I just went through the PR and it works as expected (in that, your focus goes to the correct cell) in FF, Chrome, and Safari! I couldn't test IE11 though, Browserstack refused to load today for some reason... Could someone else check it? But that's pretty good results for something we expected to be a bug!

This looks great, thank you for all your work @ffknob and thank you for the assist @chandlerprall!

Glad to help!
Couldn't have done without the assist from @chandlerprall 🍻

[chandlerprall]

Tested the new shortcuts in IE11 & Edge, everything works as expected.