Add support for highlight selection and onAction to TableView and ListView #2363

devongovett · 2021-09-21T04:25:50Z

Closes #1638. Closes #1905. Closes #2013. Closes #2018. Relates to: #2266. Relates to: #789.

This implements two features across all collection components (currently TableView and ListView):

Highlight selection – this allows checkboxes to be hidden, and a highlight style to be applied to the row to indicate selection instead. It also changes the selection behavior, so that clicking an item replaces the entire selection with that item rather than toggling it. Multiple items can be selected using modifier keys like Ctrl/Cmd and Shift. On touch, highlight selection behaves like toggle because there are no modifiers available. This can be enabled using the selectionBehavior="replace" prop (TBD).
onAction – this prop allows a separate action to be defined for rows in addition to selection. For mouse, this is double click. For keyboard, it's the Enter key. On touch, the action becomes the primary tap interaction, and a long press enters into selection mode. Deselecting all items exits selection mode. The useLongPress hook has been added to @react-aria/interactions to implement this.

This also required moving the usePress call into useSelectableItem rather than calling it from each usage site, so there are many changes related to that.

To do:

Talk to a11y about iOS Safari not announcing aria-describedby on grid rows/cells. Possible put the long press description on the grid instead.
Determine if we want both onRowAction and onCellAction for tables. Answer: keep onAction on row, add onCellAction later if needed.
Decide on API for enabling highlight selection in Spectrum (e.g. selectionStyle="highlight" vs selectionBehavior="replace"`). Answer: update to use selectionStyle.

add demo stories fix clicking without shift

… on focus mode

* Mobile updates for selection * Move mobile awareness to the component * fix lint * move to touch/vo detection

* Select on focus first time in collection * fix lint * Only start selecting on first focus if selectOnFocus is on * add a test * fix aria * fix lint * remove dead code * fix lint

…/react-spectrum-v3 into highlight-selection # Conflicts: # packages/@react-spectrum/table/stories/Table.stories.tsx

…o highlight-selection

adobe-bot · 2021-09-21T04:36:09Z

Build successful! 🎉

LFDanLu

Menus also look weird after the merge with main:

LFDanLu · 2021-10-01T21:08:49Z

packages/@react-aria/selection/src/useSelectableItem.ts

+    onLongPress(e) {
+      if (e.pointerType === 'touch') {
+        onSelect(e);
+        manager.setSelectionBehavior('toggle');
+      }
+    }


Say there is the following scenario:

The user long presses on a ListView in mobile and selects a couple row to perform some bulk action.

The user then presses some button outside of the ListView to execute said bulk action.

How is the user supposed to exit from the "toggle" selection behavior? I know that you mentioned deselecting all the rows will flip it back to the original "replace" behavior, perhaps we should provide a "select all" checkbox like TableView has if selectionMode is "multiple".
Hmm, perhaps the external bulk action action would set selected keys to an empty array, aka handled by the application

Also, should this have an accessibilityDescription added here?

Yeah setting the selectedKeys back to empty should clear it. As for accessibilityDescription, that's one of my questions in the pr description. Unfortunately, VoiceOver doesn't read aria-describedby on a gridcell...

packages/@react-aria/utils/src/useGlobalListeners.ts

packages/@react-spectrum/listbox/test/ListBox.test.js

LFDanLu · 2021-10-01T21:54:52Z

packages/@react-aria/selection/src/useSelectableItem.ts

+  // Pressing the Enter key with selectionBehavior = 'replace' performs an action (i.e. navigation).
+  let onKeyDown = hasSecondaryAction ? (e: KeyboardEvent) => {
+    if (e.key === 'Enter') {
+      onAction();


Should onAction be fired if the user pressed option + Enter? I figured that would toggle selection only

Hmm, where did you get that assumption?

Well, it just felt a bit inconsistent that other interaction modes have onAction and selection as separate actions (e.g. w/ mouse, you double click to trigger onAction in highlight selection mode and change selection with single click. onAction doesn't fire on single click here).

packages/@react-aria/selection/stories/useSelectableList.stories.tsx

packages/@react-spectrum/table/stories/Table.stories.tsx

adobe-bot · 2021-10-13T01:15:09Z

Build successful! 🎉

adobe-bot · 2021-10-13T01:34:59Z

Build successful! 🎉

LFDanLu

Still haven't tested tableview on mobile, preliminary review

LFDanLu · 2021-10-14T18:37:23Z

packages/@react-aria/selection/src/useSelectableItem.ts

+    onLongPress(e) {
+      if (e.pointerType === 'touch') {
+        onSelect(e);


In the "selectionStyle: highlight, onAction" ListView story:
using talkback on android, I'm able to toggle selection on a ListView item via double tapping it a couple of times. This triggers onAction a couple of times (which is expected) and then the item gets the selected styles. If you then perform a long press via tap + tap and hold on a different item, you'll be able to see that the previous item is indeed selected.

For highlight selection + onAction, selection of a list item shouldn't be possible without entering the selection mode view via long press right?

This seems to happen in the TableView "selectionStyle: highlight, onAction" story as well

Sounds like a great bug to follow up on. 😉

😄 I'll make a note

packages/@react-aria/selection/src/useSelectableItem.ts

LFDanLu · 2021-10-14T19:14:48Z

packages/@adobe/spectrum-css-temp/components/table/skin.css

+  &.spectrum-Table-row--highlightSelection {
+    border-bottom-color: var(--spectrum-global-color-blue-500);
+  }


Noticed that the top border is missing when the top most row is selected:

TBH, do we really want this border color when it is selected? It kinda looks strange IMO with the focus ring also applying a border:

First one will be interesting because it's the border of the whole table, not of the row... Second one is spectrum question

LFDanLu · 2021-10-14T19:17:37Z

packages/@react-spectrum/table/stories/Table.stories.tsx

+  .add(
+    'selectionStyle: highlight',
+    () => (
+      <TableView aria-label="TableView with dynamic contents" selectionMode="multiple" selectionStyle="highlight" width={500} height={400} onSelectionChange={s => onSelectionChange([...s])}>


Should onSelectionChange be firing with the same selection when navigating within the currently selected row (e.g. focusing the cells via keyboard) or hitting space a bunch of times? Kinda like how Tabs work I guess so this might be fine

Yeah it's weird. I tried to get rid of this behavior but it seems a bunch of components rely on it... :/

LFDanLu · 2021-10-14T20:35:32Z

packages/@react-spectrum/table/stories/Table.stories.tsx

+    'selectionStyle: highlight',
+    () => (
+      <TableView aria-label="TableView with dynamic contents" selectionMode="multiple" selectionStyle="highlight" width={500} height={400} onSelectionChange={s => onSelectionChange([...s])}>


In the "selectionStyle: highlight" TableView story, multiple row selection via Talkback only works if you double tap when focusing a cell. If the entire row is focused and you double tap to select it, it actually unselects all previously selected rows and only selects the current row.

LFDanLu · 2021-10-14T20:36:17Z

packages/@react-aria/selection/src/useSelectableItem.ts

+    onLongPress(e) {
+      if (e.pointerType === 'touch') {
+        onSelect(e);


This seems to happen in the TableView "selectionStyle: highlight, onAction" story as well

LFDanLu

Tested TableView stories on Android, found 1 extra thing

snowystinger · 2021-10-14T20:44:08Z

packages/@react-aria/interactions/test/useLongPress.test.js

+  let {elementType: ElementType = 'div', onPress, onPressStart, onPressEnd, ...otherProps} = props;
+  let {longPressProps} = useLongPress(otherProps);
+  let {pressProps} = usePress({onPress, onPressStart, onPressEnd});
+  return <ElementType {...mergeProps(longPressProps, pressProps)} tabIndex="0">test</ElementType>;


Order matters here, if longPress is first in the merge, then longPress events will happen first, but if press is first, then press events will happen first
This could be troublesome if we're not careful with it everywhere and consistent, maybe we should have a hook that combines them in the right order? I can't think of a great solution off the top of my head

You mean onPressStart and onLongPressStart? Where would you expect this to be problematic?

Just trying to think if others combine it on other components, it might be different component to component

<ComponentA onPressStart={onPressStart} onLongPressStart={onLongPressStart} /> vs <ComponentB onPressStart={onPressStart} onLongPressStart={onLongPressStart} /> might yield onPressStart then onLongPressStart for ComponentA but onLongPressStart then onPressStart for ComponentB

packages/@react-aria/selection/src/useSelectableItem.ts

packages/@react-aria/utils/src/useGlobalListeners.ts

packages/@react-aria/grid/src/useGridCell.ts

packages/@react-spectrum/table/test/Table.test.js

packages/@react-stately/selection/src/useMultipleSelectionState.ts

adobe-bot · 2021-10-15T01:58:30Z

Build successful! 🎉

LFDanLu

Approving, other comments to be handled in followup

adobe-bot · 2021-10-15T20:05:37Z

Build successful! 🎉

adobe-bot · 2021-10-15T21:56:56Z

Build successful! 🎉

dannify · 2021-10-15T22:19:25Z

Some follow up tickets will be added for this.

snowystinger

yep, looks good to me

snowystinger and others added 27 commits June 11, 2021 15:12

Followup to selectOnFocus

f8acf33

add demo stories fix clicking without shift

add selectionBehavior

cc4baaf

Add story for useSelectableList

425c142

update default

fa1f1f8

cleanup stories

1fd01ba

remove extraneous dependency

af04353

Merge branch 'main' into 1905-selectonfocus-updates

7855bb3

support keyboard navigation without replacing the selection in select…

877ae2c

… on focus mode

fix lint

3bbe205

Merge branch 'main' into 1905-selectonfocus-updates

83a514c

Touch multiselect always use toggle (#2046)

b78e0bb

* Mobile updates for selection * Move mobile awareness to the component * fix lint * move to touch/vo detection

Merge branch 'main' into 1905-selectonfocus-updates

15a7e9f

Select on initial focus (#2026)

1065847

* Select on focus first time in collection * fix lint * Only start selecting on first focus if selectOnFocus is on * add a test * fix aria * fix lint * remove dead code * fix lint

fix lint

e5893bb

Merge branch '1905-selectonfocus-updates' of github.com:adobe-private…

45bdb8d

…/react-spectrum-v3 into highlight-selection # Conflicts: # packages/@react-spectrum/table/stories/Table.stories.tsx

Fix ctrl + alt + space

c73e987

Hide checkboxes and add border for highlight selection mode

c32a9dd

Expose selectionBehavior option on SelectionManager

32a9ba4

Don't show focus ring when pressing modifier keys

3eb3ef5

Merge branch 'main' of github.com:adobe-private/react-spectrum-v3 int…

79e62b2

…o highlight-selection

Fix test

0937428

Fix table tests

5f820d6

Add useLongPress to @react-aria/interactions

ca96678

Implement onAction support in TableView

b6b17ce

Add highlight selection and onAction support to ListView

e6a0217

Merge branch 'main' of github.com:adobe-private/react-spectrum-v3 int…

77e7233

…o highlight-selection

Fix tests

cf68261

devongovett requested review from dannify and snowystinger as code owners September 21, 2021 04:25

LFDanLu requested changes Oct 1, 2021

View reviewed changes

This was referenced Oct 1, 2021

Grid edit mode #2375

Closed

Add support for long press to MenuTrigger #780

Closed

Rename to selectionStyle in Spectrum and fix some bugs

79fb117

devongovett removed the needs API review label Oct 13, 2021

Enforce selection mode better

84607d8

LFDanLu reviewed Oct 14, 2021

View reviewed changes

snowystinger reviewed Oct 14, 2021

View reviewed changes

code review

619bfc0

matthewdeutsch changed the title ~~Add support for highlight selection and onAction to TableView and ListView~~ #2353 Oct 15, 2021

devongovett changed the title ~~#2353~~ Add support for highlight selection and onAction to TableView and ListView Oct 15, 2021

LFDanLu previously approved these changes Oct 15, 2021

View reviewed changes

Remove global styles from story

b106532

devongovett dismissed LFDanLu’s stale review via b106532 October 15, 2021 19:56

Merge branch 'main' into highlight-selection

4d4c1be

Add stopPropagation back when continuePropagation is not available

bb3d4a3

dannify approved these changes Oct 15, 2021

View reviewed changes

LFDanLu approved these changes Oct 15, 2021

View reviewed changes

snowystinger approved these changes Oct 15, 2021

View reviewed changes

dannify merged commit 885b5e6 into main Oct 15, 2021

dannify deleted the highlight-selection branch October 15, 2021 22:35

devongovett mentioned this pull request May 4, 2022

Enable onAction with checkbox selection in ListView and TableView #3103

Merged

1 task

Add support for highlight selection and onAction to TableView and ListView #2363

Add support for highlight selection and onAction to TableView and ListView #2363

Uh oh!

Conversation

devongovett commented Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adobe-bot commented Sep 21, 2021

Uh oh!

LFDanLu left a comment

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!

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!

Uh oh!

Uh oh!

adobe-bot commented Oct 13, 2021

Uh oh!

adobe-bot commented Oct 13, 2021

Uh oh!

LFDanLu left a comment

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!

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!

LFDanLu left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adobe-bot commented Oct 15, 2021

Uh oh!

LFDanLu left a comment

Choose a reason for hiding this comment

Uh oh!

adobe-bot commented Oct 15, 2021

Uh oh!

adobe-bot commented Oct 15, 2021

Uh oh!

dannify commented Oct 15, 2021

devongovett commented Sep 21, 2021 •

edited

Loading