feat: Added focus ring for focus visible #37700
Conversation
…o ads-v2/focus-ring
WalkthroughThe pull request introduces several modifications to the CSS styles for the Changes
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
|
github-actions
bot
added
IDE Navigation
|
/build-deploy-preview skip-test=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12025302772. |
![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
🧹 Outside diff range and nitpick comments (1)
app/client/packages/design-system/ads/src/__theme__/default/index.css (1)
242-244: LGTM! Consider documenting the focus ring variables.The focus ring variables follow accessibility best practices. However, there's a TODO comment about focus issues across the platform.
Consider adding a comment block explaining the purpose and usage of these focus ring variables for better maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/client/packages/design-system/ads/src/Select/styles.css(2 hunks)app/client/packages/design-system/ads/src/__theme__/default/index.css(2 hunks)app/client/src/index.css(0 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/index.css
🔇 Additional comments (3)
app/client/packages/design-system/ads/src/Select/styles.css (2)
53-53: LGTM: Focus ring offset implementation
The fixed -2px offset provides consistent focus indication across browsers, improving accessibility for keyboard users.
115-117: LGTM: Clean focus state implementation
Setting transparent border prevents double-border effect while maintaining the focus ring visibility. This is a good practice for accessible focus states.
app/client/packages/design-system/ads/src/__theme__/default/index.css (1)
255-258: LGTM! Verify browser compatibility for :focus-visible.
The focus ring implementation follows modern accessibility patterns. The use of !important is appropriate here to prevent focus ring suppression.
Let's verify browser support for the :focus-visible pseudo-class:
✅ Verification successful
Focus ring implementation is compatible with modern browsers
The codebase shows consistent focus ring implementation across components using :focus-visible with fallback .ads-v2-focus-ring class. The styles are properly scoped and use CSS custom properties for consistent theming.
Key findings from the codebase scan:
:focus-visibleis used consistently in design system components- Proper outline styles with
!importantto prevent overrides - CSS custom properties for outline width, color and offset
- No conflicting browser-specific focus styles found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any browser-specific focus styles in the codebase
# that might conflict with the new implementation
# Search for any existing focus-related styles
rg -g '*.css' -i '(:focus|focus-|outline:)'
# Search for browser-specific focus style overrides
rg -g '*.css' -i '(-moz-|webkit-|ms-).*(focus|outline)'
Length of output: 125880
| @@ -50,8 +50,7 @@ | |||
| /* Focused */ | |||
| .ads-v2-select.rc-select-focused { | |||
| outline: var(--ads-v2-border-width-outline) solid var(--ads-v2-color-outline); | |||
| outline-offset: var(--ads-v2-offset-outline); | |||
| --select-color-border: var(--ads-v2-colors-control-field-active-border); | |||
| outline-offset: -2px; | |||
There was a problem hiding this comment.
💡 Codebase verification
Focus ring contrast needs improvement
The current focus ring uses --ads-v2-color-blue-300: #8bb0fa which may not provide sufficient contrast against light backgrounds like #fff used in the Select component. Consider using a darker blue shade (e.g., --ads-v2-color-blue-600: #2d6bf4) for better accessibility compliance.
app/client/packages/design-system/ads/src/__theme__/default/index.css: Update--ads-v2-color-outlineto use a higher contrast colorapp/client/packages/design-system/ads/src/Select/rc-styles.css: Multiple background colors (#fff, #ddd) need to be tested against the focus ring
🔗 Analysis chain
Verify focus ring contrast ratio
Please ensure the focus ring color (var(--ads-v2-color-outline)) provides sufficient contrast against all possible background colors for accessibility compliance.
Also applies to: 115-117
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the definition of --ads-v2-color-outline to verify contrast
rg --type css "ads-v2-color-outline|ads-v2-color-blue-300" -A 2
Length of output: 2365
Script:
#!/bin/bash
# Search for background colors that might be used with the focus ring
rg --type css "background|bg-color" app/client/packages/design-system/ads/src/Select/
Length of output: 1031
Script:
#!/bin/bash
# Search for color definitions in theme files to analyze contrast
rg --type css "color:|background-color:|bg-color:" app/client/packages/design-system/ads/src/__theme__/default/
Length of output: 1103
Script:
#!/bin/bash
# Check if there are any dark theme definitions
fd -e css -e ts -e js "dark" app/client/packages/design-system/ads/src/__theme__/
Length of output: 81
…o ads-v2/focus-ring
|
/build-deploy-preview skip-test=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12027252268. |
…o ads-v2/focus-ring
|
/build-deploy-preview skip-test=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12035598494. |
|
Deploy-Preview-URL: https://ce-37700.dp.appsmith.com |
Description
This PR adds focus ring for CTA's in the platform.
Fixes #37699
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Warning
Tests have not run on the HEAD 429e7b1 yet
Tue, 26 Nov 2024 17:10:06 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Chores