fix(autocomplete): resolve Firefox focus reset issue #5842
+102
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@nikhil-304 is attempting to deploy a commit to the HeroUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
🦋 Changeset detectedLatest commit: 3917a39 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdded Tab-key tracking and focus selection logic to the autocomplete hook to distinguish keyboard (Tab) vs mouse selection behaviors, updated listbox virtualization activation, and added tests plus a changeset documenting the fixes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Input as AutocompleteInput
participant Hook as useAutocomplete
participant List as ListBox
rect rgba(200,230,255,0.3)
User->>Input: Tab focus (keyboard)
Note right of Input: onKeyDown records Tab -> lastKeyRef = 'Tab'
Input->>Hook: onFocus event
Hook->>Hook: sees lastKeyRef == 'Tab' -> shouldSkipSelect = true
Hook->>Input: set selection (select all text)
end
rect rgba(230,255,210,0.3)
User->>Input: Click (mouse)
Input->>Hook: onFocus event
Hook->>Hook: shouldSkipSelect false -> place cursor at end / preserve cursor
end
alt listbox large
Hook->>List: getListBoxProps -> enable virtualization (size > 50)
else small list
Hook->>List: getListBoxProps -> no virtualization
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
@heroui/accordion
@heroui/alert
@heroui/autocomplete
@heroui/avatar
@heroui/badge
@heroui/breadcrumbs
@heroui/button
@heroui/calendar
@heroui/card
@heroui/checkbox
@heroui/chip
@heroui/code
@heroui/date-input
@heroui/date-picker
@heroui/divider
@heroui/drawer
@heroui/dropdown
@heroui/form
@heroui/image
@heroui/input
@heroui/input-otp
@heroui/kbd
@heroui/link
@heroui/listbox
@heroui/menu
@heroui/modal
@heroui/navbar
@heroui/number-input
@heroui/pagination
@heroui/popover
@heroui/progress
@heroui/radio
@heroui/ripple
@heroui/scroll-shadow
@heroui/select
@heroui/skeleton
@heroui/slider
@heroui/snippet
@heroui/spacer
@heroui/spinner
@heroui/switch
@heroui/table
@heroui/tabs
@heroui/toast
@heroui/tooltip
@heroui/user
@heroui/react
@heroui/system
@heroui/system-rsc
@heroui/theme
@heroui/use-aria-accordion
@heroui/use-aria-accordion-item
@heroui/use-aria-button
@heroui/use-aria-link
@heroui/use-aria-modal-overlay
@heroui/use-aria-multiselect
@heroui/use-aria-overlay
@heroui/use-callback-ref
@heroui/use-clipboard
@heroui/use-data-scroll-overflow
@heroui/use-disclosure
@heroui/use-draggable
@heroui/use-form-reset
@heroui/use-image
@heroui/use-infinite-scroll
@heroui/use-intersection-observer
@heroui/use-is-mobile
@heroui/use-is-mounted
@heroui/use-measure
@heroui/use-pagination
@heroui/use-real-shape
@heroui/use-ref-state
@heroui/use-resize
@heroui/use-safe-layout-effect
@heroui/use-scroll-position
@heroui/use-ssr
@heroui/use-theme
@heroui/use-update-effect
@heroui/use-viewport-size
@heroui/aria-utils
@heroui/dom-animation
@heroui/framer-utils
@heroui/react-rsc-utils
@heroui/react-utils
@heroui/shared-icons
@heroui/shared-utils
@heroui/stories-utils
@heroui/test-utils
commit: |
|
Hey @jrgarciadev 👋 |
wingkwong
changed the title
wingkwong
requested changes
Oct 28, 2025
There was a problem hiding this comment.
- please add changeset
- please include a test case
- your change will alter the existing behaviour. If there is a value in autocomplete, when you tab it to focus, the expected behavior is the whole text got selected and users can type without producing the incorrect input after 1-2 characters (referring to the linked issue). You can also see this behaviour in other components like input.
…Input behavior and prevent partial overwrite
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/components/autocomplete/src/use-autocomplete.ts (2)
149-152: Variable naming could be clearer.The variable
shouldSkipSelectis confusing because when it'sfalse, text selection actually occurs, and when it'strue, selection is skipped. Consider renaming to something more intuitive likeshouldPlaceCursorAtEndor inverting the logic withshouldSelectTextto improve code readability.
513-537: Consider simplifying the onFocus logic.The onFocus handler has multiple conditional branches that could be refactored for better readability. Consider extracting this logic into a separate helper function with clear documentation explaining each case:
- When to place cursor at end (mouse selection)
- When to select all text (Tab selection with matching value)
- Default fallback behavior
Apply this refactor if you want to improve readability:
+const handleTextSelection = ( + target: HTMLInputElement, + shouldSkipSelect: boolean, + selectedItem: any, +) => { + if (!target.value) return; + + const length = target.value.length; + + // Place cursor at end if mouse selection + if (shouldSkipSelect) { + target.setSelectionRange(length, length); + return; + } + + // Select text if Tab selection with matching value + if (selectedItem && target.value === selectedItem.textValue) { + target.select(); + return; + } + + // Default: place cursor at end + target.setSelectionRange(length, length); +}; onFocus: chain( inputProps.onFocus, otherProps.onFocus, (e: React.FocusEvent<HTMLInputElement>) => { - if (shouldSkipSelect) { - if (e.target.value) { - const length = e.target.value.length; - e.target.setSelectionRange(length, length); - } - } else if ( - e.target.value && - state.selectedItem && - e.target.value === state.selectedItem.textValue - ) { - e.target.select(); - } else if (e.target.value) { - const length = e.target.value.length; - e.target.setSelectionRange(length, length); - } + handleTextSelection(e.target, shouldSkipSelect, state.selectedItem); setShouldSkipSelect(false); }, ),packages/components/autocomplete/__tests__/autocomplete.test.tsx (1)
1092-1142: Consider adding edge case tests.While the current tests cover the primary scenarios, consider adding tests for edge cases:
- Clicking into the field after a previous selection (does it preserve cursor position?)
- Tab selection followed by blur and re-focus (is selection preserved/reset correctly?)
- Rapid switching between mouse and keyboard selection methods
These additional tests would provide more confidence in the robustness of the fix, especially for the Firefox-specific focus behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/fix-text-selection.md(1 hunks)packages/components/autocomplete/__tests__/autocomplete.test.tsx(1 hunks)packages/components/autocomplete/src/use-autocomplete.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-27T21:48:35.308Z
Learnt from: adbjo
Repo: heroui-inc/heroui PR: 5846
File: packages/components/tabs/src/tabs.tsx:76-101
Timestamp: 2025-10-27T21:48:35.308Z
Learning: In packages/components/tabs/src/tabs.tsx, the updateCursorPosition useCallback dependency array intentionally includes `cursorRef.current` to handle the case where the cursor span element is unmounted and remounted (e.g., when `disableAnimation` or `disableCursorAnimation` toggles). This ensures the callback is recreated when the ref points to a new element, triggering a dependency chain that re-establishes the ResizeObserver and initializes the new cursor element with the data-initialized attribute.
</learning]
Applied to files:
packages/components/autocomplete/src/use-autocomplete.ts
📚 Learning: 2025-10-25T17:11:59.338Z
Learnt from: adbjo
Repo: heroui-inc/heroui PR: 5846
File: packages/components/tabs/src/tabs.tsx:155-155
Timestamp: 2025-10-25T17:11:59.338Z
Learning: In packages/components/tabs/src/tabs.tsx, the renderTabs useMemo dependency array intentionally includes both `domRef` and `cursorRef` to maintain consistency in how ref objects are handled in dependency arrays, even though ref objects have stable identity across renders.
Applied to files:
packages/components/autocomplete/src/use-autocomplete.ts
📚 Learning: 2025-10-27T21:52:33.324Z
Learnt from: adbjo
Repo: heroui-inc/heroui PR: 5846
File: packages/components/tabs/src/tabs.tsx:115-125
Timestamp: 2025-10-27T21:52:33.324Z
Learning: In packages/components/tabs/src/tabs.tsx, the useEffect dependency array at line 125 intentionally uses `domRef.current` rather than `domRef` because domRef.current can change between renders (when React sets it during the commit phase), whereas domRef itself has stable identity and won't change.
Applied to files:
packages/components/autocomplete/src/use-autocomplete.ts
📚 Learning: 2025-10-25T17:08:46.283Z
Learnt from: adbjo
Repo: heroui-inc/heroui PR: 5846
File: packages/components/tabs/src/tabs.tsx:156-157
Timestamp: 2025-10-25T17:08:46.283Z
Learning: In packages/components/tabs/src/tabs.tsx, the renderTabs useMemo dependency array intentionally includes `variant` and `isVertical` to prevent potential side-effects, even though they might appear redundant based on static analysis.
Applied to files:
packages/components/autocomplete/src/use-autocomplete.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Continuous Release
- GitHub Check: TypeScript
- GitHub Check: Build
- GitHub Check: ESLint
🔇 Additional comments (6)
packages/components/autocomplete/src/use-autocomplete.ts (3)
218-227: Text selection logic works correctly.The logic correctly handles the two selection paths:
- Mouse selection: sets
shouldSkipSelect = true→ cursor placed at end on next focus- Tab selection: sets
shouldSkipSelect = false→ text selected on next focusThis addresses the Firefox focus reset issue by ensuring proper text selection behavior based on interaction method.
431-434: Tab key tracking implementation is correct.Tracking the Tab key press in
onKeyDownallows the component to differentiate between keyboard and mouse-based selections. ThelastKeyRefis appropriately reset inonSelectionChangeafter being consumed.
540-559: The virtualization logic predates the Firefox focus fix—it was not introduced by those changes.The virtualization feature (
isVirtualizedprop with auto-enable threshold at 50 items) already existed before the Firefox focus fix commit (c60b080). The Firefox fix specifically added only theonFocushandler to manage text selection behavior during focus events. These are separate, unrelated features. The virtualization logic is a performance optimization for large lists and has no connection to the Firefox focus reset issue, which involves keyboard/selection behavior management.Likely an incorrect or invalid review comment.
packages/components/autocomplete/__tests__/autocomplete.test.tsx (2)
1092-1117: Mouse selection test validates cursor positioning correctly.The test confirms that selecting an item via mouse click places the cursor at the end of the value without selecting text. This is the expected behavior for mouse-based interactions.
1119-1142: Tab selection test validates text selection correctly.The test confirms that committing a selection via Tab key fully selects the text (selectionStart = 0, selectionEnd = length). This addresses the Firefox focus reset issue by ensuring the text is in a selected state, allowing immediate typing to replace the value.
.changeset/fix-text-selection.md (1)
1-5: Changeset documentation is clear and appropriate.The changeset correctly documents the fix as a patch release and clearly describes the two behavioral changes: text selection on Tab key and cursor positioning on mouse click. This aligns well with the changes implemented in the code.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #5840
📝 Description
This PR fixes the Firefox Autocomplete input reset issue where typing after navigating into the input field using the Tab key caused the text to reset or get overwritten.
The problem occurred only in Firefox and only when focusing via keyboard - clicking into the field worked fine.
The fix ensures that the Autocomplete input now correctly handles focus events in Firefox, maintaining the input state regardless of how the field is focused.
⛳️ Current behavior (updates)
📹 Before videos:
https://github.com/user-attachments/assets/1137923c-6b03-46ac-a93d-07c8a7295a94
https://github.com/user-attachments/assets/76cf2edf-8add-4e22-b8c3-b0d1f2a2100e
🚀 New behavior
📹 After video:
https://github.com/user-attachments/assets/54b31896-c4d1-4273-826a-2fa47b54bb0e
💣 Is this a breaking change (Yes/No):
No
📝 Additional Information
Summary by CodeRabbit
Bug Fixes
New Features