fix(text-input): do not clobber aria-invalid computed by Field validation#627
fix(text-input): do not clobber aria-invalid computed by Field validation#627MaxLee-dev wants to merge 2 commits into
Conversation
…tion
TextInput이 aria-invalid={invalid}를 항상 명시 전달해, invalid prop이
없을 때 undefined가 Base UI Field 검증이 계산한 aria-invalid="true"를
덮어쓰고 있었다 — data-invalid는 붙지만 보조기술에는 invalid로 전달되지
않는 접근성 결함. invalid prop이 실제로 지정된 경우에만 전달하도록 수정.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: a18800a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 GitHub.
|
|
Warning Review limit reached
Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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 |
|
✅ All tests passed!
Click here if you need to update snapshots. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/components/text-input/text-input.test.tsx (1)
85-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win테스트 로직은 정확하나 axe 접근성 검증 보강 고려
이 테스트는
Field검증 시나리오에서aria-invalid가 유지되는지 정확히 검증하고 있어 버그 수정 목적에 부합합니다. 다만 이번 수정 자체가 접근성(ARIA) 관련이라는 점을 고려하면,vitest-axe로 렌더링 결과물의 접근성 위반 여부까지 함께 검증하면 회귀 방지에 도움이 될 것입니다.♻️ 제안: axe 검증 추가
+ expect(await axe(rendered.container)).toHaveNoViolations(); expect(rendered.getByText('Please enter a valid email')).toBeInTheDocument(); expect(input).toHaveAttribute('aria-invalid', 'true');As per coding guidelines, "Add vitest-axe accessibility checks for public components when feasible."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/components/text-input/text-input.test.tsx` around lines 85 - 101, The current test in TextInput covers the Field validation behavior correctly, but it should also assert accessibility with vitest-axe since this is an ARIA-related public component. In the existing test case around TextInput and Field.Root, add an axe check against the rendered output after interaction, alongside the current aria-invalid assertion, so regressions in accessibility are caught in the same scenario.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/components/text-input/text-input.tsx`:
- Around line 61-63: The TextInput aria-invalid passthrough is still forwarding
false as an explicit value, which can override the Field validation state.
Update the conditional spread in text-input.tsx inside the TextInput component
to match the select.tsx and multi-select.tsx pattern by using invalid ||
undefined so only a true invalid value is passed through and false is omitted.
---
Nitpick comments:
In `@packages/core/src/components/text-input/text-input.test.tsx`:
- Around line 85-101: The current test in TextInput covers the Field validation
behavior correctly, but it should also assert accessibility with vitest-axe
since this is an ARIA-related public component. In the existing test case around
TextInput and Field.Root, add an axe check against the rendered output after
interaction, alongside the current aria-invalid assertion, so regressions in
accessibility are caught in the same scenario.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac18b526-1f37-42c9-b9f2-b9992dbb18ba
📒 Files selected for processing (3)
.changeset/thick-planes-refuse.mdpackages/core/src/components/text-input/text-input.test.tsxpackages/core/src/components/text-input/text-input.tsx
invalid={false}도 aria-invalid="false"로 전달돼 Field 검증이 계산한
값을 덮어쓰던 문제 수정. 단, Base UI Input은 aria-invalid 키가
존재하기만 해도 계산값을 덮어쓰므로 invalid || undefined 대신
truthy 조건부 스프레드를 유지한다.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes AppliedAddressed 1 CodeRabbit feedback item ( Note: the literal suggestion ( Files modified:
Commit: |
noahchoii
left a comment
noahchoii
left a comment
There was a problem hiding this comment.
이 부분은 사용자가 입력한 invalid와 시스템에 의해 설정된 invalid가 달라서 발생하는 문제 같아서, 좀 더 근본적으로 해결할 필요가 있을 것 같네요..!
계속 우선순위가 밀리고 있던 내용이지만, invalid 상태의 관리 책임을 시스템에 전부 위임할지, 아니면 지금처럼 디자이너와 공동으로 관리할 것인지 이 참에 논의를 해보면 좋을 것 같아요!
| // true일 때만 키를 추가 — aria-invalid 키가 존재하기만 해도(값이 false/undefined여도) | ||
| // Base UI가 Field 검증에서 계산한 aria-invalid를 덮어써 버린다 | ||
| {...(invalid && { 'aria-invalid': true })} |
There was a problem hiding this comment.
NOTE: 프리픽스를 추가해서 주석 앵커들을 한 번에 트래킹 할 수 있도록 하면 더 좋을 것 같아요!
There was a problem hiding this comment.
InputGroup에도 남기긴 했지만 changeset을 어떻게 작성하면 좋을지 같이 정리해두고 기준으로 삼으면 좋을 것 같아요!
우선 저는 아래 예시들처럼 체인지로그를 작성하면 좋을 것 같아요!
3 participants
Related Issues
Description of Changes
Summary by CodeRabbit
Bug Fixes
Tests
TextInput이aria-invalid={invalid}를 항상 명시적으로 전달하고 있어,invalidprop이 지정되지 않은 경우undefined값이 Base UI Field 검증이 계산한aria-invalid="true"를 덮어쓰는 문제를 수정했습니다.증상: Field 검증이 실패하면
data-invalid속성은 붙어서 시각적으로는 invalid 상태로 보이지만,aria-invalid가 제거되어 스크린 리더 등 보조기술에는 invalid로 전달되지 않는 접근성 결함이 있었습니다.수정:
invalidprop이 실제로 지정된 경우에만aria-invalid를 전달하도록 변경했습니다. prop이 없으면 Base UI가 계산한 값이 그대로 유지됩니다.invalidprop 미지정aria-invalid없음 (덮어씀)aria-invalid="true"(Base UI 계산값 유지)invalid={true}명시aria-invalid="true"aria-invalid="true"(동일)invalid={false}명시aria-invalid없음aria-invalid없음 (동일)Screenshots
Checklist
🤖 Generated with Claude Code