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

Keep focus inside of ComboboxInput #6783

Merged
merged 4 commits into from
Jul 29, 2024
Merged

Keep focus inside of ComboboxInput #6783

merged 4 commits into from
Jul 29, 2024

Conversation

chirokas
Copy link
Contributor

Issue can be seen in the docs or storybook on mobile.

When you open the combobox via the button, the focus moves briefly to the button, and then we restore the focus to the input itself.

This causes a tiny selection flicker (because the selection is removed when the focus moves to the button)

Also, We want to make sure that we don't accidentally trigger the virtual keyboard. This would happen if the input is focused, the options are open, and you click the ComboboxButton (which would blur the input and focus the button, then we refocus the input).

This PR improves on that behaviour by using onPressStart instead of onPress. If we use inputRef.current.focus() in onPressStart, then the focus will not be moved to the button and the focus remains in the input.

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.

Thanks for the PR. I'm pretty sure the current behavior is intentional. With touch, it's easy to interact with the wrong element, so we make sure that the interaction was international, which means waiting to see if the user drags their finger away from the trigger to cancel.

You will see this in components such as Menu as well. The menu won't open until onPress, and likewise, an element won't be selected until onPress.

We might be able to move the focus onPressStart, but wait to open until onPress.

@LFDanLu I think you've been involved in some of this work, do you know if there a reason we don't move focus during onPressStart before opening?

@chirokas
Copy link
Contributor Author

We might be able to move the focus onPressStart, but wait to open until onPress.

@snowystinger I think this can be solved by preventFocusOnPress.

// packages/@react-aria/combobox/src/useComboBox.ts
  buttonProps: {
    ...menuTriggerProps,
    ...triggerLabelProps,
    excludeFromTabOrder: true,
+    // @ts-ignore - undocumented
+    preventFocusOnPress: true,
    onPress,
    onPressStart,
    isDisabled: isDisabled || isReadOnly,
  },

@snowystinger
Copy link
Member

Yes, I think that would be better, that's what we do with NumberField as well


Though I'll still have Daniel check if there was a reason we weren't already doing that. Thanks for digging a little extra.

@LFDanLu
Copy link
Member

LFDanLu commented Jul 29, 2024

@snowystinger from what I recall it is exactly what you mentioned, aka mimicking the same behavior as menus/other trigger elements.

EDIT:
I think using preventFocusOnPress should be fine, will need to test across all devices to be sure though

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.

Tested on desktop and mobile, verified that the behavior with the virtual keyboard is much better now in RAC. I think this is good enough for now, we'll test it more rigorously in a upcoming test session. Thanks for the change!

@snowystinger snowystinger merged commit 1de2739 into adobe:main Jul 29, 2024
29 checks passed
@chirokas chirokas deleted the fix/keep-focus-in-combobox-input branch July 30, 2024 06:47
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.

3 participants