Table keyboard interaction (draft / partial implementation) #2070

msmithNI · 2024-05-03T04:13:27Z

Pull Request

🤨 Rationale

👩‍💻 Implementation

The KeyboardNavigationManager class does the bulk of the work:

Tracks what is currently focused / should be focused (focusState property which contains focusType - there are multiple TableFocusType values like columnHeader/row/cell/cellActionMenu/etc)
- Note: If the user clicks in the table away from the current keyboard focus, focusState would end up out-of-date. There's code in handleFocus() to handle for this (see additional comments there)
Listens for table keypresses to handle arrow key navigation, Tab/Shift-Tab presses
If the user unfocuses and refocuses the table, re-focuses the appropriate elements in the table (handleFocus() again)
Tab/Shift-Tab behavior should match the HLD / expectations for ARIA treegrid. (Basically, Tab/Shift-Tab go through the tabbable elements of the header row / current row, not including cells/column headers, until the end is reached, then the table blurs and elements before/after it will be focused with additional Tab presses.)
Not implemented in this version:
- Home/End/PageUp/PageDown keys
- Ctrl-Enter / F2 to focus elements in a cell (action menu can still be reached via Tab/Shift-Tab)
- Row selection via Space
- Behavior when a row is focused, then user scrolls with the scrollbar, then keyboard focus returns to the table, is unexpected / probably not right

The overall approach of only setting tabindex=0 on the single element in the table we want focused is called roving tabindex.

packages/nimble-components/src/anchor/index.ts

packages/nimble-components/src/checkbox/template.ts

msmithNI · 2024-05-03T04:50:19Z

packages/nimble-components/src/table-column/base/cell-view/index.ts

