Compact interactive list keyboard navigation #1919

adoroshk · 2023-11-30T00:03:01Z

Summary

This PR adds keyboard navigation to compact interactive list component.

What was changed:
Following functionality has been added:

The list is a single roving tab stop.
Right/Left Arrows navigate across visual rows. When focus hits the end of the visual row navigation stops.
Up/Down Arrows navigate across visual columns. Navigating down from the bottom of a visual column takes you to the corresponding cell at the top of the next visual column and vice versa (except for the last and first items).
Moves to the first cell in the focused visual row
- Windows
  - Home
- Mac
  - Home
  - Fn + Left Arrow (equivalent to Home)
  - Cmd + Left Arrow
Moves to the last cell in the focused visual row
- Windows
  - End
- Mac
  - End
  - Fn + Right Arrow (equivalent to End)
  - Cmd + Right Arrow
Moves to the first cell in the first item (top left)
- Windows
  - Ctrl + Home
- Mac
  - Cmd or Ctrl + Home
  - Cmd or Ctrl + Fn + Left Arrow (equivalent to Home)
  - Cmd + Ctrl + Left Arrow
Moves to the last cell in the last item (bottom item in rightmost visual column)
- Windows
  - Ctrl + End
- Mac
  - Cmd or Ctrl + End
  - Cmd or Ctrl + Fn + Right Arrow (equivalent to End)
  - Cmd + Ctrl + Right Arrow
