Support dynamic tabbableChildren for TableCellView #2340
Conversation
|
|
| @@ -0,0 +1,7 @@ | |||
| { | |||
There was a problem hiding this comment.
@msmithNI: @jattasNI @rajsite @atmgrifter00 interested in your thoughts on this implementation direction so far (but note the concerns I listed in the PR description).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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 () => { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Agreed, those would be good tests to add too.
| @@ -0,0 +1,62 @@ | |||
| import { | |||
There was a problem hiding this comment.
Does FAST have unit tests for their RefBehavior that we could leverage for patterns to write unit tests for this class?
There was a problem hiding this comment.
Not that I've found, but writing our own should be pretty quick anyway.
| if ( | ||
| this.focusType === TableFocusType.cellContent | ||
| && cellView.recordId === this.cellContentState.recordId | ||
| && cellView?.column?.columnId === this.cellContentState.columnId | ||
| ) { | ||
| if ( | ||
| this.cellContentState.index >= cellView.tabbableChildren.length | ||
| ) { |
There was a problem hiding this comment.
I think you could collapse these two ifs into one.
| @@ -0,0 +1,7 @@ | |||
| { | |||
There was a problem hiding this comment.
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)
Pull Request
🤨 Rationale
#2210
👩💻 Implementation
Summary of changes:
TableCellViewtabbableChildrenchanged from a getter to be an@observablepropertydynamicRefdirective, used in the anchor/ menu button cell templates. The issue with therefdirective 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.anchorwould still be set (to an element no longer in the DOM) withref, whereasdynamicRefclears out the propertychildrendirective 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.tabbableChildrenchanges, the cell view fires a bubbling'cell-tabbable-children-change'event, handled byKeyboardNavigationManager, which focuses the cell instead (if the cell content was previously focused, and is no longer present)Implementation concerns:
'cell-tabbable-children-change'eventKeyboardNavigationManagerkeeps track of focused rows/ cells via index, whereas most of the rest of the table code usesrecordIdandcolumnId. In order to only handle the event for the focused row, there's some new code inKeyboardNavigationManagerthat keeps track of thecolumnId/recordIdif cell content is focused, which complicates that class a bit more.UX concerns:
KeyboardNavigationManagertracks 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.🧪 Testing
dynamicRefcurrently (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.