Skip to content

Conversation

@snowystinger
Copy link
Member

@snowystinger snowystinger commented Jun 15, 2021

Closes #1905

Fixes a bug where tabbing into a collection with selectOnFocus wouldn't select anything until the arrow key was used. Now this will select the item that first gains focus.

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

@snowystinger snowystinger changed the base branch from main to 1905-selectonfocus-updates June 15, 2021 20:57
@adobe-bot
Copy link

Build successful! 🎉

@snowystinger snowystinger changed the title 1905 selectonfocus bug selectonfocus bug Jun 15, 2021
@snowystinger snowystinger changed the title selectonfocus bug selectonfocus initial selection bug Jun 15, 2021
manager.setFocused(true);

if (manager.focusedKey == null) {
let navigateToFirstKey = (key: Key | undefined, childFocus?: FocusStrategy) => {
Copy link
Member

Choose a reason for hiding this comment

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

Is the childFocus bit prep for grid cases w/ focusable children? Just trying to figure out if useSelectableCollection will have access to childFocus information in onFocus

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 this function is actually stolen from above https://github.com/adobe/react-spectrum/blob/main/packages/@react-aria/selection/src/useSelectableCollection.ts#L114
it's a bit of copy pasta that i forgot about because lint didn't yell at me, it seems as though childFocus is only for arrow keys, so I'm thinking we don't need to worry about it at all

@adobe-bot
Copy link

Build successful! 🎉

@devongovett
Copy link
Member

Would be nice to have a better PR title and maybe a description for this so I don't need to go to the original bug to find out what this is. 😉

Copy link
Member

@ktabors ktabors left a comment

Choose a reason for hiding this comment

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

LGTM

@snowystinger snowystinger changed the title selectonfocus initial selection bug Select on initial focus Jul 14, 2021
# Conflicts:
#	packages/@react-aria/selection/test/useSelectableCollection.test.js
@snowystinger snowystinger merged commit 1065847 into 1905-selectonfocus-updates Jul 21, 2021
@snowystinger snowystinger deleted the 1905-selectonfocus-bug branch July 21, 2021 22:45
dannify pushed a commit that referenced this pull request Oct 15, 2021
…tView (#2363)

* Followup to selectOnFocus
add demo stories
fix clicking without shift

* add selectionBehavior

* Add story for useSelectableList

* update default

* cleanup stories

* remove extraneous dependency

* support keyboard navigation without replacing the selection in select on focus mode

* fix lint

* Touch multiselect always use toggle (#2046)

* Mobile updates for selection

* Move mobile awareness to the component

* fix lint

* move to touch/vo detection

* Select on initial focus (#2026)

* 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

* Fix ctrl + alt + space

* Hide checkboxes and add border for highlight selection mode

* Expose selectionBehavior option on SelectionManager

* Don't show focus ring when pressing modifier keys

* Fix test

* Fix table tests

* Add useLongPress to @react-aria/interactions

* Implement onAction support in TableView

* Add highlight selection and onAction support to ListView

* Fix tests

* Fix ListView onAction with no selection

* Only require alt key not ctrl + alt

* Fix CardView tests

* Prevent context menu on touch

* Improve event cleanup

* Change non-contiguous selection modifier depending on platform

* Rename to selectionStyle in Spectrum and fix some bugs

* Enforce selection mode better

* code review

* Remove global styles from story

* Add stopPropagation back when continuePropagation is not available

Co-authored-by: Rob Snow <[email protected]>
Co-authored-by: Rob Snow <[email protected]>
Co-authored-by: Daniel Lu <[email protected]>
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.

Follow up to selectOnFocus + multiple selection changes in useSelectableCollection

6 participants