Support dynamic tabbableChildren for TableCellView #2340

msmithNI · 2024-08-09T01:17:28Z

Pull Request

🤨 Rationale

#2210

👩‍💻 Implementation

Summary of changes:

TableCellView tabbableChildren changed from a getter to be an @observable property
Both anchor & menu button columns only have a tabbable/focusable element in cells in some cases, so opt them into this change.
Create a new dynamicRef directive, used in the anchor/ menu button cell templates. The issue with the ref directive is that once a reference is initialized, it's never cleared. So, for example, if an anchor column starts out with valid URL data then gets data without URLs, cellView.anchor would still be set (to an element no longer in the DOM) with ref, whereas dynamicRef clears out the property
- As an alternative, I considered the children directive with a filter, but it seemed like that would require adding yet another extra container div to the cell views, which didn't seem desirable.
When tabbableChildren changes, the cell view fires a bubbling 'cell-tabbable-children-change' event, handled by KeyboardNavigationManager, which focuses the cell instead (if the cell content was previously focused, and is no longer present)

Implementation concerns:

'cell-tabbable-children-change' event
- This works, but feels somewhat heavy handed (but I couldn't really think of a better alternative).
- This fires for every visible row, not just the focused row (extra events firing that won't be handled)
- KeyboardNavigationManager keeps track of focused rows/ cells via index, whereas most of the rest of the table code uses recordId and columnId. In order to only handle the event for the focused row, there's some new code in KeyboardNavigationManager that keeps track of the columnId / recordId if cell content is focused, which complicates that class a bit more.

UX concerns:

In Chrome, if a focused element inside a cell is removed from the DOM, the table loses focus entirely. KeyboardNavigationManager tracks when the table gains/ loses focus, and won't programmatically set focus if the table doesn't already contain focus. So in this case (in Chrome) the user would need to manually refocus the table, at which point the cell would get focused. I discovered this fairly recently so haven't spent too much time pondering what it would take to have the keyboard nav code handle this specific case.
- This didn't occur in Firefox / WebKit, which is why the new test only conditionally refocuses the table right now.

🧪 Testing

Existing keyboard nav / tabbableChildren tests still pass
Add new test ensuring that if tabbableChildren are changed (removed), focus reverts to the cell
No tests for dynamicRef currently (we'd want some if we went with this approach)

✅ Checklist

N/A for now since this draft PR is to get feedback on this approach.

msmithNI · 2024-08-09T01:19:16Z

<moved to thread>

rajsite · 2024-08-12T15:39:52Z

change/@ni-nimble-components-8499a62b-64ea-41e0-877c-6bacca7ae0c8.json

@@ -0,0 +1,7 @@
+{


@msmithNI: @jattasNI @rajsite @atmgrifter00 interested in your thoughts on this implementation direction so far (but note the concerns I listed in the PR description).

The general direction looks reasonable to me.

For the cell-tabbable-children-change I'm a bit curious how often that event is firing. Is it firing once per row continuously as you scroll for example? That could potentially be excessive object creation / gc pressure on scroll.

In Chrome, if a focused element inside a cell is removed from the DOM, the table loses focus entirely.

Do you have a screencap of how that manifests? Not exactly clear how to try out the UX for that situation and if it's a new change in behavior or existing.

No objections to the overall direction from me. (I left a couple comments while wrapping my head around the PR because I couldn't stop myself; neither one is existential, just normal code review feedback)

When cell-tabbable-children-change fires:

With what's in this PR, there's some unnecessary firing as the table loads with initial data. If we omit firing when !cellView.$fastController.isConnected, we could prevent that though.

During scrolling:

Anchor column: Yes, depending on what's in the cells. With the Storybook example, yes, since the recycled cells change between having valid links / no link / placeholder text, as you scroll (depending on the data).

Menu button: Yes with this current PR. But not if we skip firing when the FAST controller is not connected.

Do you have a screencap of how that manifests?

Not currently, I was just going by the behavior of the new autotest failing in Chrome without the code to refocus the table. If we updated an example to periodically update the table data with an anchor column periodically switching between a valid URL / no URL, that would probably show it.

atmgrifter00 · 2024-08-14T13:55:10Z

packages/nimble-components/src/table/tests/table-keyboard-navigation.spec.ts

@@ -1422,6 +1426,28 @@ describe('Table keyboard navigation', () => {
                    expect(blurSpy).toHaveBeenCalledTimes(1);
                    expect(currentFocusedElement()).not.toBe(cellContent);
                });
+
+                it('and then the cell updates to no longer have tabbableChildren, the cell is focused instead', async () => {


I presume if we went with this approach (which I think I'm fine with), that there are some other tests we still need to add, yes? Namely, that after the tabbableChildren change (but not completely removed, as this test covers that) that tabbing (and shift-tabbing) behave as expected.

Agreed, those would be good tests to add too.

jattasNI · 2024-08-14T14:40:01Z

packages/nimble-components/src/utilities/directive/dynamic-ref.ts

@@ -0,0 +1,62 @@
+import {


Does FAST have unit tests for their RefBehavior that we could leverage for patterns to write unit tests for this class?

Not that I've found, but writing our own should be pretty quick anyway.

jattasNI · 2024-08-14T14:42:54Z

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

+        if (
+            this.focusType === TableFocusType.cellContent
+            && cellView.recordId === this.cellContentState.recordId
+            && cellView?.column?.columnId === this.cellContentState.columnId
+        ) {
+            if (
+                this.cellContentState.index >= cellView.tabbableChildren.length
+            ) {


I think you could collapse these two ifs into one.

jattasNI · 2024-08-14T14:46:39Z

change/@ni-nimble-components-8499a62b-64ea-41e0-877c-6bacca7ae0c8.json

@@ -0,0 +1,7 @@
+{


No objections to the overall direction from me. (I left a couple comments while wrapping my head around the PR because I couldn't stop myself; neither one is existential, just normal code review feedback)

rajsite · 2024-12-03T18:04:55Z

change/@ni-nimble-components-8499a62b-64ea-41e0-877c-6bacca7ae0c8.json

@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Update TableCellView tabbableChildren to be an observable property",


Closing the draft PR as the discussion is stale. Can reopen if continuing progress.

FYI @msmithNI @fredvisser I'm betting this refactor is needed as part of the editable table cells implementation where I suspect new tabbable elements will be created dynamically
#2477

msmithNI added 4 commits August 8, 2024 18:20

Initial changes for dynamic table cell view tabbableChildren

e0df708

Change files

e189c08

Merge branch 'main' into users/masmith/tableDynamicTabbableChildren-1

3e0cc93

Fix tests and lint issues

6681a48

msmithNI requested review from rajsite, jattasNI and atmgrifter00 August 9, 2024 01:18

rajsite reviewed Aug 12, 2024

View reviewed changes

atmgrifter00 reviewed Aug 14, 2024

View reviewed changes

jattasNI reviewed Aug 14, 2024

View reviewed changes

rajsite reviewed Dec 3, 2024

View reviewed changes

rajsite closed this Dec 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support dynamic tabbableChildren for TableCellView #2340

Support dynamic tabbableChildren for TableCellView #2340

msmithNI commented Aug 9, 2024

msmithNI commented Aug 9, 2024 •

edited by rajsite

Loading

rajsite Aug 12, 2024

rajsite Aug 12, 2024

jattasNI Aug 14, 2024

msmithNI Aug 14, 2024

msmithNI Aug 14, 2024

atmgrifter00 Aug 14, 2024

msmithNI Aug 14, 2024

jattasNI Aug 14, 2024

msmithNI Aug 15, 2024

jattasNI Aug 14, 2024

jattasNI Aug 14, 2024

rajsite Dec 3, 2024 •

edited

Loading

Support dynamic tabbableChildren for TableCellView #2340

Support dynamic tabbableChildren for TableCellView #2340

Conversation

msmithNI commented Aug 9, 2024

Pull Request

🤨 Rationale

👩‍💻 Implementation

🧪 Testing

✅ Checklist

msmithNI commented Aug 9, 2024 • edited by rajsite 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

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

rajsite Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

msmithNI commented Aug 9, 2024 •

edited by rajsite

Loading

rajsite Dec 3, 2024 •

edited

Loading