+     * Gets the child elements in this cell view that should be able to be reached via Tab/ Shift-Tab,
+     * if any.
+     */
+    public get tabbableChildren(): HTMLElement[] {


If a table column type will contain focusable/tabbable elements, our guidance would be:

That column's cell view overrides this property, and returns the focusable element(s) in the array

The focusable elements should be tabindex=-1 by default

KeyboardNavigationManager uses this when handling Tab/Shift-Tab for a given row, and to reflect the correct focus state if one of these elements is focused (TableFocusType.cellContent).

Currently KeyboardNavigationManager updates tabindex of these elements directly (except for the tabIndexOverride cases described previously). For arbitrary cell content, this might not be sufficient, since the element that should be tabindex=0 could be an element descendant in a shadow root or something similar. So we may need to add an additional API/method to handle focusing/unfocusing these children.

msmithNI · 2024-05-03T04:54:53Z

packages/nimble-components/src/table/components/row/styles.ts

+    }
+
+    /* TODO: Scope this to only hasDataHierarchy; ~4px more padding on left, so border doesn't touch expand/collapse button border */
+    nimble-table-cell:first-of-type${focusVisible} {


(Note: These 2 rules are handing the special case of making the cell border for the 1st column / hierarchy column wide enough to include the expand/collapse button. It probably needs another cleanup pass - I couldn't come up with something that worked with just margin/padding on the cell, but I got something working adding a ::before pseudoselector too).

msmithNI · 2024-05-03T04:59:02Z

packages/nimble-components/src/table/template.ts

@@ -33,6 +33,7 @@ import {
 export const template = html<Table>`
    <template
        role="treegrid"
+        tabindex="0"


As everything in the table will now start as tabindex=-1, this tabindex=0 ensures the table can be keyboard focused initially. The table remains as tabindex=0 for almost all (see KeyboardNavigationManager.blurAfterLastTab() for an exception)

msmithNI · 2024-05-03T20:24:54Z

packages/nimble-components/src/table/models/keyboard-navigation-manager.ts

+        table.addEventListener('keydown', e => this.onKeyDown(e));
+        table.addEventListener('focusin', e => this.handleFocus(e));
+        table.addEventListener('focusout', e => {
+            console.log(


(These console.logs are just temporary/for debugging, for anyone that wants to get a better sense of what's getting focused/unfocused while interacting with the table in Storybook. It's not always obvious what's getting focused.)

msmithNI · 2024-05-03T20:29:19Z

packages/nimble-components/src/table/models/keyboard-navigation-manager.ts

+        }
+    }
+
+    public onVirtualizerChange(): void {


This is how we handle re-focusing the appropriate row after scrolling / virtualizer changes. Observing the virtualizer.visibleItems is another option, but we'd also need to observe each individual row and monitor for dataIndex changes.
Possibly keeping a reference to the currently focused row (so we could just watch for dataIndex changes) is another option.

Noticed while demoing to Milan / Meyer today that this simplified approach has issues once you start scrolling down with just DownArrow (you end up with no focused row visible).
Adding a rAF works but is hacky, so I'll probably switch back to the original approach mentioned in the comment above.

msmithNI · 2024-05-03T20:55:00Z

packages/nimble-components/src/table/models/keyboard-navigation-manager.ts

+    }
+
+    private readonly handleFocus = (event: FocusEvent): void => {
+        // User may have clicked elsewhere in the table (on an element not reflected in this.focusState). Update our focusState


As indicated by the code comments, I'm not super satisfied with all of this logic being in the focusin handler, I think it makes it harder to reason about the various focus/unfocusing states of the table.

If we only had to worry about keyboard navigation by itself (without mouse interactions), the responsibilities of this class would be pretty simple - basically just a state machine: current focus state + a key press => new focus state.
But in practice, the user can click anywhere on the table (like interacting with an action menu), and then we still want keyboard navigation to behave in a sane way.

Here's 2 action menu scenarios that behave differently, that led to the current logic in this function:

Table is not focused at all, then the user clicks a cell action menu button. Result: first the MenuButton is the active element, then a MenuItem is active (once the menu is open)

Table is focused (via Tab), the 1st row is focused (Down Arrow), then the user clicks the action menu button on some other row (not the 1st). Result: the MenuItem is active. I didn't see any focus events get triggered with the actual MenuButton in this scenario, which is why I have code going from a MenuItem back to the linked action menu.

This function also handles setting the appropriate element in the table on initial focus, so there's a fair amount going on in this function.

I think there might be better ways to handle all of this, so I'm looking for suggestions on different approaches here too.

A simple improvement might be to do a "clean code" pass and see if you can move each of the if blocks into their own well-named functions.

Another idea is to make focusState into a class rather than an object with individual fields being mutated. That might help make the state transitions more explicit. This might look something like:

make all the fields of the class readonly/getters from the outside

add public methods to set state in batches. e.g.

focusCellActionMenu(rowIndex, columnIndex) { this.focusType = TableFocusType.cellActionMenu; this.rowIndex = rowIndex; this.columnIndex = columnIndex; }

see if those public methods can enable this class to be even more explicit about state transitions. Maybe each one can be called from a navigation manager method that also updates the navigation manager's state.

I only looked at a couple of state transitions so I'm not sure how much this would help, might require prototyping one or two states in a branch to see if we like it better.

Ah, I see Meyer made similar suggestions in https://github.com/ni/nimble/pull/2070/files#r1591068527

msmithNI · 2024-05-03T21:00:18Z

packages/nimble-components/src/table/models/keyboard-navigation-manager.ts

+        // Note: In Chrome this is only needed for Shift-Tab, but in Firefox both Tab+Shift-Tab need this
+        // to work as expected.
+        this.table.tabIndex = -1;
+        window.requestAnimationFrame(() => {


requestAnimationFrame calls always need vetting/questioning, so opening a comment thread here. See code comments for why this is here (if the table was still tabindex=0 when the user did Shift-Tab, the overall table would get focused which isn't what we want).

I'm also not aware of a good way to determine what the next or previous focused element will be, if we wanted to focus it programmatically (the browser computes it based on semi-complicated tab order rules).
So this code is just trying to ensure the browser can handle it: we blur the focused table sub-element, and then we don't preventDefault() on the Tab/Shift-Tab key event, so the browser will do the default behavior still (of focusing the next/previous element in tab order).

msmithNI · 2024-05-03T21:04:33Z

@rajsite @jattasNI @atmgrifter00 I think I've now commented on the main areas of this code that I'd like feedback on, so feel free to start looking it over now. If there's any demos/walkthroughs of it that you'd like to see to understand something better (or any discussions that you think would be better as Teams calls) we can do that next week.

Edit: Any feedback on overall implementation approach, ways to simplify the code, etc, are welcome too at this point. For example all the key press handlers (like onRightArrowPressed()) are fairly verbose right now, but hopefully easy enough to follow.

atmgrifter00 · 2024-05-06T13:50:10Z

packages/nimble-components/src/table/models/keyboard-navigation-manager.ts

+            }
+        }
+
+        if (event.target instanceof MenuItem) {


I'm guessing it's not possible to enter into this if block if the above if block was entered? If not, can we make it more clear by making this an if/else if block? If it can make sense to enter into both if blocks, are there ways for us to avoid duplicate efforts (possibly an over-optimization as iterating over the columns isn't a big deal).

packages/nimble-components/src/table/models/keyboard-navigation-manager.ts

atmgrifter00 · 2024-05-06T14:07:37Z

packages/nimble-components/src/table/models/keyboard-navigation-manager.ts

+                }
+                while (
+                    cellIndex >= 0
+                    && cellIndex < this.table.visibleColumns.length


Maybe we can make a couple of private helper methods to help with code readability (reduce function length). Something like setNextTabFocusState(cellIndex: number) and setPreviousTabFocusState(...)? Possibly even getNextTabCellIndex and getPreviousTabCellIndex that just encapsulates the switch logic.

Agreed. I'll want to do another pass on code sharing/ helper methods if people are otherwise satisfied with the general approach on tracking focus state that I have here. I wanted to see if there were any large course corrections needed first though.

I'm ok with the overall architecture of managing the state from this class. For me the state machine readability is the biggest course correction. I left some brainstorm-y ideas on that in this thread: https://github.com/ni/nimble/pull/2070/files#r1595622740

packages/nimble-components/src/table/models/keyboard-navigation-manager.ts

packages/nimble-components/src/table/components/cell/styles.ts

atmgrifter00 · 2024-05-06T22:31:09Z

packages/nimble-components/src/table/models/keyboard-navigation-manager.ts

@@ -0,0 +1,1025 @@
+/* eslint-disable no-console */


Here are the set of behaviors that I noticed that I believe are not ideal (unexpected):

Table with hierarchy data (single selection mode):

If the first action menu has focus, I think pressing Shift-Tab should focus the row.

If an action menu has focus (or any focusable element), pressing the Left or Right arrow keys should shift focus to the previous or next cells respectively if there are no other focusable elements in the cell.

If the table has ever moved focus to something like an action menu, then after you click out of the table, and then tab to it, it will skip the header content and refocus the action menu. This means that it is no longer easy to just tab past the table at the top-level.

Table Anchor Column

The anchor column cells do not seem like they can show the highlighted cell border. I admit that this may be intentional, and not sure if there is a better answer.

Possibly more to come...

Anchor Column: Intentional. This is the case of a cell containing a single focusable control, in which case we only focus the control, not the cell (according to the HLD).

I think our current anchor focus styling makes it a bit difficult to tell when the anchor is focused (at least compared to the focused cell styling), so we might end up revisiting that behavior. (I can demo it to the team and get opinions)

I'm not sure if I agree on the 1st 3 points (behavior after an action menu is focused). To summarize the current behavior:
Once an action menu is focused, you're no longer in row/cell navigation mode. You can return to that mode by pressing Esc, though. I'd characterize being able to press LeftArrow / RightArrow to go from a focused action menu to cell focus again as a behavior change from what's in the HLD, but if others agree I can look at adding that.

If the table has ever moved focus to something like an action menu, then after you click out of the table, and then tab to it, it will skip the header content and refocus the action menu. This means that it is no longer easy to just tab past the table at the top-level.

I'm not sure how I'd reconcile that suggestion with our desire to return to the previously focused row once you tab past the table, and then tab back to it. Currently I'm characterizing interacting with the action menu the same way as interacting with a row/cell (in that we want to return focus to that same place when tabbing past and returning). I'm curious if others have an opinion on this point.

our desire to return to the previously focused row once you tab past the table, and then tab back to it

I think that workflow is important. It'd be annoying to navigate through a bunch of rows and columns to get to the cell you want, then accidentally press tab or click one extra time causing the table to lose focus and all your navigation to be lost.

I'm ok with the current behavior, but would be open to ideas for returning the table to its initial state where it only has one tab stop. Right now the way I know to do it is to reverse all the navigation that got you to your current state (ESC to unfocus action menu, arrows to return focus to row / header). As an example idea, maybe if you keep pressing ESC the focus should move up the hierarchy (action menu -> cell -> row -> header)?

But I'm wary of designing this on the fly and increasing scope too much before we get this in the hands of actual users.

Yeah, I'm okay with the tab behavior. More than anything I wanted to check that we were fine with the Table entering into a state where it is no longer to just tab past it at the top-level. The AzDO table avoids this by not letting you tab through focusable elements in a row, and instead relies on the arrow keys (as it does not let you put cell focus in columns with no focusable elements). Screen readers like NVDA allow a user to navigate to non-focusable columns with Ctl-Arrow keys, which does not work without an active screen reader.

This example, (which I recognize is just some jo-shmo's idea of a treegrid) does what is currently implemented in ours.

For the first point on Shift-Tab putting focus on the row when the first action menu is focused, I think I just needed to become accustomed to pressing Esc to re-enter navigation mode.

jattasNI

Early feedback from some storybook testing. Haven't looked at the code much yet.

packages/nimble-components/src/anchor/index.ts

jattasNI · 2024-05-07T22:23:40Z

packages/nimble-components/src/anchor/index.ts

@@ -1,4 +1,4 @@
-import { attr } from '@microsoft/fast-element';
+import { attr, observable } from '@microsoft/fast-element';


I'm not able to get row selection to work. From the HLD:

If a row is focused, pressing Space will select/unselect that row

When I focus an entire row, space either does nothing or scrolls the page. I tried this with both single select and multi select mode on the hierarchical table (again in macOS Firefox).

In multiselect mode I am able to focus the selection checkbox and press Space to toggle it.

Just not yet implemented (updated the PR description), but should be straightforward.

jattasNI · 2024-05-07T22:28:26Z

packages/storybook/src/nimble/table/table.stories.ts

@@ -28,11 +28,23 @@ const metadata: Meta<BaseTableArgs> = {
    parameters: {
        actions: {
            handles: [
+                /*


Shot in the dark, but I recall an issue with the Actions add-on where it can get broken / slow trying to print large / complex event data. I think I recall fixing it by limiting the depth of the event data serialization. I can help search for that if you think that might be the issue and can't discover how to configure it on your own.

I'm guessing this is referencing the configureActions({depth: 1}) here in the Storybook project, but changing it to depth=0 doesn't get rid of the issue.

Yep, that's what I was recalling. Too bad it doesn't help 😔

jattasNI · 2024-05-09T12:59:58Z

packages/nimble-components/src/table/components/cell/index.ts

+        // via keyboard, to ensure they're visible to focus. This code ensures we remove that CSS class when the menu
+        // button is no longer focused (which may not be for a keyboard nav reason, i.e. a mouse click elsewhere on
+        // the table)
+        this.actionMenuButton!.classList.remove('focused');


This is an implicit coupling between the table cell and the keyboard navigation manager which seems likely to get out of date if we change the keyboard manager implementation (e.g. rename this class). Is there anything we can do to make it more explicit and keep the knowledge of the focused class within one file?

For example, could the cell fire an event when the action menu is blurred and the keyboard navigation manager listen for that event?

jattasNI · 2024-05-09T13:56:47Z

packages/nimble-components/src/table/models/keyboard-navigation-manager.ts

@@ -0,0 +1,1025 @@
+/* eslint-disable no-console */


our desire to return to the previously focused row once you tab past the table, and then tab back to it

I think that workflow is important. It'd be annoying to navigate through a bunch of rows and columns to get to the cell you want, then accidentally press tab or click one extra time causing the table to lose focus and all your navigation to be lost.

I'm ok with the current behavior, but would be open to ideas for returning the table to its initial state where it only has one tab stop. Right now the way I know to do it is to reverse all the navigation that got you to your current state (ESC to unfocus action menu, arrows to return focus to row / header). As an example idea, maybe if you keep pressing ESC the focus should move up the hierarchy (action menu -> cell -> row -> header)?

But I'm wary of designing this on the fly and increasing scope too much before we get this in the hands of actual users.

…nArrow

…wport tabstop (Firefox)

…c as Tab for rows/cells)

- If an anchor is focused in a cell, and we're in the 'single interactive element' special case, and the user mousewheel scrolls the table, the row will be focused instead. (So we don't try to refocus the anchor while the focusedRecycleCallback blurs it) - If cell contents were focused (tabbable child), and a scroll occurs, the cell will be focused instead. Also ensure action menu focus state is set once an action menu is opened. - If an action menu button was focused and a scroll occurs, the cell will be focused instead.

msmithNI added 8 commits April 3, 2024 23:01

Prototype as of 4/3

e823d57

Merge from main

b974ba5

Updated keyboard nav code

4dad3d3

Lint/prettier

07cc96b

Add TODO

f57ac31

Merge from main

6f2126b

Merge branch 'main' into users/masmith/table-keyboard-interaction-temp

893db0c

Fix test, less console.log noise when body focused

e185f0d

msmithNI mentioned this pull request May 3, 2024

Table keyboard interaction #1137

Open

9 tasks

msmithNI commented May 3, 2024

View reviewed changes

packages/nimble-components/src/anchor/index.ts Outdated Show resolved Hide resolved

msmithNI commented May 3, 2024

View reviewed changes

packages/nimble-components/src/checkbox/template.ts Outdated Show resolved Hide resolved

msmithNI commented May 3, 2024

View reviewed changes

msmithNI requested review from rajsite, jattasNI and atmgrifter00 May 3, 2024 21:01

atmgrifter00 reviewed May 6, 2024

View reviewed changes

packages/nimble-components/src/table/models/keyboard-navigation-manager.ts Outdated Show resolved Hide resolved

atmgrifter00 reviewed May 6, 2024

View reviewed changes

packages/nimble-components/src/table/models/keyboard-navigation-manager.ts Outdated Show resolved Hide resolved

atmgrifter00 reviewed May 6, 2024

View reviewed changes

packages/nimble-components/src/table/components/cell/styles.ts Show resolved Hide resolved

Remove no-op call

65ab3dd

atmgrifter00 reviewed May 6, 2024

View reviewed changes

jattasNI reviewed May 7, 2024

View reviewed changes

jattasNI reviewed May 9, 2024

View reviewed changes

msmithNI added 25 commits May 10, 2024 14:00

Bugfix for Safari Mac issue, temporary fix for scrolling down via Dow…

078fccb

…nArrow

Remove tabIndexOverride

afaa3cb

Undo no-op change

247b51a

Feature complete keyboard nav

0ba987e

Cleanup / fix F2 for single-element cells

0a92870

prettier

7a7ce21

Fix action menu Shift-Tab issue

e29b789

Prettier

5110178

Simplify onTabPressed

4248deb

Fix anchor view tabbableChildren implementation, get rid of extra vie…

531eac3

…wport tabstop (Firefox)

Cleanup (methods to set focus type, Tab for headers uses similar logi…

249c1bf

…c as Tab for rows/cells)

Merge branch 'main' into users/masmith/table-keyboard-interaction-temp

c6244a1

Fix build

8f3c5ce

Uptake tabindex changes

cacf215

Fix column header tabIndex behavior

adfb7a7

Fix issues scrolling with a focused cell (+ switch back to observers)

b23ea0d

Fix build

0a26787

Refactoring/cleanup

0d5ff72

Cleanup

6dbad6d

Remove (flatten) TableFocusState into kbd nav code

44f0936

Add anchor to lastName in Storybook (for testing)

e472265

Fix another 'single interactive element' corner case with Tab

cccd1d2

Remove 'single interactive element' special case

9a32fb3

Update Enter (when cell focused) to focus content, but not activate it

c80b0f0

msmithNI closed this May 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Table keyboard interaction (draft / partial implementation) #2070

Table keyboard interaction (draft / partial implementation) #2070

msmithNI commented May 3, 2024 •

edited

msmithNI May 3, 2024

msmithNI May 3, 2024

msmithNI May 3, 2024

msmithNI May 3, 2024

msmithNI May 3, 2024

msmithNI May 8, 2024

msmithNI May 3, 2024 •

edited

jattasNI May 9, 2024

jattasNI May 9, 2024

msmithNI May 3, 2024

msmithNI commented May 3, 2024 •

edited

atmgrifter00 May 6, 2024 •

edited

atmgrifter00 May 6, 2024 •

edited

msmithNI May 6, 2024

jattasNI May 9, 2024

atmgrifter00 May 6, 2024

msmithNI May 8, 2024

msmithNI May 8, 2024

jattasNI May 9, 2024

atmgrifter00 May 9, 2024

jattasNI left a comment

jattasNI May 7, 2024

msmithNI May 8, 2024

jattasNI May 7, 2024

msmithNI May 8, 2024

jattasNI May 8, 2024

jattasNI May 9, 2024

jattasNI May 9, 2024

		@@ -1,4 +1,4 @@
		import { attr } from '@microsoft/fast-element';
		import { attr, observable } from '@microsoft/fast-element';

Table keyboard interaction (draft / partial implementation) #2070

Table keyboard interaction (draft / partial implementation) #2070

Conversation

msmithNI commented May 3, 2024 • edited

Pull Request

🤨 Rationale

👩‍💻 Implementation

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

Choose a reason for hiding this comment

msmithNI May 3, 2024 • edited

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

msmithNI commented May 3, 2024 • edited

atmgrifter00 May 6, 2024 • edited

Choose a reason for hiding this comment

atmgrifter00 May 6, 2024 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jattasNI left a 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

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

msmithNI commented May 3, 2024 •

edited

msmithNI May 3, 2024 •

edited

msmithNI commented May 3, 2024 •

edited

atmgrifter00 May 6, 2024 •

edited

atmgrifter00 May 6, 2024 •

edited