If cell has an interactive element (only one per cell allowed), focus is set to that element, not the cell (only one interactive element per cell allowed) and the arrow keys behavior is default to that element (NOTE: currently it causes problems with input elements that won't allow to navigate away from that with keyboard, a decision needs to be maid by UX team on what is the best way around that).
Cells are select-able by Space key (it triggers onClick for cell) unless the cell has an interactable element within, in that case its not selectable neither via mouse click nor per space key.

Testing

Note: at that moment tests are work in progress.
This change was tested using:

Reviews

In addition to engineering reviews, this PR needs:

UX review
Accessibility review
Functional review

Additional Details

This PR resolves:

UXPLATFORM-9788

Co-authored-by: Saad Adnan <[email protected]>

…s added

…om/cerner/terra-framework into compact_interactive_list_keyboard_navigation

cm9361 · 2023-12-11T20:42:21Z

packages/terra-compact-interactive-list/src/CompactInteractiveList.jsx

  columnMinimumWidth: PropTypes.string,

  /**
-   * Columns maximum width should be a valid css string in value in px, em, or rem units..
+   * Columns maximum width should be a valid css string in value in px, em, or rem units.
   */
  columnMaximumWidth: PropTypes.string,


Wouldn't the minimum and maximum widths be part of the columnShape? I would expect to be able to control the values for each column.

There are 2 ways to control column max and min width:
Option 1: specify min and max width for a specific column as a part of column shape prop:

terra-framework/packages/terra-compact-interactive-list/src/proptypes/columnShape.js

Line 27 in 466e8c8

minimumWidth: PropTypes.string,

Option 2: (the one you are looking at) columnMaximumWidth and columnMinimumWidth prop of the whole component, this way they'll be applied to each column.

Option 1 takes precedence on Option 2, so if there is columnMaximumWidth set to the whole component and hence applied to all columns, but then some columns have minimumWidth (or maximumWidth) props set for these columns directly, the last ones would be applied to those columns.

eawww · 2023-12-11T20:47:51Z

@mjpalazzo @adoroshk I wouldn't want to give overly verbose instructions detailing every possible interaction, especially since that also needs to be available to non screen reader users, but something like "Use arrow keys to navigate and space or enter to activate a cell" would probably be helpful.
We still need to make the external instructions components for all these grids.

cm9361 · 2023-12-11T20:49:16Z

packages/terra-compact-interactive-list/src/CompactInteractiveList.jsx

+  const onFocus = (event) => {
+    if (!event.currentTarget.contains(event.relatedTarget)) {
+      // Not triggered when swapping focus between children
+      focusCell(focusedCell.current);


What happens if the list was updated while focus was lost. You will end up with a similar exception that we had to fix for the WorklistDataGrid.

Good catch. In this commit I changed the focused cell data to row and column ids instead of indexes: 396ed59

cm9361 · 2023-12-11T20:56:23Z

packages/terra-compact-interactive-list/src/utils/keyHandlerUtils.js

+export const getFocusableElements = (parentElement) => [...parentElement.querySelectorAll(`${focusableElementSelector}`)].filter(
+  element => !element.hasAttribute('disabled')
+      && !element.getAttribute('aria-hidden')
+      && window.getComputedStyle(element).visibility !== 'hidden',
+);


We have a similar method in focusManagement.js in the Table component. I would think we would not want to duplicate it all. Same comment for the other methods.

Actually it's a bit different. I have simplified this one a bit as the one you are referring to checks for offsets for cases when components are not hidden but rather moved outside of the container to not be visible for sighted user. It was messing with jest enzyme tests and as I don't expect the cell having that king of "hidden" elements, I simplified it.

You should not need to change it. You just need to update your jest tests appropriately. If you check the other jest tests, there is code to handle providing an offset value. The logic should work for your component.

@cm9361 huge thank you for the help with tests to accommodate the full original logic from terra-table. I fixed the tests and currently added getFocusableElements is the original one that is implemented in terra-table.

In order to reuse the same logic rather than copy-paste it, I added the export for the getFocusableElements method from the terra-table. Once the terra-table is released, I will remove copied getFocusableElements method from the compact interactive list and attach the one exported from the terra-table. Would that work?
The commit: cdbf56a

You can use a relative path instead of having to add a package dependency. That logic was released in terra-table quite a while ago though, if I am not mistaken.

Correct, imported from terra-table: e7343db

cm9361 · 2023-12-11T21:00:39Z

packages/terra-compact-interactive-list/src/subcomponents/Cell.jsx

+  /**
+   * The cell's column position in the table. This is zero based.
+   */
+  columnIndex: PropTypes.number,


Shouldn't the cell also have a column id?

UPD: there is a full column info passed a s a column prop (needed for cell max/min width, etc). It also has a column id.

… compact_interactive_list_keyboard_navigation

cm9361 · 2023-12-11T21:03:34Z

packages/terra-compact-interactive-list/src/subcomponents/Cell.jsx

+  const handleKeyDown = (event) => {
+    // does not need setFocusedCell call as the cell focused by key is tracked in CompactInteractiveList component
+    const key = event.keyCode;
+    if (key === KeyCode.KEY_SPACE && isSelectableCell) {


So a cell cannot be selected if it doesn't have interactable elements?

Per @eawww at that moment (unless it changes in the future) the cell can NOT be selected If there is an interactable element in it (and there can be only one intreactive element per cell).

cm9361 · 2023-12-11T21:04:58Z

packages/terra-compact-interactive-list/src/utils/keyHandlerUtils.js

+    return focusedCell;
+  }
+  // focus should go to the next semantic row unless the last row
+  return { row: row + 1 < rowsLength ? row + 1 : row, cell };


You probably could simplify this by using the Math.min function.

Not sure I got how to do that. An example will be appreciated.

eawww

Thank you @adoroshk! It works fantastically.

One last request

Could you add to the prop description for numberOfColumns: "Number of rows is calculated as the number of items divided by the number of columns, rounded up." or if there's a more accurate wording, that'd be cool.

adoroshk · 2023-12-12T02:23:17Z

Thank you @adoroshk! It works fantastically.

One last request

Could you add to the prop description for numberOfColumns: "Number of rows is calculated as the number of items divided by the number of columns, rounded up." or if there's a more accurate wording, that'd be cool.

@eawww added the description here:

terra-framework/packages/terra-compact-interactive-list/src/CompactInteractiveList.jsx

Line 73 in 396ed59

    
              * A number of visual columns. Defaults to 1. Number of visual rows is calculated as the number of items divided by the number of columns, rounded up.

kenk2

Just a nit and a question, but otherwise looks good.

kenk2 · 2023-12-12T23:04:14Z

packages/terra-compact-interactive-list/src/utils/keyHandlerUtils.js

+ * @param {number} rowsLength - a total number of seamntic rows in the list.
+ * @returns - a { row, cell } indexes pair to focus on.
+ */
+export const handleDownKey = (focusedCell, numberOfColumns, flowHorizontally, rowsLength) => {


nit: I'm wondering if there was a way to not have to delegate all the directional keys to a unique function. I see that in each handleXKey function, we're re-stating a lot of the same logic and params across each function. In the DataGrid, we generally were able to keep all the directional handles in the same function.

Up to you on how/when/if to address this.

I understand your concern, I'll look into shared logic between these methods and see if it can be improved in my next PR, this one gets too complex to monitor the changes.

kenk2 · 2023-12-12T23:09:43Z

packages/terra-compact-interactive-list/src/CompactInteractiveList.jsx

+        event.preventDefault();
+        break;
+      case KeyCode.KEY_LEFT: {
+        moveFocusTo = handleLeftKey(event, moveFocusTo, numberOfColumns, flowHorizontally, columns.length, rows.length);


Do we need to event.preventDefault() on the other navigation actions? I would think so as all of these are related to navigation.

Good catch on preventDefault inconsistency. Actually, we don't need the preventDefault on Up and Down keys, as there is one preventDefault for all cases after the switch. Escape and Tab bave return at the end, so they need their own preventDefaults, but arrow keys have break, not return and go to the end of the method to reach the preventDefault there and I just missed that. I removed extra in this commit: a6bcd2f

adoroshk and others added 30 commits October 26, 2023 09:51

initial setup

d015127

package.json fix

7183a0c

snapshot updated

a639575

terra-table one more url fixed

0cd19f2

Update NOTICE

f30994e

Co-authored-by: Saad Adnan <[email protected]>

Merge branch 'main' into compact_interactive_list_component_setup

23db101

validators and constants removed, shapes updated

4392403

removed headers classname

862cf1d

layout components added

fb7800e

Merge branch 'main' into compact_interactive_list_component_layout

fa4f287

Basic layout examples added, styles adjusted

dc7fe0f

examples added

5385822

VIsualRow component removed

2397f1f

em support, min width support added

0490e2a

examples refactored.

382d5a3

added vertical placement and placeholer cells

aebba6a

css lint error fixed

8737387

css lint errors fixed

e75cc58

Compact Interactive list maxWidth and minWidth jest tests and example…

6e5b5a0

…s added

wdio and jest tests added

b954478

fixed width wdio added

fe7bd82

Merge branch 'main' into compact_interactive_list_component_layout

89e55cb

prop warnings fix

9fbb954

renaming methods and updating screenshots

fde184e

flowHorizontally renamed to flowHorizontally

164cba7

test fix per remanimg

d6b0a56

left and right arrow key navigation added

37f3e44

tests added for inconsistent width type

8ed28d2

changelog updates

aa60d9b

Merge branch 'compact-interactive-list-add-tests' of https://github.c…

7dbc9e1

…om/cerner/terra-framework into compact_interactive_list_keyboard_navigation

cm9361 reviewed Dec 11, 2023

View reviewed changes

Merge branch 'main' of https://github.com/cerner/terra-framework into…

d67f62d

… compact_interactive_list_keyboard_navigation

cm9361 reviewed Dec 11, 2023

View reviewed changes

eawww approved these changes Dec 11, 2023

View reviewed changes

focused cell data switched to ids from indexes

396ed59

github-actions bot temporarily deployed to preview-pr-1919 December 12, 2023 02:20 Destroyed

adoroshk added ⭐ UX Reviewed UX Reviewed and approved. and removed UX Review Required UX Review Ready Accessibility Review Required Accessibility Review Ready labels Dec 12, 2023

use getFocusableElements logic from terra-table

cdbf56a

github-actions bot temporarily deployed to preview-pr-1919 December 12, 2023 20:09 Destroyed

changed import for getFocusableElements to be from terra-table component

e7343db

github-actions bot temporarily deployed to preview-pr-1919 December 12, 2023 21:12 Destroyed

reverted changes to terra-table

611e666

github-actions bot temporarily deployed to preview-pr-1919 December 12, 2023 21:51 Destroyed

cm9361 approved these changes Dec 12, 2023

View reviewed changes

kenk2 approved these changes Dec 12, 2023

View reviewed changes

removed unnessesary event.preventDefault

a6bcd2f

github-actions bot temporarily deployed to preview-pr-1919 December 12, 2023 23:39 Destroyed

adoroshk merged commit 5dc1d7e into main Dec 13, 2023
21 checks passed

adoroshk deleted the compact_interactive_list_keyboard_navigation branch December 13, 2023 06:35

adoroshk mentioned this pull request Dec 19, 2023

Compact interactive list implementation guide added #1950

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compact interactive list keyboard navigation #1919

Compact interactive list keyboard navigation #1919

adoroshk commented Nov 30, 2023 •

edited by eawww

Loading

cm9361 Dec 11, 2023

adoroshk Dec 11, 2023 •

edited

Loading

eawww commented Dec 11, 2023 •

edited

Loading

cm9361 Dec 11, 2023

adoroshk Dec 12, 2023

cm9361 Dec 11, 2023

adoroshk Dec 11, 2023

cm9361 Dec 12, 2023

adoroshk Dec 12, 2023 •

edited

Loading

cm9361 Dec 12, 2023

adoroshk Dec 12, 2023

cm9361 Dec 11, 2023

adoroshk Dec 11, 2023 •

edited

Loading

cm9361 Dec 11, 2023

adoroshk Dec 11, 2023

cm9361 Dec 11, 2023

adoroshk Dec 12, 2023

eawww left a comment

adoroshk commented Dec 12, 2023

kenk2 left a comment

kenk2 Dec 12, 2023

adoroshk Dec 12, 2023

kenk2 Dec 12, 2023 •

edited

Loading

adoroshk Dec 12, 2023

Compact interactive list keyboard navigation #1919

Compact interactive list keyboard navigation #1919

Conversation

adoroshk commented Nov 30, 2023 • edited by eawww Loading

Summary

Testing

Reviews

Additional Details

Choose a reason for hiding this comment

adoroshk Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

eawww commented Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adoroshk Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adoroshk Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eawww left a comment

Choose a reason for hiding this comment

adoroshk commented Dec 12, 2023

kenk2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenk2 Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adoroshk commented Nov 30, 2023 •

edited by eawww

Loading

adoroshk Dec 11, 2023 •

edited

Loading

eawww commented Dec 11, 2023 •

edited

Loading

adoroshk Dec 12, 2023 •

edited

Loading

adoroshk Dec 11, 2023 •

edited

Loading

kenk2 Dec 12, 2023 •

edited

Loading