eui icon indicating copy to clipboard operation
eui copied to clipboard

[EuiDataGrid] Add keyboard shortcuts reference

Open cee-chen opened this issue 3 years ago β€’ 17 comments
trafficstars

Summary

closes https://github.com/elastic/eui/issues/2482, see also https://github.com/elastic/kibana/issues/90515 for reference

⚠️ WIP, opening for early staging QA/testing and feedback

@miukimiu Just opening this for a quick reference - I'd love to get your thoughts on the design, styling, etc:

  1. Is this too large for a popover? Would a modal be better?
  2. Does an EuiDescriptionList work best for this? Or would, e.g. a table or similar be better?
  3. I don't love the "info" icon but I don't think the "keyboardShortcut" icon is much better, it looks a bit confusing. Is there any chance we could add a keyboard icon that looks maybe something like this? image

CC @1Copenut as well for any thoughts here

Checklist

  • [ ] Checked in both light and dark modes
  • [ ] Checked in mobile
  • [ ] Checked in Chrome, Safari, Edge, and Firefox
  • [ ] Props have proper autodocs and playground toggles
  • [ ] Added documentation
  • [ ] Checked Code Sandbox works for any docs examples
  • [ ] Added or updated jest and cypress tests
  • [ ] Checked for breaking changes and labeled appropriately
  • [ ] Checked for accessibility including keyboard-only and screenreader modes
  • [ ] Updated the Figma library counterpart
  • [ ] A changelog entry exists and is marked appropriately

cee-chen avatar Jul 07 '22 21:07 cee-chen

Tagging along as a reviewer. Linking to the wider Kibana issue discussing global keyboard shortcuts for visibility. I think we can use this as a starting point for consistent UX and to road test a11y.

1Copenut avatar Jul 07 '22 21:07 1Copenut

Preview documentation changes for this PR: https://eui.elastic.co/pr_6036/

[CONSTANCE EDIT] Moving my original PR description to this comment to maintain conversation clarity

⚠️ WIP, opening for early staging QA/testing and feedback

@miukimiu Just opening this for a quick reference - I'd love to get your thoughts on the design, styling, etc:

  1. Is this too large for a popover? Would a modal be better?
  2. Does an EuiDescriptionList work best for this? Or would, e.g. a table or similar be better?
  3. I don't love the "info" icon but I don't think the "keyboardShortcut" icon is much better, it looks a bit confusing. Is there any chance we could add a keyboard icon that looks maybe something like this? image

kibanamachine avatar Jul 07 '22 22:07 kibanamachine

