Disabling TableView/ListView keyboard navigation and press interactions if collection is empty by LFDanLu · Pull Request #3422 · adobe/react-spectrum

LFDanLu · 2022-08-19T19:57:17Z

Known issues:

clicking on a resizable column header, closing the menu, and hitting ArrowLeft will move focus onto the adjacent resizer instead of onto the adjacent column header
- looks to be fixed in Col resize hit area increase #3414, tested locally and the skipped test works after merging that code in
hitting Enter/Space while focused on the TableView's select all checkbox doesn't do anything
- pre-existing, will investigate and fix in separate PR (fixed by Fix usePress with HTML input elements #3446)

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Test the "renderEmptyState" TableView story and the "empty" ListView stories. You should be able to tab to the link in the illustrated message and the select all checkbox should be disabled for the TableView. Empty CardViews shouldn't work at the moment

🧢 Your Project:

RSP

LFDanLu · 2022-08-19T20:00:34Z

      'aria-multiselectable': state.selectionManager.selectionMode === 'multiple' ? 'true' : undefined
    },
-    state.isKeyboardNavigationDisabled ? {} : collectionProps,
+    state.collection.size === 0 || state.isKeyboardNavigationDisabled ? {} : collectionProps,


Opted for putting this logic here so this behavior would apply to all grid components, not just TableView.

Note that this would prevent the user from tabbing to the column headers to resize them if the TableView is empty, but I figured this was a non issue since the Table doesn't have row data to display. Perhaps we should remove the resize menu entirely for this case?

EDIT: the above isn't 100% true (the column resize buttons still have tabindex = 0) but it would be a simple enough task to hide that as well

this makes resizing, clicking on column header and other interactions all inert if the table is empty. Partially makes these changes TableView specific instead of grid wide, to confirm with team.

…table_empty

also fixes some small things the tests revealed like how the hidden inputs in the resizer werent disabled

adobe-bot · 2022-08-23T21:42:48Z

Build successful! 🎉

test is skipped for now due to outstanding bug that is fixed in separate PR

LFDanLu · 2022-08-23T22:37:51Z

Known issues:

clicking on a resizable column header, closing the menu, and hitting ArrowLeft will move focus onto the adjacent resizer instead of onto the adjacent column header
- looks to be fixed in Col resize hit area increase #3414, tested locally and the skipped test works after merging that code in
hitting Enter/Space while focused on the TableView's select all checkbox doesn't do anything
- pre-existing, will investigate and fix in separate PR

adobe-bot · 2022-08-23T22:41:53Z

Build successful! 🎉

LFDanLu · 2022-08-23T22:42:18Z

+    // TODO: this will affect all grids, maybe make it specific to Table via useTableState?
+    // something like isInteractionDisabled: state.collection.size === 0
+    isDisabled: state.collection.size === 0


Open to opinions here. I could make the changes in this PR to be grid wide instead of only for TableView which would make the above change valid. If we wanna only affect TableView, I can add something specific to the table state instead OR add isDisabled to useGridCell/useGridRow's props (feels iffy). A third option is that we are ok with the above change even though we are scoping this behavior to TableView, especially since TableView is the only grid that has grid cells and rows when the collection is empty

LFDanLu · 2022-08-23T22:42:48Z

