Skip to content

fix(ComboBox): avoid extra onSelectionChange calls#9729

Merged
snowystinger merged 2 commits intomainfrom
combobox-fix-extra-onSelectionChange-calls
Mar 17, 2026
Merged

fix(ComboBox): avoid extra onSelectionChange calls#9729
snowystinger merged 2 commits intomainfrom
combobox-fix-extra-onSelectionChange-calls

Conversation

@reidbarber
Copy link
Member

Closes #9727

Before:

  • Selecting an option fired onSelectionChange once (expected)
  • Clicking away fired it one extra time
  • Tabbing away fired it two extra times

After:

  • No extra onSelectionChange calls occur when values are already in sync

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

Unit tests should cover, but smoke test in storybook.

🧢 Your Project:

@github-actions github-actions bot added the RAC label Mar 3, 2026
@rspbot
Copy link

rspbot commented Mar 3, 2026

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.

doing some testing still, but do you know if this was a regression? Feels like didn't use to happen...

EDIT: has been happening since last Feb at least, so probably not a regression

expect(preventDefault).toHaveBeenCalledTimes(1);
});

it('should only call commit on Tab when the menu is open', function () {
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 still true if it allows a custom value?

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 useComboBoxState will still clear the selected key on field blur via

still

LFDanLu
LFDanLu previously approved these changes Mar 17, 2026
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.

I think these changes are fine after walking through it, but will need to do some careful testing.

Comment on lines +373 to +374
// If multiple things are controlled, call onSelectionChange only when selecting the focused item,
// or when inputValue needs to be synced back to the selected item on commit/blur.
Copy link
Member

Choose a reason for hiding this comment

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

Decided to post this for others walking this logic for this change:
commitSelection is only called in these cases:

  • when revert is called aka pressing Esc, which should revert the inputValue and make sure the selectedKey doesn't change
  • when inputValue === itemText in the allowCustomValue case of commitValue of which results in a no-op except for multiple selection mode which we don't really support anyways
  • conversely, used to reset the inputValue and close the menu in commitValue if allowCustomValue is false. This is only actually called as .close or if the menu is already closed and we are committing the value
  • as a part of commit if we are selecting an already selected key

of those cases, we only really need to fire the onSelectionChange and onChange for the last two cases which seems to check out with new conditions here. The only one I'm not entirely sure is the selectionMode === 'multiple' part of the check, do we really need that? It might not hurt to have it I guess since it would simply call it for an extra case where inputValue === itemText but that is only when inputValue = '' since selectedKey is always null for multiple selection

@snowystinger snowystinger enabled auto-merge March 17, 2026 22:55
@rspbot
Copy link

rspbot commented Mar 17, 2026

@snowystinger snowystinger added this pull request to the merge queue Mar 17, 2026
Merged via the queue into main with commit b5cbf5b Mar 17, 2026
29 checks passed
@snowystinger snowystinger deleted the combobox-fix-extra-onSelectionChange-calls branch March 17, 2026 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[RAC ComboBox] onSelectionChange is called extra times when focus is changed after selection (fully controlled)

4 participants