Navigate the table with keyboard by severo · Pull Request #140 · hyparam/hightable

severo · 2025-04-28T16:57:09Z

Needed for #30

This PR adds the keyboard event handlers to navigate the table cells as expected. The actions in each cell will be implemented other PRs

The list of expected keyboard interactions are published in https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/grid_role#keyboard_interactions or https://www.w3.org/WAI/ARIA/apg/patterns/grid/#datagridsforpresentingtabularinformation

We must choose one of two options for handling the focus inside the table: "Roving tabindex" vs "aria-activedescendant". See https://www.w3.org/WAI/ARIA/apg/practices/keyboard-interface/#keyboardnavigationinsidecomponents.

We'll try "roving tabindex". The focus will be in one of the cells with tabindex="0", and the other ones will have tabindex="-1" so that they can be selected with .focus() but are no part of the Tab path. An interesting implementation here: https://github.com/stevejay/react-roving-tabindex/blob/master/src/use-roving-tabindex.ts

Some problems appeared:

when scrolling with the mouse, or after switching back to the scroll div with Tab and scrolling with arrows, the useEffect snippet scrolls back to the currently focused cell. We want to keep the reference of the highlighted cell and show it as such, but we don't want to scroll back to it when we're not in that mode - fixed with 376fe09. See the next point though
after fixing the previous point: when scrolling too much (or pressing PgDown for a long time), the focused cell is not part of the table anymore (it's a virtual table), and the focus is moved to the page <body>. In that case, you can't use the arrows keys anymore, while we would expect going back to the focused cell. Maybe we should separate the concepts of focused cell and selected/current cell. Fixed with 93369e6
another issue with scrolling, related to the previous point (switching the focus between the cell and the body): when scrolling back near to the cell, so that it is rendered again, the scroll jumps up to the cell, which is not expected (we only want to style the current cell, but not focus it while we're scrolling) - fixed with 93369e6:

Screencast.From.2025-05-02.11-46-34.mp4

Should we consider implementing aria-activedescendant instead of tabindex roving, and let the table focused? From https://sarahmhigley.com/writing/activedescendant/, it seems like it's not a good idea.

when navigating quickly with PageUp/PageDown (keeping them pressed for example), we can get to the point where the selected cell is invisible, because it's under the sticky header. Same when navigating upwards (see video). In that case, we would want to scroll down one or two rows to see the cell. Fixed with 1f11ab5

Screencast.From.2025-05-02.15-45-00.mp4

clicking the column resize separator focuses the header cell, but it's not preserved in the focus context, and moving with the arrows returns to the previously focused cell. Maybe set tabindex="0" on the resize separator as well when navigating to a column header cell, so that it's selectable with Tab, and then left and right arrow could be used to resize. It would fix the next point. Done with b384455
it's not possible to select and operate the column resize separator with the keyboard. Done with b05db42
the horizontal scroll is unstable when resizing a column using the keyboard - fixed with 2a2a1cd

Screencast.From.2025-05-02.18-51-15.mp4

navigating to the right/left with the keyboard should scroll to show the current cell as a whole (aligning with the left side if the cell is larger than the scroll area) - fixed with a027aa4

Screencast.From.2025-05-05.12-09-55.mp4

fix failing test: 7952c9b, 64b892d
pressing space scrolls down by half a page, and after pressing it multiple times, the current cell disappears and cannot be focused again. Same with Shift + space to scroll up. Fixed with 2793161
when focusing a cell, then pressing Shift+Tab to focus the table scroll, then scrolling until the current cell is deleted from the virtual table, then pressing Tab -> no focus. We should fetch the slice with the current cell, and focus it. Fixed with feb1ef9

Missing:

add tests

Copilot

Pull Request Overview

This PR implements keyboard navigation for table cells using a roving tabindex strategy as part of issue #30. Key changes include:

Introduction of the useFocus hook and FocusContext to manage keyboard events and focus state.
Updates to table components (TableHeader, TableCorner, RowHeader, ColumnHeader, and Cell) to integrate focus handling.
Integration of the FocusProvider in the HighTable component to wrap the table grid and handle keyboard events.

Reviewed Changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/hooks/useFocus.tsx	New hook to provide and manage focus state and handle keyboard events.
src/components/TableHeader/TableHeader.tsx	Updates to pass ariaRowIndex and compute ariaColIndex for header cells.
src/components/TableCorner/TableCorner.tsx	Updated to require ariaRowIndex and integrate cell focus on click.
src/components/RowHeader/RowHeader.tsx	Updates to pass ariaRowIndex to row headers and use cell focus.
src/components/HighTable/HighTable.tsx	Added FocusProvider and applied onKeyDown for keyboard navigation.
src/components/ColumnHeader/ColumnHeader.tsx	Integrated useCellFocus and updated aria attributes for header clarity.
src/components/Cell/Cell.tsx	Integrated useCellFocus and updated event handlers to trigger cell focus.
Tests (TableHeader and ColumnHeader)	Updated test cases to include ariaRowIndex in component props.

Files not reviewed (1)

src/components/HighTable/HighTable.module.css: Language not supported

Comments suppressed due to low confidence (1)

src/hooks/useFocus.tsx:115

[nitpick] The dependency 'cell' in the useEffect hook does not affect the effect’s behavior and appears redundant. Consider removing it to simplify the dependency array.

useEffect(() => { if (ref.current && isFocused && document.activeElement !== ref.current) { ref.current.focus() } }, [ref, isFocused, ariaColIndex, ariaRowIndex, cell])

Copilot

Pull Request Overview

This PR implements keyboard navigation for table cells using a roving tabindex approach while enhancing accessibility across the table components. Key changes include the addition of the useCellsNavigation hook to manage focus and keyboard events, updates to header and cell components to propagate aria indexes, and improved keyboard support for column resizing.

Reviewed Changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/hooks/useCellsNavigation.tsx	Introduces a context-based hook for managing cell navigation and keyboard events.
src/components/TableHeader/TableHeader.tsx	Updates header to include ariaRowIndex and passes correct aria properties to child components.
src/components/TableHeader/TableHeader.test.tsx	Updates tests to provide the necessary ariaRowIndex prop.
src/components/TableCorner/TableCorner.tsx	Integrates cell navigation for improved accessibility in the corner cell.
src/components/RowHeader/RowHeader.tsx	Applies cell navigation to row header, ensuring consistent focus handling.
src/components/HighTable/HighTable.tsx	Wraps table rendering in a CellsNavigationProvider and adds scrolling logic to focus cells.
src/components/ColumnResizer/ColumnResizer.tsx	Enhances column resizer to be keyboard accessible, with added focus management and key events.
src/components/ColumnHeader/ColumnHeader.tsx	Integrates useCellNavigation for accessible header cell focus and click handling.
src/components/ColumnHeader/ColumnHeader.test.tsx	Updates tests with default aria props for consistent behavior.
src/components/Cell/Cell.tsx	Augments cell component with accessible cell navigation and event handlers.

Files not reviewed (1)

src/components/HighTable/HighTable.module.css: Language not supported

Comments suppressed due to low confidence (1)

src/components/ColumnResizer/ColumnResizer.tsx:112

Consider adding an accessible label (e.g., aria-label='Resize column') to the separator element to provide additional context for screen reader users.

<span role="separator" aria-busy={ariaBusy} onDoubleClick={handleDoubleClick} onMouseDown={onMouseDown} onClick={disableOnClick} onFocus={onFocus} onBlur={onBlur} onKeyDown={onKeyDown} tabIndex={tabIndex} />

Copilot

Pull Request Overview

This PR implements keyboard navigation for table cells using a roving tabindex strategy while addressing scrolling and focus issues. Key changes include:

Integrating keyboard event handlers and navigation state via a new CellsNavigationProvider and related hooks.
Updating multiple table components (header, cells, row/column headers, corner, resizer) to use aria attributes and keyboard callbacks.
Adjusting tests and scroll behavior in HighTable for improved accessibility and user experience.

Reviewed Changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/hooks/useCellsNavigation.tsx	Introduces a context and hook for managing cell navigation state.
src/components/TableHeader/TableHeader.tsx	Adds ariaRowIndex support and updates ordering for header navigation.
src/components/TableCorner/TableCorner.tsx	Integrates cell navigation for table corner; now focusable via keyboard.
src/components/RowHeader/RowHeader.tsx	Updates to support cell navigation with aria indices.
src/components/HighTable/HighTable.tsx	Refactors focus and scroll logic; uses keyboard events from the context.
src/components/ColumnResizer/ColumnResizer.tsx	Enhances keyboard accessibility for resizing with new aria attributes.
src/components/ColumnHeader/ColumnHeader.tsx	Implements navigation on header click with updated aria labeling.
src/components/Cell/Cell.tsx	Updates cell behavior to incorporate keyboard navigation on events.

Files not reviewed (1)

src/components/HighTable/HighTable.module.css: Language not supported

bleakley · 2025-05-05T16:56:02Z

Overall this is really nice. I like the behavior and the implementation. The one behavior that I think is pretty awkward is this one:

When you use up/down to move outside of the viewport the table jumps half the page length. I think this would feel more natural if it worked the same way as Sheets and Excel where the selected cell stays at the top or bottom of the view when you do this.

severo · 2025-05-05T17:11:48Z

Thanks for the comments. I'll try to fix that

severo · 2025-05-05T17:58:55Z

Done, it was not too hard :)

5dc803a

Copilot

Pull Request Overview

This PR introduces keyboard navigation for table cells and headers using a roving tabindex approach, along with fixes for scrolling behavior and keyboard controls on column resizers. Key changes include:

Implementation of a CellsNavigationContext to manage keyboard navigation state.
Integration of keyboard event handlers across table headers, cells, and resizable columns.
Updates to tests and accessibility attributes to support the new navigation features.

Reviewed Changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/hooks/useCellsNavigation.tsx	Added context and keyboard event handlers for roving tabindex navigation.
src/components/TableHeader/TableHeader.tsx	Updated to pass ariaRowIndex and compute correct aria attributes.
src/components/TableCorner/TableCorner.tsx	Integrated cell navigation on click using useCellsNavigation.
src/components/RowHeader/RowHeader.tsx	Updated to include cell navigation with proper aria properties.
src/components/HighTable/HighTable.tsx	Wrapped table content with the new navigation provider and adjusted scrolling.
src/components/ColumnResizer/ColumnResizer.tsx	Added keyboard accessibility support for resizing columns.
src/components/ColumnHeader/ColumnHeader.tsx	Updated to use the cell navigation hook and pass appropriate aria attributes.
src/components/Cell/Cell.tsx	Updated to include keyboard navigation within table cells.
Test files	Added ariaRowIndex to tests and updated expectations for column widths.

Files not reviewed (1)

src/components/HighTable/HighTable.module.css: Language not supported

Comments suppressed due to low confidence (1)

src/hooks/useCellsNavigation.tsx:80

Consider adding explicit parentheses in the condition (e.g. (key === 'PageDown' || (key === ' ' && !event.shiftKey))) to clarify operator precedence.

else if (key === 'PageDown' || key === ' ' && !event.shiftKey ) {

severo · 2025-05-06T12:36:15Z

I think the PR is finally ready for review @bleakley @platypii.

Note that it adds keyboard navigation (and let use the column resizer), but does not implement other keyboard actions like selecting rows, sorting along columns, opening cells, or copying the contents of a cell to clipboard. See #30

platypii

Looking good! Played with it locally and everything seemed to work.

platypii · 2025-05-07T21:18:44Z

+      return
+    }
+    if (width === undefined) {
+      // don't allow other keyboard events when width is not set


good helpful comments here 👍

platypii · 2025-05-07T21:19:35Z

  thead [role="separator"] {
    position: absolute;
-    top: 0;
+    top: 1px;


why did this need to change?

it was to ensure the separator is visually inside the cell header

platypii · 2025-05-07T21:20:56Z

-  }
-  /* TODO(SL): add a global pending state? */
-  /* .pending thead th::before {
-    animation: shimmer 2s infinite linear;


looks like shimmer animation is no longer referenced by anything

yes, for a long time, it's not related to this PR. But I can look at it to add it again if needed

severo · 2025-05-08T05:54:00Z

Thanks for the review. Do you prefer me to merge this PR to master now, and let #147 handle the conflicts? The other option is to hold this PR until #147 is ready, then I take care of the conflicts here.

platypii · 2025-05-08T06:03:46Z

@rembrandtreyes eta on having #147 ready for merge? I feel like my comments there were mostly minor and the menu looked slick. Let me know if you want me to re-review. Not a rush I super appreciate the contribution! Trying to make life easier by not making you deal with the merge conflicts :-)

@severo I'm fine with holding on keyboard navigation for a bit until that one merges. If PRs start stacking up maybe we start merging then.

rembrandtreyes · 2025-05-08T17:03:05Z

@platypii just wrapped up the last few PR comments I re-requested a review if you want to look it over again. 👍

severo changed the title ~~create useFocus~~ Navigate the table with keyboard Apr 28, 2025

severo marked this pull request as draft April 28, 2025 16:57

severo force-pushed the navigate-through-keyboard branch from 4932003 to 5d51e79 Compare April 29, 2025 08:19

severo marked this pull request as ready for review April 29, 2025 10:10

severo requested review from bleakley, Copilot and platypii April 29, 2025 10:10

Copilot AI reviewed Apr 29, 2025

View reviewed changes

severo removed request for bleakley and platypii April 29, 2025 12:42

severo marked this pull request as draft April 29, 2025 12:42

severo force-pushed the navigate-through-keyboard branch from 1c297f3 to 4459207 Compare May 2, 2025 09:29

severo marked this pull request as ready for review May 2, 2025 16:50

severo requested review from bleakley, Copilot and platypii May 2, 2025 16:50

Copilot AI reviewed May 2, 2025

View reviewed changes

Comment thread src/components/HighTable/HighTable.tsx Outdated

severo requested review from Copilot and removed request for bleakley and platypii May 2, 2025 16:52

Copilot AI reviewed May 5, 2025

View reviewed changes

severo added 6 commits May 5, 2025 22:40

create useFocus

daf275a

set tabIndex on every cell to 0 or -1

ae12f67

navigate with keyboard with focus

45f1e8c

fix test

d6a623f

use :focus, otherwise the user agent might hide the focus

1097a26

handle click on a cell - missing: click on a column header separator

46c8edf

severo added 4 commits May 5, 2025 22:41

fix test, because vitest cannot apply scrollIntoView

a7fe67a

set the column header label as the column name

23f1e89

handle space and shift-space scrolling

924cdb9

scroll one row at a time

3f294be

severo force-pushed the navigate-through-keyboard branch from 5dc803a to 3f294be Compare May 5, 2025 20:41

severo added 9 commits May 6, 2025 10:09

Merge branch 'master' into navigate-through-keyboard

f15811f

Use Tab, Enter or Space to enter cell navigation mode

feb1ef9

avoid a loop

e8c8e72

add tests about initial focus and keydown on scrollable div

114b404

add tests

e63c2a8

only handle keydown events from the scroller in onScrollKeyDown

7471e9a

add a test about the column resizer

2f645c0

add a test on resizer activation

97b3696

exit resizer after autoresize done with double click + add tests

fcf25b5

severo requested review from bleakley, Copilot and platypii May 6, 2025 12:34

Copilot AI reviewed May 6, 2025

View reviewed changes

Comment thread src/hooks/useCellsNavigation.tsx

Comment thread src/components/ColumnResizer/ColumnResizer.tsx

This was referenced May 6, 2025

Replace aria-label and aria-description with aria-labelledby and aria-describedby #151

Open

Enable consistent navigation with the keyboard #30

Closed

sort columns with the keyboard #152

Merged

Add a prop to handle key down on a cell #153

Merged

platypii approved these changes May 7, 2025

View reviewed changes

severo merged commit c8f94c5 into master May 10, 2025
10 checks passed

severo deleted the navigate-through-keyboard branch May 10, 2025 07:48

severo mentioned this pull request May 10, 2025

Restore shimmer animation #154

Open

Conversation

severo commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

bleakley commented May 5, 2025

Uh oh!

severo commented May 5, 2025

Uh oh!

severo commented May 5, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

severo commented May 6, 2025

Uh oh!

platypii left a comment

Choose a reason for hiding this comment

Uh oh!

platypii May 7, 2025

Choose a reason for hiding this comment

Uh oh!

platypii May 7, 2025

Choose a reason for hiding this comment

Uh oh!

severo May 10, 2025

Choose a reason for hiding this comment

Uh oh!

platypii May 7, 2025

Choose a reason for hiding this comment

Uh oh!

severo May 10, 2025

Choose a reason for hiding this comment

Uh oh!

severo May 10, 2025

Choose a reason for hiding this comment

Uh oh!

severo commented May 8, 2025

Uh oh!

platypii commented May 8, 2025

Uh oh!

rembrandtreyes commented May 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

severo commented Apr 28, 2025 •

edited

Loading