fix(input): ensure clear button is not focusable when disabled #3774
Conversation
🦋 Changeset detectedLatest commit: 10b4a1d The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
|
@ryxxn is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis update introduces a patch to 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
|
and update tests for clear button using button element
|
@ryo-manba This change removes the need to manually handle tabIndex and role, while improving accessibility. |
wingkwong
requested review from
wingkwong
and removed request for
jrgarciadev
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Thank you for the feedback! |
![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 (3)
packages/components/input/__tests__/input.test.tsx (3)
48-54: LGTM: New test case for non-focusable clear button.This test case effectively verifies that the clear button is not focusable by checking its
tabIndexattribute. It aligns well with the PR objective and uses the recommendedgetByRolemethod for querying elements.Consider adding an additional assertion to check if the button is not disabled in this case, as it would help distinguish between the behavior of a disabled button and a non-focusable button:
expect(clearButton).not.toBeDisabled();
160-160: LGTM: Updated clear button selection method.The change to use
getByRole("button")aligns with best practices for querying elements in tests.While the non-null assertion (
!) ensures type safety, consider handling the case where the button might not be present:const clearButton = getByRole("button", { name: /clear/i }); expect(clearButton).toBeInTheDocument();This approach would provide a more descriptive error if the button is missing and aligns with Testing Library's error-first philosophy.
275-275: LGTM: Improved submit button selector specificity.The change to use
querySelector('button[type="submit"]')improves the specificity of the submit button selection, which is a good practice for robust tests.Consider using Testing Library's
getByRolemethod for consistency with other tests and better accessibility representation:submitButton = screen.getByRole('button', { type: 'submit' });This approach would also eliminate the need for the non-null assertion and provide a more descriptive error if the button is missing.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/components/input/tests/input.test.tsx (4 hunks)
🔇 Additional comments not posted (1)
packages/components/input/__tests__/input.test.tsx (1)
40-46: LGTM: New test case for disabled clear button.This test case effectively verifies that the clear button is disabled when the
isDisabledprop is set to true. It aligns well with the PR objective and uses the recommendedgetByRolemethod for querying elements.
There was a problem hiding this comment.
I think you misunderstood my comment.
First the PR here is to fix the issue where currently the clear button is focusable when the input is disabled. With your PR changes, it looks fine.
In my previous comment, I was saying that the clear button can be focusable when the input is not disabled. With your PR changes, this existing expected behaviour was changed, which is not expected. I've also provided two links for you to compare. That's why I asked for changes and a new test case to cover the base case.
Therefore, I'm expecting two test cases.
- should allow focus on clear button when input is not disabled
- should not allow focus on clear button when input is disabled
|
I previously received feedback suggesting that setting Could we discuss which approach aligns better with the project's accessibility guidelines? Should the clear button always be non-focusable for accessibility, or should it remain focusable when the input is not disabled? I can adjust the implementation based on the preferred approach. |
|
I see. Lemme discuss with Ryo internally first and get back to you afterwards. Thanks. |
|
confirmed the clear button will not be focusable no matter it is disabled or not. ref: |
📝 Description
This PR fixes an issue where the
clear buttoninside theInputcomponent could still receive focus when the input is disabled (isDisabled). The button is now properly excluded from the tab order by settingtabIndexto-1when the input is disabled.⛳️ Current behavior (updates)
record.mov
Currently, the
clear buttonin theInputcomponent can still receive focus via the Tab key when the input is disabled. This behavior can cause accessibility issues and an inconsistent user experience.🚀 New behavior
With this fix, the
clear buttonwill no longer be focusable when the input is disabled. ThetabIndexfor theclear buttonis set to-1whenisDisabledis true, preventing it from being part of the tab order.💣 Is this a breaking change (Yes/No): No
📝 Additional Information
This fix enhances accessibility and user experience by ensuring that the
clear buttonbehaves correctly when the input is disabled.Summary by CodeRabbit
<span>to a<button>element for improved semantic meaning and usability.tabIndexproperty for the clear button to ensure compliance with keyboard navigation standards.