Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

[terra-data-grid] Flowsheet keyboard navigation #1927

Merged
merged 10 commits into from
Dec 7, 2023

Conversation

cm9361
Copy link
Contributor

@cm9361 cm9361 commented Dec 4, 2023

Summary

What was changed:
The Terra Table and Flowsheet components were updated to facilitate proper keyboard navigation within the Flowsheet data grid component.

Why it was changed:
The ability to use keyboard navigation is a core accessibility requirement of a grid control.

Testing

This change was tested using:

  • WDIO
  • Jest
  • Visual testing (please attach a screenshot or recording)
  • Other (please describe below)
  • No tests are needed

Reviews

In addition to engineering reviews, this PR needs:

  • UX review
  • Accessibility review
  • Functional review

Additional Details

N/A

This PR resolves:

UXPLATFORM-9403
UXPLATFORM-9911


Thank you for contributing to Terra.
@cerner/terra

@cm9361 cm9361 marked this pull request as ready for review December 5, 2023 14:16
text={text}
isOpen={hasSectionButton ? !isCollapsed : undefined}
isTitleFixed
onClick={hasSectionButton ? handleClick : undefined}
Copy link
Contributor

@adoroshk adoroshk Dec 5, 2023

Choose a reason for hiding this comment

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

I observed that if SPACE button was pressed on focused section, it does both - first opens the section, then closes it. Per discussion with @cm9361 it become clear that the second toggle is happening due to section header button being triggered as well.

I guess it can be fixed with onKeyDown event handler added to intercept a SPACE key press only:

Suggested change
onClick={hasSectionButton ? handleClick : undefined}
onClick={hasSectionButton ? handleClick : undefined}
onKeyDown={handleKeyDown}

where

  const handleKeyDown = (event) => {
    const key = event.keyCode;

    // Intercept the SPACE key only
    if (key === KeyCode.KEY_SPACE) {
      onSectionSelect(id);
      event.preventDefault();
    }
  };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will have to make an update in terra-core to fix this behavior. I don't think the update should be part of this PR.

@adoroshk
Copy link
Contributor

adoroshk commented Dec 5, 2023

I tested the flowsheet sections keyboard navigation. Observed results:

  1. The Arrow Up/Down keys navigation works as expected. If the section header is met, it gets focus. After focus leaves the section header, the next cell that gets focus keeps the original column order, which is expected behavior.
  2. The Arrow Left/Down keys navigation preserved within regular cells, including when ctrl + cmd + left arrow moves focus to the section header (as it's the first cell). However, once the focus is on the first cell which is the section header, ctrl + cmd + right arrow won't move focus to the last cell, as it would for any other cell, which probably is not the expected behavior?

// -------------------------------------

const isGridActive = grid.current?.contains(document.activeElement);
Copy link
Contributor

Choose a reason for hiding this comment

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

💪 Nice use of JS to track the grid active state!

}

setFocusedRowCol(toCell.row, toCell.col, true);
};

const handleColumnSelect = useCallback((columnId) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Will we have to account for scenarios where a cell/column header can be selected without having been reached from a mousedown or keyboard navigation? I would not think so but removing these could affect that scenario if it is a concern. I know that JAWS has a quick navigation ability so I was wondering about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This event only triggered for the mouse down and spacebar being pressed. I think this new implementation is equivalent.


// browser.keys(['Shift', 'Tab']);
// expect(browser.$('thead th:nth-child(2)').isFocused()).toBe(true);
// });
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Why is this being commented out? We should be testing that the last available column is selected if the last focused column index doesn't exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should not be removing the column during the click event. We are mutating the object in the middle of an event. There should be another way to remove the column such as a button.

Copy link
Contributor

@kenk2 kenk2 Dec 5, 2023

Choose a reason for hiding this comment

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

In that case, I believe we should make a comment that this test ought to be changed. We clearly should be testing this behavior but if this was implemented incorrectly, then we should take a note of that to be addressed?

@cm9361
Copy link
Contributor Author

cm9361 commented Dec 5, 2023

I tested the flowsheet sections keyboard navigation. Observed results:

  1. The Arrow Up/Down keys navigation works as expected. If the section header is met, it gets focus. After focus leaves the section header, the next cell that gets focus keeps the original column order, which is expected behavior.
  2. The Arrow Left/Down keys navigation preserved within regular cells, including when ctrl + cmd + left arrow moves focus to the section header (as it's the first cell). However, once the focus is on the first cell which is the section header, ctrl + cmd + right arrow won't move focus to the last cell, as it would for any other cell, which probably is not the expected behavior?

Hi, @adoroshk - The command is Ctrl+Fn+(Right/Left) Arrow

@mjpalazzo
Copy link
Contributor

Validated several examples with Edge and Chrome. Keyboard interactions are working as expected. I also tested several examples with JAWS. I was able to navigate with the screen reader and hear announcements as expected.
Great work!

@github-actions github-actions bot temporarily deployed to preview-pr-1927 December 7, 2023 08:10 Destroyed
Copy link
Contributor

@eawww eawww left a comment

Choose a reason for hiding this comment

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

High level check looks good. Approving to unblock Kenny but will dive deeper and log any issues found.

@eawww eawww added ⭐ UX Reviewed UX Reviewed and approved. and removed UX Review Required ⭐ UX Reviewed UX Reviewed and approved. labels Dec 7, 2023
@cm9361 cm9361 merged commit f16b226 into main Dec 7, 2023
21 checks passed
@cm9361 cm9361 deleted the flowsheet-keyboard-navigation branch December 7, 2023 15:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants