Skip to content

Conversation

@snowystinger
Copy link
Member

@snowystinger snowystinger commented Jun 11, 2021

Closes #2018
Adds selectionBehavior prop which can be toggle or replace. Previously, it was always toggle, this specifically adds replace.
This is added for mouse/touch and keyboard is handled the way it is in Windows where alt + arrows will allow a user to navigate and select a non-contiguous set of items

✅ 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:

🧢 Your Project:

add demo stories
fix clicking without shift
@adobe-bot
Copy link

Build successful! 🎉

@snowystinger snowystinger changed the title [WIP] - Followup to selectOnFocus Followup to selectOnFocus Jun 14, 2021
@snowystinger snowystinger changed the title Followup to selectOnFocus Selection Behavior 'replace' Jun 14, 2021
@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly story things, but some odd cases noted as well

<code>selectOnFocus</code> is true ❌
</li>
</ol>
<List selectionMode="multiple" disallowEmptySelection>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this one is supposed to disallowEmptySelection? I can deselect all selection via clicking

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I wasn't sure what to do with this one, the code in SelectionManager is VERY specific that disallowEmptySelection only applies to single selection
https://github.com/adobe/react-spectrum/blob/main/packages/@react-stately/selection/src/SelectionManager.ts#L369

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I guess it makes sense that disallowEmptySelection doesn't apply for multiselect, can't really think of a use case for that

.add(
'multiple selection selectionBehavior replace',
() => (
<TableView aria-label="TableView with dynamic contents" selectionMode="multiple" selectionBehavior="replace" width={300} height={200} onSelectionChange={s => onSelectionChange([...s])}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels odd to be unable to deselect a selected row by clicking its checkbox. Also noticed that shift + click doesn't quite work if you click on the row's checkbox cell

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps TableView should work a bit differently with replace?
EDIT: Ah I see, the CMD key modifier allows for this. Is there a spec somewhere that describes the expected behavior?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I was unable to figure out how to do this with Finder, however, you can select disparate files with the mouse by holding CMD, so I thought that might be a good way to approach this. Agreed, less than ideal that CMD + Space (which usually acts as a press) brings up the spotlight search, but I don't have a good alternative and I think it's a well known shortcut, so if someone hits that, they'd automatically try Enter anyways?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is reasonable, perhaps something to note in the docs

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one question but otherwise the behavior seems right to me

// Keyboard events bubble through portals. Don't handle keyboard events
// for elements outside the collection (e.g. menus).
if (e.altKey || !ref.current.contains(e.target as HTMLElement)) {
if ((e.altKey && !e.ctrlKey) || !ref.current.contains(e.target as HTMLElement)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This addition is supposed to prevent ctrl + alt (aka control + option on Mac) from being handled by the child element right? Right now in https://reactspectrum.blob.core.windows.net/reactspectrum/83a514c745dcc6475b057e38ac23c3292b8c8770/storybook/index.html?path=/story/tableview--crud ctrl + alt + arrow down still opens the menu.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that story isn't a replacement behavior, so it shouldn't need this anyways?

@devongovett
Copy link
Member

So I think there's a few things that need to happen here:

  1. selectionBehavior = 'replace' and selectOnFocus should be a single option (selectionBehavior is a better name). They should always be applied together, and shouldn't be used separately.
  2. I see that ctrl + alt + arrow keys moves the focus without changing the selection, but I couldn't figure out a way to add the focused item to the selection without replacing it. Should that be ctrl + alt + space key or something?
  3. I think we should wait on having a UI for highlight style before merging this. It shouldn't be possible to have checkboxes with selectionBehavior = replace. Still waiting on spectrum for this, so maybe we can combine your PRs related to this into a single feature branch until then.

* 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
@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

multiple selection selectionBehavior

5 participants