Hi @constancecchen πŸ‘‹πŸ½ ,

  1. I think the popover works.
  2. A basic table works well.
  3. There was a discussion about the keyboardShortcut icon (https://github.com/elastic/eui/issues/1573). And there were some attempts to add a keyboard instead of just keys and it didn't work out. So I think is better if we stick with the current keyboardShortcut.
Screenshot 2022-07-12 at 15 35 49

For the keyboard shortcut what do you think of using the tag <kbd> instead of a <EuiBadge>? But I think we need better styles for the <kbd> tag which relates to this issue: https://github.com/elastic/eui/issues/5016. I can prioritize this issue and add a new component.

I did some tests in a CodeSandbox and with what I suggested the popover would look like the following screenshot:

Screenshot 2022-07-12 at 15 21 22

CodeSandbox design idea: https://codesandbox.io/s/modest-jepsen-6xsj0v?file=/demo.js

Note The kbd styles might change a little. But the styles were based on what we have in Figma: https://www.figma.com/file/RzfYLj2xmH9K7gQtbSKygn/Elastic-UI?node-id=13648%3A4134

elizabetdev avatar Jul 12 '22 14:07 elizabetdev

Love it, thanks Elizabet! <kbd> definitely makes more sense. One quick note, I don't think the table needs to have a responsive mode as the popover is already very small. What do you think?

Re the keyboardShortcut icon, we chatted about this in sync today and it sounds like we may be revisiting it - I can use it for now and we can contemplate updating it in-place later.

cee-chen avatar Jul 12 '22 19:07 cee-chen

One quick note, I don't think the table needs to have a responsive mode as the popover is already very small. What do you think?

@constancecchen I agree. The table doesn't need the responsive mode. I updated the CodeSandbox.

elizabetdev avatar Jul 14 '22 11:07 elizabetdev

New icon

@constancecchen here are a few ideas for the new icon:

Screenshot 2022-07-14 at 20 48 30

I'm not 100% convinced these icons work in 16px x 16px but let me hear your opinion.

The iInCircle icon

Another option would be to keep your initial idea of using the iInCircle icon. I think it works better than the old keyboardShortcut icon. Also, it aligns with other "product's help" across Kibana and Cloud as you can see in our In product help guidelines.

Just to show an example in Discover:

Screenshot 2022-07-14 at 15 51 23

Prototype with the iInCircle and new icon

Here are a few mocks with both iInCircle and the new icon in the toolbar or footer: Figma prototype.

In the prototype, one of the ideas is to keep the icon in the toolbar.

Screenshot 2022-07-14 at 20 57 34 Screenshot 2022-07-14 at 21 02 52

Another idea would be to align with the markdown editor and keep the "help" in the footer:

Screenshot 2022-07-14 at 20 57 52 Screenshot 2022-07-14 at 20 58 50

Let me hear your thoughts.

elizabetdev avatar Jul 14 '22 20:07 elizabetdev

I love it - I like the 6th keyboard icon the most personally! πŸŽ‰

I'd push against putting the keyboard shortcuts button on the bottom bar / next to the pagination because of how that affects screen reader users - they won't even get to that by the time they've finished interacting with the grid.

EDIT: Also worth mentioning, I'm not strongly against using the help icon, but I think I'd prefer a keyboard icon for EuiDataGrid specifically. This is for several reasons:

  1. It really does only contain keyboard shortcuts, unlike the Kibana examples where the popover is explaining other things
  2. EuiDataGrid has a specific need for defining keyboard shortcuts: normal screen reader and keyboard navigation (e.g. tab, ctrl+opt+arrow keys) does not work as normal / as expected. These keyboard shortcuts are not optional enhancements on top of existing behavior; it is necessary that we note that there is a specific way of interacting with EuiDataGrid, or we risk SR/keyboard users not knowing what to do.
  3. We're trying to establish a pattern for Kibana to use for defining keyboard shortcuts in the future (https://github.com/elastic/kibana/issues/90515), and I think it makes sense that there will be more keyboard shortcut popovers in the future in Kibana

cee-chen avatar Jul 18 '22 15:07 cee-chen

@1Copenut Hey Trevor! Before I switch this branch over to using a table layout format for the keyboard shortcuts, could I get your opinion on the screen reader experience of it?

<table>: https://codesandbox.io/s/modest-jepsen-6xsj0v?file=/demo.js <dl>/<dt>/<dd>: https://eui.elastic.co/pr_6036/#/tabular-content/data-grid

I could definitely be wrong here, but I'm surprisingly not a super huge fan of the table experience. I think the screen reader ends up reading a lot of cruft like 'row X of Y' and mashes the column title + cell contents together and it leads to a more distracting experience with that information than without (the description list just reads out a '20 of 24' at the end of the item).

I could be totally wrong about that not being useful info though - WDYT?

cee-chen avatar Jul 18 '22 19:07 cee-chen

Preview documentation changes for this PR: https://eui.elastic.co/pr_6036/

kibanamachine avatar Jul 18 '22 20:07 kibanamachine

@constancecchen I'm leaning toward this as a description list too. It's subtle, but my thinking is that the description list is used to define key / value pairs. I feel our intended audience for the aural readback will be familiar with what each term means, so having table columns read each time could be extraneous.

The only thing I might suggest adding would be an aria-labelledby="HEADING_ID" to the DL tag that references a heading in the popover. That gives the list a unique name in SR Lists menus, and reinforces the relationship between the heading as proxy for table columns.

1Copenut avatar Jul 19 '22 20:07 1Copenut

Awesome, thank you @1Copenut! πŸ™Œ

@miukimiu Are you OK with moving forward with using EuiDescriptionList instead of EuiBasicTable? It changes the visual appearance somewhat, but if you want I can try to get back to that visual appearance w/ CSS alone rather than a table structure. For what it's worth, I feel fairly strongly about this as the biggest beneficiaries of the keyboard shortcuts panel is keyboard/screen reader users πŸ˜…

EDIT: any custom CSS tweaking should perhaps wait until Bree is done converting EuiDescriptionList in #5971. This PR can definitely hang around until then, there's no rush!

cee-chen avatar Jul 19 '22 21:07 cee-chen

Preview documentation changes for this PR: https://eui.elastic.co/pr_6036/

kibanamachine avatar Jul 19 '22 21:07 kibanamachine

@miukimiu Are you OK with moving forward with using EuiDescriptionList instead of EuiBasicTable?

Yes, I'm ok with that. I'll update CodeSandbox to reflect that.

elizabetdev avatar Jul 20 '22 16:07 elizabetdev

I'd like to make one quick suggestion on the formatting of the popover contents in an effort to make it more concise. Here's a screenshot:

image

  1. Use the emoticon where possible to signify the correct key. It's much easier to scan for ↓ than arrow down.
  2. Combine some of the simple & similar shortcuts in a comma separated list
  3. Use bold format to highlight the key differences in the command
  4. Try not to use terms like "popover" but instead "details"

Also, I'd label this "keyboard navigation" vs "shortcuts" because "shortcuts" assumes there's a menu item associated with the command. But really, this is just using the keyboard to traverse the elements.

If we want to think scalability, I'd probably opt for the iInCircle icon as the triggering button so that this popover could then contain more information in the future.


One item I'm still not clear on is the page up/down navigation. I can't tell if something is broken or why it's not another "focus" type of navigation.

cchaos avatar Jul 25 '22 17:07 cchaos

Use the emoticon where possible to signify the correct key. It's much easier to scan for ↓ than arrow down.

I'm super digging this! Screen readers are reading the arrows out perfectly as well (although a quick note that we do need to use the actual unicode arrow characters vs emojis)

Combine some of the simple & similar shortcuts in a comma separated list

I'm not 100% confident about this but I strongly worry about the cognitive separation here for screen reader users, who will be navigating through 4 different arrow keys and then just "moves cell focus".

For what it's worth, when I separated each individual arrow key description, I was following W3's pattern of listing shortcuts: https://www.w3.org/WAI/ARIA/apg/patterns/grid/#keyboard-interaction-for-data-grids

Sadly W3 doesn't have a pattern/spec for listing keyboard shortcuts (https://www.w3.org/WAI/ARIA/apg/practices/keyboard-interface is fairly vague / doesn't mention how to tell consumers what your keyboard shortcuts are), so I don't have a lot of other examples/standards to reference, but as another example GitHub also separates out each arrow key: https://docs.github.com/en/get-started/using-github/keyboard-shortcuts#network-graph

