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

Add table block keyboard navigation #16957

Closed
wants to merge 17 commits into from
Closed

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Aug 8, 2019

Status: This is blocked by #17084, which will move some of the functionality from this PR to a WritingFlow.

Description

This PR implements improved keyboard navigation within the table block. Previously only tab could be used to navigate cells.

This PR implements the following changes:

  • Arrow keys are now used to navigate cells along with HOME and END. The Cmd/Ctrl modifiers can be used to go directly to the start/end of a column, row, or the entire table (when used with HOME and END)
    table-arrow-nav

  • Tab / Shift+Tab is now used to navigate directly to from an individual cell within the table. A user is able to navigate to a cell using arrow keys, shift tab to the toolbar to make some changes to the table, then tab back to the last selected cell. Previously this was quite hard to achieve as tabbing to the toolbar also changed the selected cell.
    table-tab

The implementation follows the described behaviours here and uses a roving tabIndex:
https://www.w3.org/TR/wai-aria-practices/examples/grid/dataGrids.html

The changes here open up the possibilities for other future enhancements to the table block:

  • Multi-cell selection using the keyboard (holding down shift with navigation keys).
  • Merging of cells (colspan/rowspan), which will require multi-selection.
  • The improvements to keyboard usage also help considerably with implementing column/row selection (Table column and row selection #16493), which is why I switched to working on this first.

How has this been tested?

Added e2e and unit tests

Manual testing:

  1. Add a table block
  2. Toggle on the header and footer.
  3. Use cursor keys to navigate around. Ensure all cells are navigable.
  4. Type some text.
  5. Use cursor keys to navigate through the cell with text.
  6. Use HOME and END to navigate to the start and end of the row.
  7. Use Cmd/Ctrl with arrow keys and HOME and END
  8. Use shift tab and tab to navigate out of the table and back in to the last selected cell.

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@talldan talldan added [Type] Enhancement A suggestion for improvement. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Status] In Progress Tracking issues with work in progress [Block] Table Affects the Table Block labels Aug 8, 2019
@talldan talldan self-assigned this Aug 8, 2019
@talldan talldan force-pushed the add/table-arrow-key-nav branch 2 times, most recently from 1b17951 to 840f48a Compare August 12, 2019 03:32
@talldan talldan added Needs Accessibility Feedback Need input from accessibility and removed [Status] In Progress Tracking issues with work in progress labels Aug 12, 2019
@talldan talldan force-pushed the add/table-arrow-key-nav branch from fc74d6f to 11d0961 Compare August 14, 2019 05:53
@talldan talldan changed the title WIP: Add table block keyboard navigation Add table block keyboard navigation Aug 14, 2019
@talldan talldan marked this pull request as ready for review August 14, 2019 05:54
@talldan talldan requested a review from ajitbohra as a code owner August 14, 2019 05:54
Copy link
Member

@ellatrix ellatrix left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

Aside from the few comments I left, I have some bigger thoughts on the implementation. I feel like we're reimplementing a lot of what WritingFlow does. WritingFlow was never written with tables in mind, so that's why it currently doesn't play very nice with it. I think horizontal arrow keys work well, but vertical arrow keys should move vertically across cells instead of horizontally. To me this seems like an implementation issue with WritingFlow: the component should always look at the visual order of things, instead of the DOM order. I've done some rewriting before to reflect this in #14462, but only within the isEdge function, not within the WritingFlow component. I believe we should consider extending this to the WritingFlow. I think I already wrote this down somewhere on an issue related to table selection or navigation, but I can't find it now. (Edit: see #14675.)

const columnCount = get( state, [ sectionName, previousRowIndex, 'cells', 'length' ], 0 );
const hasCellAbove = columnIndex < columnCount;

return hasCellAbove ? {
Copy link
Member

Choose a reason for hiding this comment

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

Minor: I think if ( columnIndex < columnCount ) return would read better.

@@ -80,6 +80,10 @@ _Type_

Keycode for DOWN key.

<a name="END" href="#END">#</a> **END**

Keycode for SPACE key.
Copy link
Member

Choose a reason for hiding this comment

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

This sentence seems incorrect, same below.


// Signal our intention to handle the event by disallowing any
// native or other event handling. Even if the next cell to
// navigate to can't be found, selection should still remain in
Copy link
Member

Choose a reason for hiding this comment

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

Why?


return getLocationOfCellAbove( tableState, selectedCell );
}
if ( isDown ) {
Copy link
Member

Choose a reason for hiding this comment

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

else if?

@ellatrix
Copy link
Member

See also #14675. It would also be good to split this into multiple pieces:

  • Horizontal arrow key navigation works.
  • Make vertical arrow key navigation work in the WritingFlow component.
  • Think about HOME and END separately. This is probably something that applies to many blocks in general, not just the table block. If I have multiple fields in one block, we can take the selection to the first/last field. If that field is already selected, we should maybe take the user to the first/last block. We do cmd+a similarly.

@mapk
Copy link
Contributor

mapk commented Aug 14, 2019

I just tested this and it's working wonderfully! It's so great to be able to arrow through the cells now. Thanks for this. 🌟

  • Arrow keys worked both horizontally and vertically for me.
  • Shift + Tab brought me to the block toolbar where I was able to quickly and easily insert another row.
  • Cmd + Arrow key took me to either the end/beginning of a row/column.

@talldan
Copy link
Contributor Author

talldan commented Aug 15, 2019

Thanks for all the feedback. 😄

@ellatrix It does seem like the NavigableTable part of the implementation could be moved to WritingFlow.

I think WritingFlow would need a way detect when it should use grid-based navigation.

It could check for particular parent elements (table, td, ...) or aria attributes (aria-role="grid", aria-rowindex, ...).

It'd be pretty amazing if that worked out of the box. I'll do some investigation.

edit: re-reading your comments, I see what you mean about 'visual positioning', which would mean looking for a physically positioned element below the current selection.

@ellatrix
Copy link
Member

Thanks @talldan, that would be great! I strongly believe it all belong in WritingFlow. :) Imagine a block implements a table with a set amount of rows and columns to only let the user fill in some data. It should work there as well. I can think of many more use cases like grids etc., as you mentioned.

@talldan talldan added the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Dec 5, 2019
@talldan talldan closed this Mar 19, 2020
@aristath aristath deleted the add/table-arrow-key-nav branch November 10, 2020 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Table Affects the Table Block [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Accessibility Feedback Need input from accessibility [Status] Blocked Used to indicate that a current effort isn't able to move forward [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants