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 |
| outline: var(--ads-v2-border-width-outline) solid var(--ads-v2-color-outline); | ||
| outline-offset: var(--ads-v2-offset-outline); | ||
| .ads-v2-focus-ring, | ||
| :focus-visible { |
There was a problem hiding this comment.
Do we know exactly where we should apply styles? Can we use more specific selectors?
…psmith into ads-v2/focus-ring
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
…o ads-v2/focus-ring
|
/build-deploy-preview skip-test=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12357100341. |
| css` | ||
| outline: var(--ads-v2-border-width-outline) solid | ||
| var(--ads-v2-color-outline) !important; | ||
| outline-offset: var(--ads-v2-offset-outline) !important; |
There was a problem hiding this comment.
Why we need !important here?
There was a problem hiding this comment.
Some how this is conflicting with focus. I have added a different fix but that also look little confusing.
There was a problem hiding this comment.
when i disable :focus outline, it affecting :focus-visible also
| box-sizing: border-box; | ||
| width: 100%; | ||
|
|
||
| input:focus-visible { |
There was a problem hiding this comment.
As I understand, here we should also use focus and not focus-visible, right?
| outline-offset: var(--ads-v2-offset-outline); | ||
| } | ||
| ${({ isFocusVisible }) => | ||
| isFocusVisible && |
There was a problem hiding this comment.
Is only focus-visible needed for the button?
There was a problem hiding this comment.
Yeah we don need focus styles right
| @@ -1794,6 +1794,7 @@ class CodeEditor extends Component<Props, State> { | |||
| onMouseOver={this.handleMouseMove} | |||
| ref={this.editorWrapperRef} | |||
| removeHoverAndFocusStyle={this.props?.removeHoverAndFocusStyle} | |||
| showFocusRing={!this.props.isJSObject} | |||
There was a problem hiding this comment.
Why are we disabling focus ring for isJSObject? Why can't we implement it here?
I would also suggest using a different name for the prop, since the focusRing is name of hook from Spectrum and we can start using something else tomorrow. Let's call it showFocusVisible?
There was a problem hiding this comment.
This is JsObject editor. Showing a focus ring for entire editor doesn't look good.
| /** | ||
| Disable outline completely from all components | ||
| */ | ||
| :focus, |
There was a problem hiding this comment.
@jsartisan You have to see this!
There was a problem hiding this comment.
I don't think we cant escape till we remove blueprint components component. This is done because of blueprint CSS from what I know.
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has been closed because of inactivity. |
|
/build-deploy-preview skip-test=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12742095072. |
|
/ok-to-test |
|
Whoops! Looks like you're using an outdated method of running the Cypress suite. |
|
Deploy-Preview-URL: https://ce-37700.dp.appsmith.com |
|
/build-deploy-preview skip-test=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12742461505. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/packages/design-system/ads/src/Link/Link.styles.tsx (1)
94-95: Consider documenting the CSS custom properties.While the implementation is solid, it would be helpful to add JSDoc comments describing the expected values for
--ads-v2-color-outlineand--ads-v2-offset-outlinevariables.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/packages/design-system/ads/src/Link/Link.styles.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (1)
app/client/packages/design-system/ads/src/Link/Link.styles.tsx (1)
94-95: LGTM! Focus ring implementation aligns with accessibility best practices.The addition of
!importantflags ensures the focus outline styles aren't overridden, which is crucial for maintaining keyboard navigation accessibility.
|
/build-deploy-preview skip-test=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12742973071. |
|
Deploy-Preview-URL: https://ce-37700.dp.appsmith.com |
This reverts commit dd5a6d6.
Description
This PR adds focus ring for CTA's in the platform.
Fixes #37699
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12742601686
Commit: 2597087
Cypress dashboard.
Tags:
@tag.SanitySpec:
Mon, 13 Jan 2025 08:19:28 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
:focus-visible, enhancing accessibility.Bug Fixes
Chores