@1Copenut, any thoughts?

Try not to use terms like "popover" but instead "details"

Sure, but for SR users we should somehow inform them that not all cells have details / are interactive (in case they press enter on a non-interactive cell and get nothing). It's probably also worth mentioning that cell actions are available in the details (because cell actions are not available or even mentioned to non-sighted keyboard users except through the interactive cell popover).

If you really want to be brief for sighted users, we could take advantage of SR-only text to do something like:

Opens cell details <EuiScreenReaderOnly>for interactive cells that contain cell actions or overflow data. These details open in a new dialog.</EuiScreenReaderOnly>.

Also, I'd label this "keyboard navigation" vs "shortcuts" because "shortcuts" assumes there's a menu item associated with the command. But really, this is just using the keyboard to traverse the elements.

Looks like https://www.w3.org/WAI/ARIA/apg/practices/keyboard-interface agrees with you - it primarily references keyboard shortcuts in terms of 'assigning and revealing' and otherwise uses 'keyboard navigation'.

Honestly though, I think that line is pretty blurry (e.g. looking at GitHub shortcuts again, many of these are navigation adjacent - although of course many aren't).

Re: page up/down navigation - it should be focusing the same cell column but in the last/next page's last/first row. Unfortunately W3 has no examples of data grids with pagination so we're deviating from their spec there / making up our own rules for page up/down.

If we want to think scalability, I'd probably opt for the iInCircle icon as the triggering button so that this popover could then contain more information in the future.

I would strongly rather keep the focus of this popover on keyboard accessibility. As I mentioned earlier, screen reader and keyboard users unfamiliar with the spec and standard keyboard interaction for data grids will potentially either not know or be unable to fully interact with the grid otherwise. The standard SR way of browsing through items will fail them for grids that scroll due to the virtualization library, as our grid only scrolls to cells out of view via arrow keys and not via SR arrow navigation.

It's also worth noting the custom logic I added to this button, while consumers can opt to 'hide' this button via toolbarVisibility.showKeyboardShortcuts: false, it only visually hides the button - keyboard users who tab in sequence through the data grid toolbar will always have these keyboard shortcuts available to them to view. I don't think this paradigm is necessary for other random information.

My goal with this PR was to establish a pattern that Kibana could reference or use in https://github.com/elastic/kibana/issues/90515 and get ahead of a few issues (such as a few we've already solved, e.g. should we use a table or a description list? is the icon clearly readable as a keyboard?). While it's not 1:1 because EuiDataGrid doesn't exactly have 'shortcuts' or assigning shortcuts as you mentioned Caroline, nevertheless I still think it's worth keeping keyboard documentation as a first class citizen.

cee-chen avatar Jul 25 '22 22:07 cee-chen

Combine some of the simple & similar shortcuts in a comma separated list

I'd opt to leave the list as it is now, with each navigation key / chord having its own definition. It's verbose but there's a clear relationship between key and value in each pair.

Also, I'd label this "keyboard navigation" vs "shortcuts" because "shortcuts" assumes there's a menu item associated with the command. But really, this is just using the keyboard to traverse the elements.

I concur this feels like navigation. If we use this nomenclature for data grids, we can extend and use "shortcuts" when we expand this UI into Kibana custom commands. At that point it really is a shortcut; we'd be removing steps between A and B.

If we want to think scalability, I'd probably opt for the iInCircle icon as the triggering button so that this popover could then contain more information in the future.

I second keeping the focus of this popover and its icon on keyboard navigation. The navigation pattern we're looking to establish is specifically for data grids and is wholly driven by key presses. Let's keep the ilnCircle for a Yes And when we start introducing keyboard shortcuts in Kibana.

1Copenut avatar Jul 26 '22 18:07 1Copenut

It's a compromise between re-stating obvious/common navigational patterns and highlighting those that are unique. The cell expansion ones are the most unique but are at the bottom of the list most likely hidden by scroll if every item is listed by itself. We don't have to be quite as brief as Moves cell focus, but could be Moves cell focus up, down, left, right respectively.

I only mention the scalability because the toolbars are getting quite cramped and most of the time these keyboard commands charts are a one-time necessity, yet it'll be displayed on all (possibly multiple on one page) data grids all the time. I'm not saying they're not helpful but I'd be careful how close to and combined with actual data grid controls.

cchaos avatar Jul 26 '22 20:07 cchaos

πŸ‘‹ Hey there. This PR hasn't had any activity for 90 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

github-actions[bot] avatar Oct 25 '22 00:10 github-actions[bot]

@miukimiu Any chance I could get your help with the "Updated the Figma library counterpart" step? πŸ™

cee-chen avatar Oct 27 '22 19:10 cee-chen

Preview documentation changes for this PR: https://eui.elastic.co/pr_6036/

kibanamachine avatar Oct 27 '22 20:10 kibanamachine

Preview documentation changes for this PR: https://eui.elastic.co/pr_6036/

kibanamachine avatar Oct 27 '22 21:10 kibanamachine

@florent-leborgne can you help us with this PR?

This PR adds a keyboard shortcuts popover to the EuiDataGrid toolbar. The primary target audience of this functionality is low visual screen reader users who otherwise have no indication of how to interact with the data grid.

This is the current implementation that can be found here. To open the popover you just need to click the keyboard icon of the data grid:

Screenshot 2022-10-28 at 12 35 49

Our questions are:

1. Should we be using the word "popover" or "details"? For example:

  • "Opens cell expansion popover for interactive cells" or "Open cell details"
  • "Closes any open popovers." or "Close cell details/Exits full screen"

2. Should the title of the popover be "Keyboard shortcuts" or "Keyboard navigation"? 3. Should we end each line with a dot?

Also, feel free to suggest other text changes.

elizabetdev avatar Oct 28 '22 11:10 elizabetdev

Thanks @miukimiu for pinging me.

  1. Should we be using the word "popover" or "details"? For example:

"Opens cell expansion popover for interactive cells" or "Open cell details" "Closes any open popovers." or "Close cell details/Exits full screen"

I think details is much simpler. "Open/Close cell details" feels very simple in comparison to the full technical description of what really happens.

  1. Should the title of the popover be "Keyboard shortcuts" or "Keyboard navigation"?

Keyboard shortcuts is fine. Small (maybe unrelated) question though: Those are for Windows (ctrl + home). Do you plan on displaying equivalent Mac shortcuts as well (usually fn+arrows for more advanced moves)?

  1. Should we end each line with a dot?

I think no punctuation is fine. And no 3rd person needed as well. Here is what I came up with, with these changes and a few other minor tweaks: image

Let me know what you think, I'm happy to iterate.

florent-leborgne avatar Oct 28 '22 13:10 florent-leborgne

Thanks, @florent-leborgne for being so fast. All the changes look good to me. @constancecchen do you agree with the changes?

Small (maybe unrelated) question though: Those are for Windows (ctrl + home). Do you plan on displaying equivalent Mac shortcuts as well (usually fn+arrows for more advanced moves)?

I pass this question to @constancecchen. She's implementing the code.

elizabetdev avatar Oct 28 '22 13:10 elizabetdev

Just for reference there are other shortcut sections in some places of the products (see screenshot as example). Would this become the new standard for all shortcut references?

image

florent-leborgne avatar Oct 28 '22 14:10 florent-leborgne

Just for reference there are other shortcut sections in some places of the products (see screenshot as example). Would this become the new standard for all shortcut references?

@florent-leborgne we would like to enforce the usage of the EuiText <kbd> HTML element that was introduced in EUI v61.0.0. This element is used to indicate text that represents keyboard input from a user’s computer.

  • This would ensure all shortcuts would visually look similar
  • Also a better semantic usage

Would this information be a good candidate to be part of "in product assistance guidelines"?

elizabetdev avatar Oct 28 '22 14:10 elizabetdev

Would this information be a good candidate to be part of "in product assistance guidelines"?

@miukimiu Yes totally!

florent-leborgne avatar Oct 28 '22 15:10 florent-leborgne

One last suggestion on the text (thanks @gchaps); How about using bold format to highlight the key differences (like Caroline suggested earlier in this PR). I think it would help scanning the list a bit faster.

florent-leborgne avatar Oct 28 '22 15:10 florent-leborgne

I apologize in advance if I appear argumentative, it's totally not my intention. But I want to stress a few things with this PR:

  1. The goal of this PR is accessibility for non-sighted users. While visual/mouse users benefit from knowing keyboard shortcuts, they don't need this information in the same way that non-sighted users do. Screen reader users literally have no other guidance than this popover on how to correctly interact with the data grid, as the normal method of screen reader navigation in EuiDataGrid does not really work.
  2. With that context in mind, while I appreciate the desire for brevity, I strongly think we need to err on the side of being explicit. Basically my litmus is, "if a blind user would not know otherwise know this information, do we need to spell out? Yes."
  3. I want to mention that the majority of this copy I originally cribbed from W3 itself. W3C essentially sets the standard for accessibility and accessibility examples.

With that context in mind, I wanted to answer a few questions:

"Opens cell expansion popover for interactive cells" or "Open cell details" [...] I think details is much simpler. "Open/Close cell details" feels very simple in comparison to the full technical description of what really happens.

One minor objection - the cell expansion popover does not necessarily just contain more details. It also contains any truncated/overflowing cell actions that keyboard users literally cannot access outside of the expansion popover. The cell content itself is secondary to the fact that cell actions (i.e., interactive cells) must be accessed by keyboard user via popover, hence the stress on 'interactive cells'.

That being said, I get that it's wordy, and now that I write it out my main issue with the simplification, I think we can find a compromise. "Open cell details & actions" might sufficiently cover what I'm trying to convey πŸ˜… WDYT? Any suggestions?

Those are for Windows (ctrl + home). Do you plan on displaying equivalent Mac shortcuts as well (usually fn+arrows for more advanced moves)?

For this I'm following W3's example which is only listing "Control". I think that might be because 90%~ of screen reader users are on Windows however (per WebAIM's 2021 survey). @1Copenut, any thoughts on this? Should we try to write a component that detects the user OS and displays Cmd instead?

(Ctrl) Home/End copy suggestions

"end/beginning of the row" is not enough information for a screen reader keyboard user. IMO, they need to know that it's the first cell/beginning of the row of the cell they are currently focused on (i.e., the current row). I don't think it's implied which row, otherwise. I also really think the ctrl descriptions are slightly misleading as it's not the first/last cell of the entire table (which may be paginated) - it's the first cell of the first row of the current page, and last cell of the last row of the last page.

I also personally prefer keeping the 'cell' copy between descriptions to make it clear that keyboard navigation centers around cells, but maybe that's just me.

How about using bold format to highlight the key differences (like Caroline suggested earlier in this PR). I think it would help scanning the list a bit faster.

Unfortunately this makes i18n translation a fairly annoying headache and I'm not confident it will translate well between languages. I would rather leave this alone (while screen readers do pick up emphasis, I'm not really sure that would provide all that much context to them). I don't feel very strongly about this however so am open to pushback if we really feel bolding is that useful.

cee-chen avatar Oct 28 '22 16:10 cee-chen

Great discussion! I agree 100% with @constancecchen that explicit, clear explanations are our goal for this shortcut reference. This sets us up well for future shortcut menus when there's questions about clarity vs. brevity. I almost always come down on the side of verbosity and explicit instruction for assistive technology. Instructions that are present are easy to skip, but implied instructions are sometimes hard to deduce.

I'm quote answering the two items from comments above that I can add some insight.

That being said, I get that it's wordy, and now that I write it out my main issue with the simplification, I think we can find a compromise. "Open cell details & actions" might sufficiently cover what I'm trying to convey πŸ˜… WDYT? Any suggestions?

This feels like a good compromise for clarity with brevity. If I was reading this with nothing else to go on, I could infer the cell had more information (details) and possibly interactions I could drive (actions).

Those are for Windows (ctrl + home). Do you plan on displaying equivalent Mac shortcuts as well (usually fn+arrows for more advanced moves)?

We won't need to add alternatives for the Ctrl + Home and Ctrl + End chords. Safari + VO on MacOS uses the Control key to activate these controls. Pressing Cmd + Home takes you to the Apple website by default in Safari.

1Copenut avatar Oct 28 '22 17:10 1Copenut