Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EuiDataGrid] Implement remaining keyboard shortcuts #2482

Closed
5 tasks done
myasonik opened this issue Oct 24, 2019 · 10 comments · Fixed by #6036
Closed
5 tasks done

[EuiDataGrid] Implement remaining keyboard shortcuts #2482

myasonik opened this issue Oct 24, 2019 · 10 comments · Fixed by #6036

Comments

@myasonik
Copy link
Contributor

myasonik commented Oct 24, 2019

  • page_down scrolls the grid, page, or both so that the currently visible bottom most row because the top most; sets focus to the cell in the same column in the new top most row
  • page_up opposite of page_down
  • home/end moves focus to the first/last cell in the row that contains focus; does nothing if already there
  • ctrl+home/end moves focus to the first/last cell in the first/last row of the current page; does nothing if already there
    - [ ] [alphanumeric keys] if a cell contains an input widget, places focus on the first one, and inserts key
  • Add a keyboard shortcuts reference somewhere in the datagrid itself

Spec reference: Keyboard Interaction For Data Grids

@ffknob
Copy link
Contributor

ffknob commented Nov 12, 2019

Hello @myasonik

I'm giving a shot with this one. What I managed to do so far is:

  • Home: ok
  • End: ok
  • Ctrl+Home: ok
  • Ctrl+End: ok

What is still missing:

Page Down / Page Up:

I could think of an easy way to identify the bottom most visibile row. My plan is to iterate the rows comparing each one's offset top (offsetTop) qith the window inner height (window.innerHeight), so when I reach the first one off screen I then set the focus in the previous one.

The thing is, as I understand I'll only be able to get the elements position inside the body part of the data grid component. I'll probably have to pass a new function down, similar to the setCellFocus in order to have the body telling the main component which one is the last visible row in the screen.

I would have a similar approach for the upper most row (Page Up).

Alphanumeric:

" if a cell contains an input widget, places focus on the first one, and inserts key"

I didn't undertand that. What do you mean by "the first one"? Woudn't the each cell have only one widget? Or do you mean "the first cell of the row which have a widget" (in the case of a tabular form, for instance)

"Inserts key"? Do you mean inserts the value of the alphanumeric key in the widget, replacing its current value?

@chandlerprall
Copy link
Contributor

For Page Down & Page Up, the spec says

Moves focus down an author-determined number of rows, typically scrolling so the bottom row in the currently visible set of rows becomes one of the first visible rows. If focus is in the last row of the grid, focus does not move.

Since our grid is paginated instead of a giant sheet with all the values, I think it makes more sense for page up / page down to move to the next logical page, and the correct cell should scroll into view once it is focused.

Alphanumeric input is a bit odd to make work in the DOM. On alphanumeric keypress:

  • if the current cell has one or more focusable elements, then:
    • move focus to the first element
    • replay the keypress, targeted at the now-focused first element

@myasonik
Copy link
Contributor Author

For Page Down & Page Up, I like the idea of switching to the next/previous page. I'd then set focus onto the first cell of the grid body.

For alphanumeric keys it's actually a bit trickier than that. We can't rely only on what is focusable because if the cell contents is something like <button>Delete this field</button><input />, the keypress needs to be sent to the input, not to the button. For now, I think we can define it as input, textarea, or elements with contenteditable="true". If we think of other elements that accept alphanumeric keys, we can add them later.

@ffknob
Copy link
Contributor

ffknob commented Nov 12, 2019

For Page Down & Page Up, I like the idea of switching to the next/previous page. I'd then set focus onto the first cell of the grid body.

For alphanumeric keys it's actually a bit trickier than that. We can't rely only on what is focusable because if the cell contents is something like <button>Delete this field</button><input />, the keypress needs to be sent to the input, not to the button. For now, I think we can define it as input, textarea, or elements with contenteditable="true". If we think of other elements that accept alphanumeric keys, we can add them later.

For the Page Down / Page Up I managed to get the grid to walk though the pages, but although I managed to get the next result set in the grid, I couldn't get it to keep the focus in the data grid component. I'll have to take a better look... (but if you have any tip, I would appreciate that)

For the alphanumeric keys should I give it a try? Check the current cell for a input-text/textarea element and then set the focus and change its value accordingly to the pressed key. Is that it?

@myasonik
Copy link
Contributor Author

For the alphanumeric keys should I give it a try? Check the current cell for a input-text/textarea element and then set the focus and change its value accordingly to the pressed key. Is that it?

Yup, that sounds correct! If you want to give it a go, that'd be awesome! But you can also punt on that and leave it unimplemented. Not all the keyboard shortcuts have to be implemented at the same time. Totally up to you.

@myasonik
Copy link
Contributor Author

although I managed to get the next result set in the grid, I couldn't get it to keep the focus in the data grid component

setFocusedCell should do that for you so if that's not working there might be a bug in that function. Not really sure outside of that but if you're super stuck on it, @chandlerprall or I can dive into the code sometime. Just let us know if you need an assist and we can take a deeper look.

@ffknob
Copy link
Contributor

ffknob commented Nov 12, 2019

Yeah I think I'm gonna wrap it around here... Perhaps the remaining feature could be moved to a new issue.

But if you have some tip about the problem of losing focus after page change, I'd be happy to fix it. (perhaps tomorrow I'll give a new look at this particular issue)

Here is what I've done in the last commit:

  • Wrote some tests for the new keybindings
  • Took the liberty to transform the switch statement into an if/else if (as suggested, but I can bring it back if you prefer)
  • Fixed the Ctrl+somekey (turns out I have to check for the event.ctrlKey boolean value, and not keycode 17 + some other key)
  • setFocusedCell() helps with the whole refresh of the grid after changing page, but it does not keep the focus on the data grid (perhaps a bug as mentioned)
  • Included a change log line

@myasonik
Copy link
Contributor Author

With many of these shortcuts about to land, we should also think about how we can let users know they're available.

We can let screen reader users know with some hidden text but I think the keyboard shortcut info would probably be beneficial to all users so we should do something that everyone can use.

@snide What do you think to about adding a little "info" popover maybe in the top right of the toolbar where we can let people know about all the shortcuts? (Can use either the ⓘ icon or the keyboard icon for the button.)

@chandlerprall
Copy link
Contributor

Home/End/PageUp/PageDown were added with #2519

@snide
Copy link
Contributor

snide commented Nov 20, 2019

@myasonik Yep. I can make something like that. I'll add it as a checklist item for this issue.

@cchaos cchaos changed the title [DATA GRID] Implement remaining keyboard shortcuts [EuiDataGrid] Implement remaining keyboard shortcuts Mar 18, 2020
@cchaos cchaos added the meta label Mar 18, 2020
@snide snide mentioned this issue Mar 9, 2021
41 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants