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.

Sign up for GitHub

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

Jump to bottom

fix: [BUG] - Autocomplete - Text selection with Shift + Home/End isn't working (#7228) #7403

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

fix: [BUG] - Autocomplete - Text selection with Shift + Home/End isn't working (#7228) #7403

wants to merge 1 commit into from
+6 −0

Conversation

majornista
Copy link
Collaborator

@majornista majornista commented Nov 19, 2024 •
edited
Loading

Closes #7228

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

  1. Open RAC ComboBox component example https://reactspectrum.blob.core.windows.net/reactspectrum/84b820bec9de9b710dbcdb8a3841d0d137d18aa7/docs/react-aria/ComboBox.html#example
  2. Enter a word in the input, like "Flamingo"
  3. Press ArrowDown to expand the ListBox
  4. Press ArrowLeft to move the cursor within the input to somewhere in the middle of the entry
  5. Press Shift+Home (fn+Shift+ArrowLeft on a laptop) to select from the cursor to the beginning of the entry.
  6. Press Shift+End (fn+Shift+ArrowRight on a laptop) to select from the cursor to the end of the entry.
  7. Use ArrowDown to move focus to an item in the ListBox and to navigate to an item in the middle of the list.
  8. Press Home (fn+ArrowLeft on a laptop) to move focus to the first item in the list.
  9. Press End (fn+ArrowRight on a laptop) to move focus to the last item in the list.
  10. Repeat from step 4.

When there is a focusedKey in the list, Home or End will navigate within the list, but with no focusedKey, Shift+Home/End should select within the input.

🧢 Your Project:

Adobe/Accessibility

@rspbot
Copy link

rspbot commented Nov 19, 2024

Build successful! 🎉

snowystinger
snowystinger approved these changes Nov 20, 2024
View reviewed changes
Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

Seems like the right thing to do

@majornista majornista mentioned this pull request Nov 20, 2024
Merged
5 tasks
@rspbot
Copy link

rspbot commented Nov 20, 2024

Build successful! 🎉

LFDanLu
LFDanLu reviewed Nov 21, 2024
View reviewed changes
packages/@react-aria/selection/src/useSelectableCollection.ts
Comment on lines 224 to +227
if (delegate.getFirstKey) {
if (manager.focusedKey === null && e.shiftKey) {
return;
}
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 this is fine, but I wonder if we should make it consistent with PageUp/Down where Home/End returns early if the focusedKey is null (or if PageUp/Down should be changed so they move focus into the collection even if a focusedKey doesn't exist). Also, I remember that you mentioned noticing an inconsistency between RAC and RSP combobox or something? Mind reminding me what that was?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not noticing the inconsistency between RAC and RSP for this behavior using the current builds for this PR now, but perhaps it's because I've gotten used to a particular way of testing.

Regarding PageUp/PageDown, I'm not sure that adding this code will change anything in the behavior when there is no focusedKey, because, with no focusedKey, PageUp/PageDown currently doesn't seem to do anything.

It may make sense for PageUp/PageDown to move focus into the collection as you suggest.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I also spent some time yesterday seeing if I could change the behavior of the Escape key so that it does not clear the input if the user wants to close the input before editing their search.

@rspbot
Copy link

rspbot commented Nov 21, 2024

Build successful! 🎉

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

@LFDanLu LFDanLu LFDanLu left review comments

@snowystinger snowystinger snowystinger approved these changes

@devongovett devongovett Awaiting requested review from devongovett

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] - Autocomplete - Text selection with Shift + Home/End isn't working
4 participants
@majornista @rspbot @snowystinger @LFDanLu