+      expect(tree.queryByRole('slider')).toBeFalsy();
+    });
+
+    it.skip('should re-enable functionality when the table recieves items', function () {


As mentioned, skipped for now until #3414 goes in

adobe-bot · 2022-08-25T19:21:05Z

Build successful! 🎉

adobe-bot · 2022-08-29T16:54:48Z

Build successful! 🎉

reidbarber

Just one comment, everything else looks good

reidbarber · 2022-08-30T21:52:58Z

+
+  return (
+    <Flex direction="column">
+      <ActionButton width="100px" onPress={() => setShow(show => !show)}>Toggle items</ActionButton>


If you toggle items in, select all, then toggle items out, the select all checkbox is shown as checked and is keyboard navigable even though it is disabled. Not sure if this is just how the story is written and a non-issue, or a separate issue.

It remains keyboard navigable even if you undo the select all again. As soon as it has gotten focus, that's what TableView will send focus to. This happens with the Column headers as well. Toggle the items, navigate to col 3, then toggle the items off. Tab will continue to send you to Column Header 3

#3422 (comment)

Thanks for catching this, I'll probably go the route of clearing the tab index if the grid is empty. One possible issue may be if the collection doesn't properly get an updated tab index but I imagine I could override that as well if need be

adobe-bot · 2022-09-01T21:39:32Z

Build successful! 🎉

snowystinger · 2022-09-01T22:31:18Z

+    onAction: onCellAction ? () => onCellAction(node.key) : onAction,
+    // TODO: this will affect all grids, maybe make it specific to Table via useTableState?
+    // something like isInteractionDisabled: state.collection.size === 0
+    isDisabled: state.collection.size === 0


is it both non-interactive and non-navigable? or should we name the prop something else? or is that what you're suggesting in the comment?
Lets only do this for Table for now? I assume we'll be asked to do this for ListView eventually, but sounds like not yet?

The PR also covers ListView.

Also, if the collection is empty, wouldn't there be no grid cells? I guess only in the case of tableview because of the headers?

Yep, only TableView has grid cells when the collection is empty, CardView and TagGroup don't. Was just a bit worried that there might people using these grid hooks directly to build a table (or some other grid pattern) that wouldn't want to disable interactions when their table is empty

snowystinger · 2022-09-01T22:33:59Z

        },
-        onChange
+        onChange,
+        disabled: isDisabled


rather than disabling this, shouldn't we just not render the menu button and not display the resizer on hover?

That should work, I just felt like it would be appropriate to give the user the ability to disable a column resizer but we can add that when a use case arises.

Nevermind, since I ran into issues here I'm going to go back to the isDisabled approach

snowystinger · 2022-09-01T22:37:26Z

 }

 let _TableColumnHeaderButton = (props, ref: FocusableRef<HTMLDivElement>) => {
+  let {isEmpty} = useTableContext();


if the table is empty, maybe we just render a TableColumnHeader instead of one with a menu?

Seems like we need to render the TableResizeColumnHeader if it is resizable, otherwise flipping from empty to non-empty doesn't replace the in view columns with resizable columns (caching probably?)

devongovett · 2022-09-02T18:59:13Z

One small issue I noticed: the column header cells still have tabindex="-1" set. Seems like you can't tab directly to them, but if you right click on them they become tabindex="0", and then you can tab to them. We might want to remove the tab index completely when disabled?

LFDanLu · 2022-09-07T00:31:10Z

@devongovett Hm, I'm not seeing that right click behavior actually. I'll still go ahead and remove the tab index when the table column headers so the previously focused key doesn't interfere with the empty TableView behavior but just wanna double check: what story/browser combo were your using?

devongovett · 2022-09-07T00:56:02Z

renderEmptyState in Safari.

LFDanLu · 2022-09-07T01:06:15Z

@devongovett kk good to know, maybe a old Safari bug, not seeing that on my end with the old storybook.

TODO: seeing some weirdness when toggling items back and forth, resizers appear for some columns and not for others, need to investigate further. The spacing between the checkbox and the column header is too close locally too

the columns in view arent updated from TableColumnHeaders to ResizableTableColumnHeader when going from no items to items in the empty TableView story, so I went back to disabling the resize column button instead

LFDanLu · 2022-09-07T19:18:48Z

One last issue I'm digging into: useVirtualizer overrides the provided grid tabindex, means that the table itself isn't tabbable if the user had focused any of the column headers before the table became empty

adobe-bot · 2022-09-07T19:24:27Z

Build successful! 🎉

virtualizer will provide tabindex=-1 if the focused item is still in view which means a empty table will have tabindex -1 if the column header was focused last. We want the table to be tabbable when empty so we override this

LFDanLu · 2022-09-07T20:13:39Z

+      // Override virtualizer provided tabindex if TableView is empty so it is tabbable.
+      {...mergeProps(otherProps, virtualizerProps, collection.size === 0 && {tabIndex: 0})}


useVirtualizer will return tabindex=-1 if the focused view is still in the TableView, which is problematic for the empty case because the last focused view could be a column header prior to the TableView becoming empty and thus would still be in view. Debated putting this logic into useVirtualizer but I figured that would be too wide of a change. Specifically, in case someone was using Virtualizer to create a component that would keep some focusable views even when the collection was empty but they wanted those view to still be tabbable/didn't want the virtualizer to be tabbable

adobe-bot · 2022-09-07T20:14:57Z

Build successful! 🎉

devongovett

Looks good.

One question: is there any value to focusing the collection itself when it is empty and contains a focusable element? Seems like it's kind of an extra tab stop in that case. Maybe we should do something like we do with tabs where if there are no focusable children, then the collection is focusable, otherwise not? Maybe a future improvement.

LFDanLu · 2022-09-07T21:00:02Z

I mainly did it for consistency with ListView's existing behavior (aka how focus moves to the ListView itself when the collection becomes temporarily empty) and to have the announcement of the link happen separately from entering the tableview. Neither of the two are very strong reasons, and making it behave like tabs sounds like a good idea for consistency. Happy to handle that in a follow up

adobe-bot · 2022-09-07T21:09:35Z

Build successful! 🎉

…table_empty

adobe-bot · 2022-09-08T00:13:24Z

Build successful! 🎉

adobe-bot · 2022-09-08T01:08:14Z

Build successful! 🎉

disabling grid navigation if collection is empty

07bdb89

LFDanLu commented Aug 19, 2022

View reviewed changes

LFDanLu added 4 commits August 22, 2022 17:58

disabling grid cell, rows, and other actions when table is empty

e1d27e4

this makes resizing, clicking on column header and other interactions all inert if the table is empty. Partially makes these changes TableView specific instead of grid wide, to confirm with team.

add focus ring to empty state table

754ed24

Merge branch 'main' of github.com:adobe/react-spectrum into tab_into_…

790e797

…table_empty

adding tests for ListView and TableView empty state

378542d

also fixes some small things the tests revealed like how the hidden inputs in the resizer werent disabled

LFDanLu changed the title ~~(WIP) Disabling grid keyboard navigation if collection is empty~~ (WIP) Disabling TableView/ListView keyboard navigation and press interactions if collection is empty Aug 23, 2022

adding extra test

1ddb884

test is skipped for now due to outstanding bug that is fixed in separate PR

LFDanLu commented Aug 23, 2022

View reviewed changes

LFDanLu changed the title ~~(WIP) Disabling TableView/ListView keyboard navigation and press interactions if collection is empty~~ Disabling TableView/ListView keyboard navigation and press interactions if collection is empty Aug 23, 2022

Merge branch 'main' into tab_into_table_empty

4de7b36

Merge branch 'main' into tab_into_table_empty

6e9c29c

reidbarber reviewed Aug 30, 2022

View reviewed changes

Merge branch 'main' into tab_into_table_empty

ca73890

snowystinger reviewed Sep 1, 2022

View reviewed changes

addressing review comments

1abf609

LFDanLu changed the title ~~Disabling TableView/ListView keyboard navigation and press interactions if collection is empty~~ (WIP) Disabling TableView/ListView keyboard navigation and press interactions if collection is empty Sep 7, 2022

LFDanLu added 2 commits September 7, 2022 12:00

fixing resizer bug when going back to empty state

ad9cc9b

the columns in view arent updated from TableColumnHeaders to ResizableTableColumnHeader when going from no items to items in the empty TableView story, so I went back to disabling the resize column button instead

fixing test

3e116d6

overriding virtualizer provided tabindex when table is empty

7c60e9d

virtualizer will provide tabindex=-1 if the focused item is still in view which means a empty table will have tabindex -1 if the column header was focused last. We want the table to be tabbable when empty so we override this

LFDanLu changed the title ~~(WIP) Disabling TableView/ListView keyboard navigation and press interactions if collection is empty~~ Disabling TableView/ListView keyboard navigation and press interactions if collection is empty Sep 7, 2022

LFDanLu commented Sep 7, 2022

View reviewed changes

devongovett previously approved these changes Sep 7, 2022

View reviewed changes

Merge branch 'main' into tab_into_table_empty

9c0f617

Merge branch 'main' of github.com:adobe/react-spectrum into tab_into_…

ee651b0

…table_empty

LFDanLu dismissed devongovett’s stale review via ee651b0 September 8, 2022 00:05

devongovett approved these changes Sep 8, 2022

View reviewed changes

reidbarber approved these changes Sep 8, 2022

View reviewed changes

Merge branch 'main' into tab_into_table_empty

d7a77c2

devongovett merged commit 09a0489 into main Sep 8, 2022

devongovett deleted the tab_into_table_empty branch September 8, 2022 01:11

		// Override virtualizer provided tabindex if TableView is empty so it is tabbable.
		{...mergeProps(otherProps, virtualizerProps, collection.size === 0 && {tabIndex: 0})}

Conversation

LFDanLu commented Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

Uh oh!

LFDanLu Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adobe-bot commented Aug 23, 2022

Uh oh!

LFDanLu commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adobe-bot commented Aug 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adobe-bot commented Aug 25, 2022

Uh oh!

adobe-bot commented Aug 29, 2022

Uh oh!

reidbarber left a comment

Choose a reason for hiding this comment

Uh oh!

reidbarber Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adobe-bot commented Sep 1, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devongovett commented Sep 2, 2022

Uh oh!

LFDanLu commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devongovett commented Sep 7, 2022

Uh oh!

LFDanLu commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LFDanLu commented Sep 7, 2022

Uh oh!

adobe-bot commented Sep 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adobe-bot commented Sep 7, 2022

Uh oh!

devongovett left a comment

Choose a reason for hiding this comment

Uh oh!

LFDanLu commented Sep 7, 2022

LFDanLu commented Aug 19, 2022 •

edited

Loading

LFDanLu Aug 19, 2022 •

edited

Loading

LFDanLu commented Aug 23, 2022 •

edited

Loading

reidbarber Aug 30, 2022 •

edited

Loading

LFDanLu commented Sep 7, 2022 •

edited

Loading

LFDanLu commented Sep 7, 2022 •

edited

Loading