fix(ui): Ensure hidden passworld field is hidden from a11y tree#8899
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: cd5b434 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesAccessibility fix for hidden password input
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
API Changes Report
Summary
🔴 Breaking changes index (1)Every breaking change, up front. Full diffs are in the package sections below.
@clerk/sharedCurrent version: 4.19.0 Subpath
|
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
inert blocks programmatic event dispatch, which broke the cache-components integration test (Playwright fill with force:true silently no-ops on inert elements, causing sign-in to submit without a password). display:none achieves the same a11y goal — removes the row from the accessibility tree — without blocking Playwright's CDP-level interaction.
![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/ui/src/components/SignIn/__tests__/SignInStart.test.tsx (1)
576-577: 🏗️ Heavy liftAdd one browser-level integration test for non-mocked autofill path.
These tests are good for component behavior, but they currently rely on mocked autofill signals. A single Playwright coverage point for real browser autofill would reduce regression risk for this specific
display:nonechange.Also applies to: 588-589, 591-591, 609-613
🤖 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/ui/src/components/SignIn/__tests__/SignInStart.test.tsx` around lines 576 - 577, The existing tests in the SignInStart.test.tsx file for the "Instant password field a11y" describe block are component-level tests that use mocked autofill signals. Add a browser-level integration test using Playwright that tests real browser autofill behavior without mocking, specifically to verify that the display:none CSS change properly hides the instant password field from the accessibility tree during actual autofill in a real browser environment. This integration test should complement the existing component tests and provide regression coverage for this specific a11y behavior change.
🤖 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/ui/src/components/SignIn/SignInStart.tsx`:
- Line 737: In the SignInStart.tsx file, the conditional styling using display:
'none' on line 737 prevents browser autofill detection because major browsers
intentionally skip autofilling inputs in display: none containers. Replace the
display: 'none' hiding method with an alternative CSS approach that keeps the
element visible to the browser's autofill engine (such as visibility: 'hidden',
height: 0 with overflow: 'hidden', or positioning off-screen). Alternatively, if
autofill detection should be intentionally disabled, remove the autofill
detection logic from the InstantPasswordRow component and add test coverage
documenting this new behavior. Clarify in the PR whether autofill functionality
is intended to be disabled or preserved.
---
Nitpick comments:
In `@packages/ui/src/components/SignIn/__tests__/SignInStart.test.tsx`:
- Around line 576-577: The existing tests in the SignInStart.test.tsx file for
the "Instant password field a11y" describe block are component-level tests that
use mocked autofill signals. Add a browser-level integration test using
Playwright that tests real browser autofill behavior without mocking,
specifically to verify that the display:none CSS change properly hides the
instant password field from the accessibility tree during actual autofill in a
real browser environment. This integration test should complement the existing
component tests and provide regression coverage for this specific a11y behavior
change.
🪄 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: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 8cc893dc-e6a4-47fd-b934-b949c3965bdf
📒 Files selected for processing (2)
packages/ui/src/components/SignIn/SignInStart.tsxpackages/ui/src/components/SignIn/__tests__/SignInStart.test.tsx
Replace display:none with off-screen CSS + aria-hidden so the browser autofill engine can still detect and fill the hidden password input. Also restores tabIndex=-1 to block keyboard focus. Updates tests to assert on aria-hidden rather than toBeVisible(), since CSS-in-JS styles are not reflected in jsdom computed styles.
alexcarpenter
force-pushed
the
carp/hidden-password-aria
branch
from
77ff758 to
cd5b434
Compare
2 participants
Description
The hidden instant-password row was still present in the accessibility tree, causing screen readers to announce a password field that isn't available to users visually.
Fix: replace the old CSS-based hiding (absolute positioning + opacity/height tricks) with
display: none. This removes the row from the a11y tree natively without needinginert— which would have blocked programmatic interaction and broke the cache-components integration test (Playwright'sfillsilently no-ops oninertelements).Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Tests
Chores