fix: prevent multiple parent focus events on click #4274
Conversation
🦋 Changeset detectedLatest commit: 476761c The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@Peterl561 is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Rate limit exceeded@jrgarciadev has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis update introduces a patch for the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Peterl561
changed the title
![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
🧹 Outside diff range and nitpick comments (6)
.changeset/tricky-panthers-build.md (1)
1-7: Enhance the changeset description with more detailsWhile the description captures the core fix, consider adding:
- The root cause of the issue
- The specific fix implemented (preventDefault on mouseDown)
- Reference to the related issue ([BUG] - Switch inside of CardBody shows blue outline on toggle #4260)
--- "@nextui-org/checkbox": patch "@nextui-org/switch": patch "@nextui-org/radio": patch --- -fixed checkbox, radio, and switch triggering focus on focusable parent multiple times +fixed checkbox, radio, and switch components triggering multiple focus events on parent elements (issue #4260) + +When these components were placed within a focusable parent container, clicking them would trigger focus events multiple times. This has been resolved by preventing the default mouseDown behavior on the components themselves.packages/components/radio/src/use-radio.ts (1)
153-161: LGTM! Consider adding JSDoc commentsThe implementation correctly prevents multiple focus events. Consider adding JSDoc comments to document the purpose of mouseProps.
+ /** + * Props to prevent multiple focus events when clicking the radio. + * This is needed when the radio is inside a focusable parent. + */ const mouseProps = useMemo( () => ({ onMouseDown: (e: React.MouseEvent<HTMLLabelElement>) => { // prevent parent from being focused e.preventDefault(); }, }), [], );packages/components/switch/__tests__/switch.test.tsx (1)
188-202: LGTM! Consider additional test scenariosThe test effectively verifies the fix for multiple focus events. Consider adding tests for:
- Focus behavior when clicking multiple times
- Focus behavior with disabled state
- Focus behavior with readonly state
it("should not trigger focus on parent when disabled", async () => { const onFocus = jest.fn(); const wrapper = render( <div tabIndex={-1} onFocus={onFocus}> <Switch isDisabled data-testid="switch-test">Switch</Switch> </div> ); const label = wrapper.getByTestId("switch-test"); await user.click(label); expect(onFocus).not.toHaveBeenCalled(); }); it("should maintain single focus behavior on multiple clicks", async () => { const onFocus = jest.fn(); const wrapper = render( <div tabIndex={-1} onFocus={onFocus}> <Switch data-testid="switch-test">Switch</Switch> </div> ); const label = wrapper.getByTestId("switch-test"); await user.click(label); await user.click(label); expect(onFocus).toHaveBeenCalledTimes(1); });packages/components/switch/src/use-switch.ts (1)
181-189: LGTM! Consider adding JSDoc comment.The mouseProps implementation correctly prevents parent focus. Consider adding a JSDoc comment to document the purpose of this handler for future maintainers.
const mouseProps = useMemo( () => ({ + /** + * Prevents parent element from receiving focus when the switch is clicked. + * This fixes the issue of multiple focus events being triggered. + */ onMouseDown: (e: React.MouseEvent<HTMLLabelElement>) => { // prevent parent from being focused e.preventDefault();packages/components/checkbox/src/use-checkbox.ts (2)
267-275: LGTM! Consider adding JSDoc comment.The mouseProps implementation correctly prevents parent focus. Consider adding a JSDoc comment to document the purpose of this handler for future maintainers.
const mouseProps = useMemo( () => ({ + /** + * Prevents parent element from receiving focus when the checkbox is clicked. + * This fixes the issue of multiple focus events being triggered. + */ onMouseDown: (e: React.MouseEvent<HTMLLabelElement>) => { // prevent parent from being focused e.preventDefault();
Line range hint
290-303: Add mouseProps to useCallback dependencies.The mouseProps object is used in getBaseProps but not included in its dependencies array. This could lead to stale closures.
}, [ slots, baseStyles, isDisabled, isSelected, isIndeterminate, isInvalid, isHovered, isFocused, pressed, inputProps.readOnly, isFocusVisible, hoverProps, + mouseProps, otherProps, ]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
.changeset/tricky-panthers-build.md(1 hunks)packages/components/checkbox/__tests__/checkbox.test.tsx(1 hunks)packages/components/checkbox/src/use-checkbox.ts(2 hunks)packages/components/radio/__tests__/radio.test.tsx(1 hunks)packages/components/radio/src/use-radio.ts(2 hunks)packages/components/switch/__tests__/switch.test.tsx(1 hunks)packages/components/switch/src/use-switch.ts(1 hunks)
🔇 Additional comments (3)
packages/components/radio/src/use-radio.ts (1)
179-179: LGTM! Props merging looks correct
The mouseProps are correctly merged with other props using mergeProps utility.
packages/components/checkbox/__tests__/checkbox.test.tsx (1)
93-107: LGTM! Well-structured test case.
The test effectively verifies that focus is triggered only once on the parent element when the checkbox is clicked, directly addressing the issue described in the PR objectives.
packages/components/radio/__tests__/radio.test.tsx (1)
145-163: LGTM! Test case effectively validates the focus handling fix.
The test case properly verifies that clicking a Radio component within a focusable parent triggers the focus event exactly once, which directly addresses the issue described in PR #4260.
A few positive observations about the test implementation:
- Clear test setup with isolated focus event tracking
- Proper use of async/await for user interactions
- Precise assertion checking focus event count
wingkwong
changed the title
|
Hey @Peterl561, thanks a lot for fixing the issue. |
|
Handling this issue in #4306 |
|
closing - fixed in #4311 |
Closes #4260
📝 Description
Checkbox,Radio, andSwitchcomponentspreventDefaultto prevent parent focus from firing multiple times⛳️ Current behavior (updates)
before.mp4
🚀 New behavior
after.mp4
💣 Is this a breaking change (Yes/No):
No
📝 Additional Information
Summary by CodeRabbit