Revert "feat: Added focus ring for focus visible (#37700)" #38650
Conversation
This reverts commit dd5a6d6.
WalkthroughThis pull request introduces comprehensive modifications to focus styles across various components in the design system. The changes primarily involve removing Changes
Suggested labels
Suggested reviewers
Possibly related PRs
Poem
Finishing Touches
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
app/client/packages/design-system/ads/src/Button/Button.tsx (1)
40-41: Consider extracting click handler logic for better readability.The inline condition handling both loading and disabled states could be clearer if extracted into a separate function or variable.
- rest.onClick = - props.isLoading || props.isDisabled ? undefined : props.onClick; + const isInteractionDisabled = props.isLoading || props.isDisabled; + rest.onClick = isInteractionDisabled ? undefined : props.onClick;app/client/packages/design-system/ads/src/Switch/Switch.styles.tsx (1)
75-76: Consider using :focus-visible pseudo-class for consistency.While removing
!importantis good, consider refactoring to use the:focus-visiblepseudo-class instead of theisFocusVisibleprop to align with the pattern used in other components.- ${({ isFocusVisible }) => - isFocusVisible && - ` - outline: var(--ads-v2-border-width-outline) solid var(--ads-v2-color-outline); - outline-offset: var(--ads-v2-offset-outline); - `} + &:focus-visible { + outline: var(--ads-v2-border-width-outline) solid var(--ads-v2-color-outline); + outline-offset: var(--ads-v2-offset-outline); + }app/client/src/components/editorComponents/CodeEditor/styledComponents.ts (1)
393-399: Focus styles should maintain sufficient contrast for accessibility.The focus indicator should be clearly visible to meet WCAG 2.1 success criterion 2.4.7 (Focus Visible).
Consider adding a higher contrast color or combining both outline and border:
&:focus { + outline: var(--ads-v2-border-width-outline) solid var(--ads-v2-color-outline); border-color: ${(props) => props.borderLess ? "none" : "var(--ads-v2-color-border-emphasis-plus)"}; }app/client/packages/design-system/ads/src/Input/Input.tsx (1)
Line range hint
119-129: Focus props implementation looks good.The spread of focusProps and addition of isFocusVisible prop maintains proper focus management while keeping the component's API clean.
However, consider adding a type for the isFocusVisible prop in Input.types.tsx:
export interface InputProps extends HTMLInputProps { // ... existing props isFocusVisible?: boolean; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
app/client/packages/design-system/ads-old/src/SearchComponent/index.tsx(1 hunks)app/client/packages/design-system/ads/src/Button/Button.styles.tsx(1 hunks)app/client/packages/design-system/ads/src/Button/Button.tsx(2 hunks)app/client/packages/design-system/ads/src/Input/Input.styles.tsx(3 hunks)app/client/packages/design-system/ads/src/Input/Input.tsx(5 hunks)app/client/packages/design-system/ads/src/Link/Link.styles.tsx(1 hunks)app/client/packages/design-system/ads/src/SegmentedControl/SegmentedControl.styles.tsx(1 hunks)app/client/packages/design-system/ads/src/Select/styles.css(2 hunks)app/client/packages/design-system/ads/src/Switch/Switch.styles.tsx(1 hunks)app/client/packages/design-system/ads/src/Tab/Tab.styles.tsx(1 hunks)app/client/packages/design-system/ads/src/Text/Text.styles.tsx(1 hunks)app/client/packages/design-system/ads/src/__theme__/default/index.css(2 hunks)app/client/src/components/editorComponents/CodeEditor/index.tsx(0 hunks)app/client/src/components/editorComponents/CodeEditor/styledComponents.ts(1 hunks)app/client/src/components/propertyControls/ColorPickerComponentV2.tsx(1 hunks)app/client/src/components/propertyControls/IconSelectControl.tsx(1 hunks)app/client/src/components/propertyControls/IconSelectControlV2.tsx(1 hunks)app/client/src/index.css(1 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/components/editorComponents/CodeEditor/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (10)
- 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-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (14)
app/client/packages/design-system/ads/src/Button/Button.tsx (1)
64-66: LGTM! Clean inline props definition.The inline iconProps definition is clear and maintains the required className.
app/client/packages/design-system/ads/src/Tab/Tab.styles.tsx (1)
91-92: LGTM! Good cleanup of focus styles.Removing
!importantflags improves style maintainability while preserving the focus ring functionality.app/client/packages/design-system/ads/src/SegmentedControl/SegmentedControl.styles.tsx (1)
70-71: LGTM! Consistent with design system standards.The focus styles properly use design system variables and follow the same pattern as other components.
app/client/packages/design-system/ads/src/Link/Link.styles.tsx (1)
94-95: LGTM! Removal of!importantflags improves style maintainability.The changes align with CSS best practices by removing unnecessary
!importantflags while maintaining the same focus behavior.app/client/packages/design-system/ads/src/Input/Input.styles.tsx (1)
179-183: LGTM! Focus styles are well-structured and accessible.The changes properly handle focus states with appropriate constraints (enabled, not read-only) and maintain consistency with the design system.
Also applies to: 189-190, 206-207
app/client/packages/design-system/ads/src/Text/Text.styles.tsx (1)
204-207: LGTM! Focus and active states are properly handled.The changes maintain visual feedback for interaction states while using consistent design system variables.
app/client/packages/design-system/ads/src/Button/Button.styles.tsx (1)
272-277: LGTM! Focus styles follow accessibility best practices.The use of :focus-visible pseudo-class improves keyboard navigation while maintaining consistency with the design system.
app/client/src/components/propertyControls/IconSelectControl.tsx (1)
55-59: LGTM! Good accessibility implementation.The focus styles combine both outline and border, providing clear visual feedback for keyboard navigation.
app/client/src/components/propertyControls/IconSelectControlV2.tsx (1)
56-60: LGTM! Consistent with V1 implementation.The focus styles maintain consistency with IconSelectControl.tsx, which is good for maintainability.
app/client/src/components/propertyControls/ColorPickerComponentV2.tsx (1)
Line range hint
98-110: LGTM! Well-implemented focus and hover states.The component properly handles both keyboard and mouse interactions with appropriate visual feedback.
app/client/packages/design-system/ads/src/Select/styles.css (1)
3-4: LGTM: Documentation improvementsThe changes only improve variable documentation with inline comments.
app/client/packages/design-system/ads/src/Input/Input.tsx (3)
1-2: Import cleanup looks good.The removal of unused imports and addition of necessary ones improves code cleanliness.
58-58: LGTM: Focus ring implementation enhances accessibility.The addition of useFocusRing hook from react-aria improves keyboard navigation accessibility by providing proper focus indicators.
70-72: Verify the impact of removing useCallback.While the simplified handleOnChange implementation is cleaner, removing useCallback might affect performance for parent components that rely on memoization.
Let's check for potential performance implications:
✅ Verification successful
Removal of useCallback is safe here
The onChange handler is a simple pass-through function with no dependencies. Given its location in a design system package and simple implementation, wrapping it in useCallback provides no meaningful performance benefit.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for parent components that might be affected by the callback change rg -l "Input.*onChange" --type ts --type tsx | xargs rg "memo|useMemo|React\.memo" -lLength of output: 28299
Script:
#!/bin/bash # Find files importing Input from design-system ast-grep --pattern 'import { $$$, Input, $$$ } from "$_"' --lang ts,tsx # Also find direct Input component usage ast-grep --pattern '<Input $$$>' --lang ts,tsxLength of output: 348
Script:
#!/bin/bash # Find Input component imports and usages rg "import.*Input.*from.*design-system" --type ts --type tsx -l | \ xargs rg "React\.memo|useMemo|memo" -l # Also find direct Input usages and check for memo rg "<Input\s" --type ts --type tsx -l | \ xargs rg "React\.memo|useMemo|memo" -lLength of output: 56611
Script:
#!/bin/bash # Find files that import Input from design-system/ads rg "from ['\"].*design-system/ads.*['\"]" --type ts --type tsx -C 2 | rg "Input" # Find direct Input component usage with surrounding context rg "<Input\s[^>]*>" --type ts --type tsx -C 5Length of output: 188
Script:
#!/bin/bash # Find Input component imports and usage with context rg "import.*Input.*from.*design-system/ads" --type typescript -B 2 -A 5 # Find direct Input usage with surrounding component context rg -B 10 -A 10 "<Input\b[^>]*>" --type typescript | rg -B 10 -A 10 "memo|useMemo"Length of output: 227
Script:
#!/bin/bash # Find files with Input component imports and memo usage rg -l "import.*Input.*from.*@design-system" | xargs rg -l "React\.memo|useMemo" # Show context around Input usage in those files rg -l "import.*Input.*from.*@design-system" | xargs rg -C 10 "<Input\b.*onChange"Length of output: 20621
Script:
#!/bin/bash # Find Input component usage in design-system package cd app/client/packages/design-system && rg -C 10 "<Input\b.*onChange" # Also check direct parent components for memo usage cd app/client/packages/design-system && rg -C 10 "export.*Input"Length of output: 245
| border: 1.2px solid var(--ads-old-color-fern-green); | ||
| box-sizing: border-box; | ||
| width: 100%; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider maintaining consistency with the new design system.
This component uses a different focus style (border) compared to the outline approach used in the new design system components. Since this is in the 'ads-old' package, consider:
- Migrating to the new design system pattern
- Using outline instead of border to avoid layout shifts
input:focus {
- border: 1.2px solid var(--ads-old-color-fern-green);
- box-sizing: border-box;
- width: 100%;
+ outline: var(--ads-v2-border-width-outline) solid var(--ads-v2-color-outline);
+ outline-offset: var(--ads-v2-offset-outline);
}Committable suggestion skipped: line range outside the PR's diff.
| /** | ||
| Disable outline completely from all components | ||
| */ | ||
| :focus, | ||
| :focus-visible { | ||
| outline: none !important; | ||
| outline-offset: 0 !important; | ||
| } |
There was a problem hiding this comment.
Critical: Removing focus indicators violates accessibility guidelines
Disabling focus outlines completely breaks keyboard navigation accessibility and violates WCAG 2.1 Success Criterion 2.4.7 (Focus Visible). Focus indicators are essential for keyboard users to track their position on the page.
Remove these rules to restore focus visibility:
-/**
- Disable outline completely from all components
-*/
-:focus,
-:focus-visible {
- outline: none !important;
- outline-offset: 0 !important;
-}| /** | ||
| Disable outline for adsv2 select component | ||
| */ | ||
| .ads-v2-select.rc-select-focused { | ||
| outline: none !important; | ||
| outline-offset: 0 !important; | ||
| } |
There was a problem hiding this comment.
Remove select component focus outline override
This specific override for select components also impacts accessibility. Focus indicators should be consistent across all interactive elements.
Remove these rules:
-/**
- Disable outline for adsv2 select component
-*/
-.ads-v2-select.rc-select-focused {
- outline: none !important;
- outline-offset: 0 !important;
-}| --ads-v2-color-outline: transparent; | ||
| --ads-v2-border-width-outline: 0; | ||
| --ads-v2-offset-outline: 0; |
There was a problem hiding this comment.
Revert outline variable changes to maintain accessibility
Setting outline to transparent and width to 0 removes visual focus indicators at the theme level. This impacts all components using these variables and breaks keyboard navigation accessibility.
Restore the previous values:
- --ads-v2-color-outline: transparent;
- --ads-v2-border-width-outline: 0;
- --ads-v2-offset-outline: 0;
+ --ads-v2-color-outline: var(--ads-v2-color-blue-300);
+ --ads-v2-border-width-outline: 2px;
+ --ads-v2-offset-outline: -2px;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| --ads-v2-color-outline: transparent; | |
| --ads-v2-border-width-outline: 0; | |
| --ads-v2-offset-outline: 0; | |
| --ads-v2-color-outline: var(--ads-v2-color-blue-300); | |
| --ads-v2-border-width-outline: 2px; | |
| --ads-v2-offset-outline: -2px; |
This reverts commit dd5a6d6.
Doing a revert because certain tests have started failing after this chnage
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12764950615
Commit: 887a1c9
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/Binding/Button_Text_WithRecaptcha_spec.js
List of identified flaky tests.Tue, 14 Jan 2025 11:08:40 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Design System Updates
!importantflags from focus styles across multiple componentsAccessibility Changes
CSS Modifications