Skip to content

Revert "feat: Added focus ring for focus visible (#37700)" #38650

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

Merged
hetunandu merged 1 commit into from
Jan 14, 2025
Merged

Revert "feat: Added focus ring for focus visible (#37700)" #38650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 3 additions & 4 deletions app/client/packages/design-system/ads-old/src/SearchComponent/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,10 @@ const SearchInputWrapper = styled(InputGroup)<{
text-overflow: ellipsis;
width: 100%;
}

input:focus {
outline: var(--ads-v2-border-width-outline) solid
var(--ads-v2-color-outline);
outline-offset: var(--ads-v2-offset-outline);
border: 1.2px solid var(--ads-old-color-fern-green);
box-sizing: border-box;
width: 100%;
Comment on lines +67 to +69
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 14, 2025

Choose a reason for hiding this comment

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

🛠️ 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:

  1. Migrating to the new design system pattern
  2. 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.

}

input:active {
Expand Down
16 changes: 6 additions & 10 deletions app/client/packages/design-system/ads/src/Button/Button.styles.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,6 @@ export const StyledButton = styled.button<{
UNSAFE_width?: string;
disabled?: boolean;
isIconButton?: boolean;
isFocusVisible?: boolean;
}>`
${Variables}
/* Kind style */
Expand Down Expand Up @@ -270,14 +269,11 @@ export const StyledButton = styled.button<{
}
}

${({ isFocusVisible }) =>
isFocusVisible &&
css`
:focus-visible {
outline: var(--ads-v2-border-width-outline) solid
var(--ads-v2-color-outline) !important;
outline-offset: var(--ads-v2-offset-outline) !important;
}
`}
/* Focus styles */
&:focus-visible {
outline: var(--ads-v2-border-width-outline) solid
var(--ads-v2-color-outline);
outline-offset: var(--ads-v2-offset-outline);
}
}
`;
18 changes: 5 additions & 13 deletions app/client/packages/design-system/ads/src/Button/Button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import React, { forwardRef } from "react";
import clsx from "classnames";
import { useFocusRing } from "@react-aria/focus";

import { StyledButton, ButtonContent } from "./Button.styles";
import type { ButtonProps } from "./Button.types";
Expand All @@ -17,11 +16,6 @@ import {
} from "./Button.constants";
import { Spinner } from "../Spinner";

// Add this before the Button component definition
const SPINNER_ICON_PROPS = {
className: ButtonLoadingIconClassName,
};

/**
* TODO:
* - if both left and right icon is used, disregard left icon and print a warning in the console.
Expand All @@ -43,25 +37,21 @@ const Button = forwardRef<HTMLButtonElement, ButtonProps>(
...rest
} = props;

// Replace the direct mutation of rest.onClick with this
const handleClick =
// disable button when loading
rest.onClick =
props.isLoading || props.isDisabled ? undefined : props.onClick;
const buttonRef = useDOMRef(ref);
const { focusProps, isFocusVisible } = useFocusRing();

return (
<StyledButton
as={renderAs || "button"}
{...rest}
onClick={handleClick}
{...focusProps}
UNSAFE_height={UNSAFE_height}
UNSAFE_width={UNSAFE_width}
className={clsx(ButtonClassName, className)}
data-disabled={props.isDisabled || false}
data-loading={isLoading}
disabled={props.isDisabled}
isFocusVisible={isFocusVisible}
isIconButton={isIconButton}
kind={kind}
ref={buttonRef}
Expand All @@ -71,7 +61,9 @@ const Button = forwardRef<HTMLButtonElement, ButtonProps>(
{isLoading === true && (
<Spinner
className={ButtonLoadingClassName}
iconProps={SPINNER_ICON_PROPS}
iconProps={{
className: ButtonLoadingIconClassName,
}}
size="md"
/>
)}
Expand Down
22 changes: 12 additions & 10 deletions app/client/packages/design-system/ads/src/Input/Input.styles.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ export const StyledInput = styled.input<{
renderer === "input" &&
css`
padding-left: calc((var(--input-padding-x) * 2) + var(--icon-size) - 1px);
`}
`};

/* adjust padding end according to icon present or not */
${({ hasEndIcon, renderer }) =>
Expand All @@ -174,13 +174,20 @@ export const StyledInput = styled.input<{
padding-right: calc(
(var(--input-padding-x) * 2) + var(--icon-size) - 1px
);
`}
`};

&:focus:enabled:not(:read-only) {
outline: var(--ads-v2-border-width-outline) solid
var(--ads-v2-color-outline);
outline-offset: var(--ads-v2-offset-outline);
}

&:hover:enabled:not(:read-only) {
--input-color-border: var(--ads-v2-colors-control-field-hover-border);
}

&:active:enabled:not(:read-only) {
&:active:enabled:not(:read-only),
&:focus:enabled:not(:read-only) {
--input-color-border: var(--ads-v2-colors-control-field-active-border);
}

Expand All @@ -196,16 +203,11 @@ export const StyledInput = styled.input<{
--input-color-border: var(--ads-v2-colors-control-field-error-border);
}

&:active:enabled:not(:read-only) {
&:active:enabled:not(:read-only),
&:focus:enabled:not(:read-only) {
--input-color-border: var(--ads-v2-colors-control-field-error-border);
}
}

&:focus {
outline: var(--ads-v2-border-width-outline) solid
var(--ads-v2-color-outline) !important;
outline-offset: var(--ads-v2-offset-outline) !important;
}
`;

export const Description = styled(Text)`
Expand Down
16 changes: 8 additions & 8 deletions app/client/packages/design-system/ads/src/Input/Input.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React, { forwardRef, useCallback } from "react";
import React, { forwardRef } from "react";
import { useFocusRing } from "@react-aria/focus";
import { useTextField } from "@react-aria/textfield";
import clsx from "classnames";

Expand Down Expand Up @@ -54,7 +55,7 @@ const Input = forwardRef<HTMLInputElement, InputProps>(
const { descriptionProps, errorMessageProps, inputProps, labelProps } =
// @ts-expect-error fix this the next time the file is edited
useTextField(props, inputRef);

const { focusProps, isFocusVisible } = useFocusRing();
const {
className: startIconClassName,
onClick: startIconOnClick,
Expand All @@ -66,12 +67,9 @@ const Input = forwardRef<HTMLInputElement, InputProps>(
...restOfEndIconProps
} = endIconProps || {};

const handleOnChange = useCallback(
(event: React.ChangeEvent<HTMLInputElement>) => {
onChange?.(event.target.value);
},
[onChange],
);
const handleOnChange = (event: React.ChangeEvent<HTMLInputElement>) => {
onChange?.(event.target.value);
};

isValid = isValid === undefined ? !errorMessage : isValid;

Expand Down Expand Up @@ -118,6 +116,7 @@ const Input = forwardRef<HTMLInputElement, InputProps>(
<StyledInput
as={renderAs}
type={type}
{...focusProps}
{...inputProps}
UNSAFE_height={UNSAFE_height}
UNSAFE_width={UNSAFE_width}
Expand All @@ -127,6 +126,7 @@ const Input = forwardRef<HTMLInputElement, InputProps>(
hasEndIcon={!!endIcon}
hasStartIcon={!!startIcon}
inputSize={size}
isFocusVisible={isFocusVisible}
onChange={handleOnChange}
readOnly={isReadOnly}
ref={inputRef}
Expand Down
4 changes: 2 additions & 2 deletions app/client/packages/design-system/ads/src/Link/Link.styles.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,8 @@ export const Styles = css<{ kind?: LinkKind }>`

:focus-visible {
outline: var(--ads-v2-border-width-outline) solid
var(--ads-v2-color-outline) !important;
outline-offset: var(--ads-v2-offset-outline) !important;
var(--ads-v2-color-outline);
outline-offset: var(--ads-v2-offset-outline);
border-radius: var(--ads-v2-border-radius);
}
`;
Expand Down
4 changes: 2 additions & 2 deletions app/client/packages/design-system/ads/src/SegmentedControl/SegmentedControl.styles.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ export const StyledControlContainer = styled.div`

&:focus-visible {
outline: var(--ads-v2-border-width-outline) solid
var(--ads-v2-color-outline) !important;
outline-offset: var(--ads-v2-offset-outline) !important;
var(--ads-v2-color-outline);
outline-offset: var(--ads-v2-offset-outline);
}

&[data-disabled="true"] {
Expand Down
12 changes: 3 additions & 9 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
@@ -1,9 +1,7 @@
.ads-v2-select {
--select-color-border: var(--ads-v2-colors-control-field-default-border);
--select-padding-x: var(--ads-v2-spaces-2);
/* padding left and right */
--select-padding-y: var(--ads-v2-spaces-2);
/* padding top and bottom */
--select-padding-x: var(--ads-v2-spaces-2); /* padding left and right */
--select-padding-y: var(--ads-v2-spaces-2); /* padding top and bottom */
--select-font-size: var(--ads-v2-font-size-2);
--select-height: 24px;
--select-search-input-padding-right: 0;
Expand Down Expand Up @@ -53,6 +51,7 @@
.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);
}

/* Error */
Expand Down Expand Up @@ -88,7 +87,6 @@
/* padding x + icon size + gap */
--select-search-input-padding-right: calc(var(--select-padding-x) + 16px);
}

.ads-v2-select.rc-select-allow-clear.rc-select-show-arrow.rc-select-selection-search-input,
.ads-v2-select.rc-select-allow-clear.rc-select-show-arrow
.rc-select-selection-overflow {
Expand All @@ -115,10 +113,6 @@
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
5 changes: 2 additions & 3 deletions app/client/packages/design-system/ads/src/Switch/Switch.styles.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,8 @@ export const StyledSwitchInput = styled.input<{
${({ isFocusVisible }) =>
isFocusVisible &&
`
outline: var(--ads-v2-border-width-outline) solid
var(--ads-v2-color-outline) !important;
outline-offset: var(--ads-v2-offset-outline) !important;
outline: var(--ads-v2-border-width-outline) solid var(--ads-v2-color-outline);
outline-offset: var(--ads-v2-offset-outline);
`}

&:hover {
Expand Down
4 changes: 2 additions & 2 deletions app/client/packages/design-system/ads/src/Tab/Tab.styles.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ export const StyledTab = styled(RadixTabs.TabsTrigger)`
&:focus-visible {
--tab-color: var(--ads-v2-colors-content-label-default-fg);
outline: var(--ads-v2-border-width-outline) solid
var(--ads-v2-color-outline) !important;
outline-offset: var(--ads-v2-offset-outline) !important;
var(--ads-v2-color-outline);
outline-offset: var(--ads-v2-offset-outline);
}
`;
7 changes: 1 addition & 6 deletions app/client/packages/design-system/ads/src/Text/Text.styles.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,13 +201,8 @@ export const StyledEditableInput = styled.input`
border-color: var(--ads-v2-colors-control-field-hover-border);
}

&:focus,
&:active {
border-color: var(--ads-v2-colors-control-field-active-border);
}

&:focus-visible {
outline: var(--ads-v2-border-width-outline) solid
var(--ads-v2-color-outline);
outline-offset: var(--ads-v2-offset-outline);
}
`;
17 changes: 6 additions & 11 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: var(--ads-v2-color-blue-300);
--ads-v2-border-width-outline: 2px;
--ads-v2-offset-outline: -2px;
--ads-v2-color-outline: transparent;
--ads-v2-border-width-outline: 0;
--ads-v2-offset-outline: 0;
Comment on lines +242 to +244
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 14, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
--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 @@ -251,22 +251,17 @@
--ads-v2-opacity-disabled: 0.6;
}

:focus {
outline: none !important;
}

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

/* Placeholder color */
::placeholder {
/* This needs to be important to override the placeholder color on main repo */
color: var(--ads-v2-colors-control-placeholder-default-fg) !important;
opacity: 1;
/* Firefox */
opacity: 1; /* Firefox */
}

:-ms-input-placeholder {
Expand Down
1 change: 0 additions & 1 deletion app/client/src/components/editorComponents/CodeEditor/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1794,7 +1794,6 @@ class CodeEditor extends Component<Props, State> {
onMouseOver={this.handleMouseMove}
ref={this.editorWrapperRef}
removeHoverAndFocusStyle={this.props?.removeHoverAndFocusStyle}
showFocusVisible={!this.props.isJSObject}
size={size}
>
{this.state.peekOverlayProps && (
Expand Down
21 changes: 5 additions & 16 deletions app/client/src/components/editorComponents/CodeEditor/styledComponents.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ export const EditorWrapper = styled.div<{
AIEnabled?: boolean;
mode: string;
maxHeight?: string | number;
showFocusVisible?: boolean;
}>`
// Bottom border was getting clipped
.CodeMirror.cm-s-duotone-light.CodeMirror-wrap {
Expand Down Expand Up @@ -84,17 +83,6 @@ export const EditorWrapper = styled.div<{
text-transform: none;

&& {
${(props) =>
props.showFocusVisible &&
`
.CodeMirror-focused {
outline: var(--ads-v2-border-width-outline) solid
var(--ads-v2-color-outline) !important;
outline-offset: var(--ads-v2-offset-outline) !important;
z-index: 1 !important;
}
`}

.CodeMirror-cursor {
border-right: none;
border-left-width: 2px;
Expand Down Expand Up @@ -402,11 +390,12 @@ export const EditorWrapper = styled.div<{
}
}

&:focus-visible {
&:focus {
.CodeMirror.cm-s-duotone-light {
outline: var(--ads-v2-border-width-outline) solid
var(--ads-v2-color-outline);
outline-offset: var(--ads-v2-offset-outline);
border-color: ${(props) =>
props.borderLess
? "none"
: "var(--ads-v2-color-border-emphasis-plus)"};
}
}

Expand Down
Loading