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

feat: Added focus ring for focus visible #37700

Merged
merged 24 commits into from
Jan 13, 2025
+103 −81
Merged

feat: Added focus ring for focus visible #37700

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
24 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
19e0258
Merge branch 'release' of https://github.com/appsmithorg/appsmith int…
albinAppsmith Dec 3, 2024
897ed4f
fix: updated focus outline as none
albinAppsmith Dec 3, 2024
cbf6b75
Merge branch 'ads-v2/focus-ring' of https://github.com/appsmithorg/ap…
albinAppsmith Dec 5, 2024
91a4e73
Merge branch 'release' of https://github.com/appsmithorg/appsmith int…
albinAppsmith Dec 16, 2024
99f4306
fix: focus ring
albinAppsmith Dec 16, 2024
bbed9e0
fix: input focu visible issue
albinAppsmith Dec 17, 2024
04ccdf8
Merge branch 'release' of https://github.com/appsmithorg/appsmith int…
albinAppsmith Dec 17, 2024
cbf01c3
fix: focus issues
albinAppsmith Dec 17, 2024
0707ddd
fix: date puicker focus
albinAppsmith Dec 17, 2024
0b4601e
fix: input focus state
albinAppsmith Dec 17, 2024
bd47af2
Merge branch 'release' of https://github.com/appsmithorg/appsmith int…
albinAppsmith Dec 17, 2024
bf0ef8e
fix: Added focus for codemirror
albinAppsmith Dec 18, 2024
b5595de
fix: flag name change
albinAppsmith Dec 18, 2024
4db67f4
Merge branch 'release' of https://github.com/appsmithorg/appsmith int…
albinAppsmith Dec 18, 2024
f0ed3ff
fix: z-index
albinAppsmith Dec 18, 2024
4f67900
Merge branch 'release' of github.com:appsmithorg/appsmith into ads-v2…
albinAppsmith Jan 10, 2025
60f67c5
Merge branch 'release' of github.com:appsmithorg/appsmith into ads-v2…
albinAppsmith Jan 13, 2025
e7483f4
fix: review comments
albinAppsmith Jan 13, 2025
be0c6b4
fix: button focus visible
albinAppsmith Jan 13, 2025
2597087
fix: link styles
albinAppsmith Jan 13, 2025
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 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we know exactly where we should apply styles? Can we use more specific selectors?

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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jsartisan You have to see this!

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we cant escape till we remove blueprint components component. This is done because of blueprint CSS from what I know.

: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