fix: accordion item focus effect input focus event #4201
Conversation
|
|
@chioio is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
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
|
![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 (1)
packages/components/accordion/src/accordion-item.tsx (1)
93-98: Consider centralizing event handling logicThe event handling pattern with
stopPropagation()is repeated across multiple handlers. Consider extracting this into a utility function for better maintainability.+ const stopEventPropagation = (e: React.SyntheticEvent) => { + e.stopPropagation(); + }; + <m.section key="accordion-content" animate="enter" exit="exit" initial="exit" style={{willChange}} variants={transitionVariants} - onClick={(e) => { - e.stopPropagation(); - }} - onFocus={(e) => { - e.stopPropagation(); - }} - onKeyDown={(e) => { - e.stopPropagation(); - }} + onClick={stopEventPropagation} + onFocus={stopEventPropagation} + onKeyDown={stopEventPropagation} {...motionProps} >
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/components/accordion/src/accordion-item.tsx(1 hunks)
🔇 Additional comments (1)
packages/components/accordion/src/accordion-item.tsx (1)
93-98: LGTM! The event handler additions effectively address the input focus issue.
The addition of onClick and onFocus handlers with stopPropagation() aligns well with the existing pattern and should resolve the input focus errors within accordion items.
Let's verify if there are any other event-dependent components within accordions:
✅ Verification successful
The event handler additions are safe and consistent with existing patterns
The verification shows that:
- The new
onClickandonFocushandlers withstopPropagation()are consistent with the existing event handling pattern in the accordion component - The same
stopPropagation()approach is already used in other event handlers within the same component - There are no conflicting event handling patterns in the related hook packages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for interactive elements (inputs, buttons, etc.) within accordion components
rg -l "AccordionItem" | xargs rg -l "input|button|select|textarea"
# Search for any existing event handlers in accordion-related files
ast-grep --pattern 'onClick={$_} | onFocus={$_} | onBlur={$_}'
Length of output: 1006
Script:
#!/bin/bash
# Let's examine the event handling patterns in accordion-related files more closely
rg -A 3 -B 3 "onClick=|onFocus=|onBlur=" packages/components/accordion/src/
rg -A 3 -B 3 "onClick=|onFocus=|onBlur=" packages/hooks/use-aria-accordion
# Also check for any existing event propagation patterns
rg "stopPropagation" packages/components/accordion/src/
rg "stopPropagation" packages/hooks/use-aria-accordion
Length of output: 1465
|
@wingkwong @ryo-manba please review |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/components/accordion/__tests__/accordion.test.tsx (2)
360-364: Improve type safety and query methodThe current implementation uses querySelector and non-null assertion (!), which could be replaced with more type-safe and semantic Testing Library queries.
- const input = wrapper.container.querySelector("input"); - - expect(input).not.toBeNull(); - - await user.click(input!); + const input = wrapper.getByRole('textbox', { name: 'name' }); + await user.click(input);
349-367: Test coverage could be more comprehensiveThe test verifies basic focus behavior but doesn't explicitly test the bug fix mentioned in the PR (event propagation). Consider adding additional assertions to verify that:
- Focus events don't propagate to parent elements
- The accordion doesn't collapse when clicking the input
Here's a suggested enhancement:
it("should handle input focus without triggering accordion collapse", async () => { const onSelectionChange = jest.fn(); const wrapper = render( <Accordion defaultExpandedKeys={["1"]} onSelectionChange={onSelectionChange}> <AccordionItem key="1" title="Accordion Item 1"> <Input label="name" type="text" /> </AccordionItem> </Accordion> ); const input = wrapper.getByRole('textbox', { name: 'name' }); await user.click(input); expect(input).toHaveFocus(); expect(onSelectionChange).not.toHaveBeenCalled(); expect(wrapper.getByRole('button')).toHaveAttribute('aria-expanded', 'true'); });
Test, changeset added, please review. @wingkwong I resolved it in my project by using a pnpm patch to add the code. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.changeset/short-bulldogs-divide.md (1)
1-5: Consider enhancing the changeset descriptionWhile the changeset is correctly structured with the appropriate patch version bump, the description could be more detailed to help users and maintainers understand:
- The specific issue that was occurring with input focus
- Why adding
onClickandonFocushandlers resolves the issue- The impact on users who were experiencing this bug
Here's a suggested enhancement:
--- "@nextui-org/accordion": patch --- -Fix input focus error in default expanded accordion item, add `onClick`, `onFocus` to `AccordionItem` content wrapper +Fix input focus error in default expanded accordion items where inputs would not receive focus correctly on page load. Added `onClick` and `onFocus` event handlers to the `AccordionItem` content wrapper to prevent event propagation and ensure proper focus behavior. This improves the user experience when using form inputs within pre-expanded accordion sections.
| </Accordion>, | ||
| ); | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 100)); |
|
|
||
| it("should focus input inside default expanded accordion item correctly", async () => { | ||
| const wrapper = render( | ||
| <Accordion selectedKeys={["1"]}> |
There was a problem hiding this comment.
if it's default expanded, then you should use defaultExpandedKeys
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| }} | ||
| onFocus={(e) => { | ||
| e.stopPropagation(); | ||
| }} |
There was a problem hiding this comment.
There's another case that need to cover. See L67.
📝 Description
Fix: input focus error when using in accordion with selected/default expand keys on page loaded.
Mini-reproduction: nextui-input-focus-error
⛳️ Current behavior (updates)
Add
onClick,onFocusevent in accordion item content render.🚀 New behavior
Input component focus behavior normal in accordion component with selected/default expand keys
💣 Is this a breaking change (Yes/No):
No
📝 Additional Information
Nothing!
Summary by CodeRabbit