Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(@react-aria/selection): don't mutate non-empty selection upon focus #7513

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alirezamirian
Copy link
Contributor

@alirezamirian alirezamirian commented Dec 12, 2024

Closes #7512

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

The example in the added test case can be used for manual testing. Let me know if more instructions is needed.

@@ -107,5 +107,27 @@ describe('useSelectableCollection', () => {
expect(options[1]).not.toHaveAttribute('aria-selected');
expect(options[2]).toHaveAttribute('aria-selected', 'true');
});

it("doesn't change the selection on focus in multiple selection selectOnFocus", async () => {
Copy link
Member

Choose a reason for hiding this comment

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

We should get a test for tab({shift: true}); as well

Also, expected behavior from Mac Finder suggests that clicking on any of the selected items while focus is currently inside the collection replaces the current selection.

However, if you leave the window (focus is no longer in the collection) and then the user clicks back in on any of the selected items, none of them are replaced.

We should have some tests around those behaviors as well and we'll need to decide what the overall behavior we ultimately want is. I'll bring it up to the team.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Good point about shift+tab, I can update the tests to cover it.

Not sure if "clicking" items should be considered relevant here. And it's handled separately.
What seems clear to me is that if there are selected items, the first/last one should get focused when tabbing into the collection without any side effect on the selection.

@alirezamirian alirezamirian force-pushed the #7512-selection-mutation-on-focus branch from 1e591a5 to 2fb4dd1 Compare December 19, 2024 21:54
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.

🐛 Selection change when selectable list focuses
2 participants