Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Added focus ring for focus visible #37700

Open
wants to merge 4 commits into
base: release
Choose a base branch
from
+12 −25
Open

feat: Added focus ring for focus visible #37700

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
15e3a88
feat: Added focus ring
albinAppsmith Nov 26, 2024
f303d56
Merge branch 'release' of https://github.com/appsmithorg/appsmith int…
albinAppsmith Nov 26, 2024
cb448a0
Merge branch 'release' of https://github.com/appsmithorg/appsmith int…
albinAppsmith Nov 26, 2024
429e7b1
Merge branch 'release' of https://github.com/appsmithorg/appsmith int…
albinAppsmith Nov 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions app/client/packages/design-system/ads/src/Select/styles.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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-outline to use a higher contrast color
  • app/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

}

/* Error */
Expand Down Expand Up @@ -113,6 +112,10 @@
overflow: hidden;
}

.ads-v2-select.rc-select-focused > .rc-select-selector {
border-color: transparent;
}

/* Placeholder */
.ads-v2-select > .rc-select-selector > .rc-select-selection-placeholder,
.ads-v2-select > .rc-select-selector > .rc-select-selection-item {
Expand Down
13 changes: 7 additions & 6 deletions app/client/packages/design-system/ads/src/__theme__/default/index.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,9 +239,9 @@
/* --ads-v2-color-outline: var(--ads-v2-color-blue-300);
--ads-v2-border-width-outline: 2px; */
/* TODO: fix focus issue across the platform */
--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;

/**
* ===========================================*
Expand All @@ -252,9 +252,10 @@
}

/* react-aria useFocusRing helper class*/
.ads-v2-focus-ring {
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 {
outline: var(--ads-v2-border-width-outline) solid var(--ads-v2-color-outline) !important;
outline-offset: var(--ads-v2-offset-outline) !important;
}

/* Placeholder color */
Expand Down
17 changes: 0 additions & 17 deletions app/client/src/index.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,23 +22,6 @@ body.dragging * {
cursor: grabbing !important;
}

/**
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;
}

#root.overlay {
opacity: 0;
pointer-events: none;
Expand Down
Loading